Need for manual review loses intent to release to channel

[Evan]

Steps to reproduce:
- Push a classic snap to the edge channel

What happens:
- The snap is flagged for manual review
- After successful review the snap is not published to edge

What should happen:
- The snap is flagged for manual review
- After successful review the snap is published to edge

Demonstrated here:

☻ snapcraft push --release=edge awscli_1.11.77_amd64.snap
Preparing to push 'awscli_1.11.77_amd64.snap' to the store.
Pushing awscli_1.11.77_amd64.snap [===================================================================================================================================================================] 100%
Processing...|
Will need manual review...
Publishing checks failed.
To release this to stable channel please request a review on the snapcraft list.
Use devmode in the edge or beta channels to disable confinement.
  - (NEEDS REVIEW) confinement 'classic' not allowed

☻ snapcraft status awscli
Track Arch Channel Version Revision

☻ snapcraft revisions awscli
Rev. Uploaded Arch Version Channels
1 2017-04-20T09:08:29Z amd64 1.11.77 -

☻ snapcraft release awscli 1 edge
Track Arch Channel Version Revision
latest amd64 stable - -
                 candidate - -
                 beta - -
                 edge 1.11.77 1
The 'edge' channel is now open.

[Daniel Manrique (roadmr)]

While snapcraft provides the --release flag as a convenience, in reality the process happens in distinct stages:

- First the snap is pushed to the store
- Once pushed, it undergoes review. Snapcraft waits to hear from the store on the result.
- If the store says the snap is ready to publish (passed automated review) then snapcraft issues a publish API request to the specified channel.

But note that if the store says the snap is NOT ready to publish (held for manual review, held behind another upload pending review or outright failed automated review) then snapcraft exits and doesn't have a chance to issue the publish request. Like you observed, an explicit snapcraft release is required in order to publish at this point.

Keeping these steps separated store-side makes sense since, for instance, there's no guarantee about how long the snap will take to complete its review. If, hypothetically, the snap were pushed with some bit saying "publish to stable when it's reviewed", and it gets held for review, it could end up being published hours or days after being uploaded, a behavior which could be surprising for developers or users.

Also, we've seen cases of developers changing their snaps in response to automated review error messages, this suggests sometimes the automated review did catch problems which the developer prefers not having published. If a reviewer happens to miss one of those errors, approves the snap and it gets auto-published from under the developer, an undesirable snap may end up being published. Halting the publishing process in the face of automated review failure makes sense in this context.

To be clear, the behavior you describe can be implemented, though it will require design and implementation with some consideration of the above and other possible scenarios.

[Evan (ev)]

> But note that if the store says the snap is NOT ready to publish (held for
> manual review, held behind another upload pending review or outright failed
> automated review) then snapcraft exits and doesn't have a chance to issue the
> publish request. Like you observed, an explicit snapcraft release is required
> in order to publish at this point.

Correct me if I'm wrong, but they are not told of this behaviour by the store or by snapcraft. They are assumed to know that if review passes, they need to manually release into the originally intended channel.

We can debate whether it happens automatically, but would you agree that:

A) snapcraft should say that snap is held for manual review and the developer will be contacted by email with further instructions

B) If the review passes, an email is send that tells them they need to manually release into a channel (with an example command) if they still desire to do that

Stretch) The store remembers the channel the developer tried to release into and includes that in the above example command

[Kyle Fazzari (kyrofa)]

I just want to add: not only is that single snap not released, but _every snap that is pushed after it_ loses the release intent as well. The store hiccuped on the first of like 10 snaps pushed from my CI today, showing a "scan error." Not only did that hold up the automated review of every following snap requiring manual intervention, but the release intent of every other snap pushed was lost. In many cases (since versions don't need to change) it's impossible to determine which release should go to which channel using only version and revision, requiring someone to take _further_ manual interaction to spin up new builds from CI, new uploads, and new releases.

[Adam Collard (adam-collard)]

Colin, suspect this might be impacted by the retry work that we're doing on LP?

[Launchpad QA Bot (lpqabot)]

Fixed in stable r18988 <http://bazaar.launchpad.net/~launchpad-pqm/launchpad/stable/revision/18988>.

[Colin Watson (cjwatson)]

There are one or two more bits to fix with how Launchpad presents the case where a pushed snap is held for manual review, but it's now passing release intents through to the store and the store is handling it from there.