please make the process of rejecting revisions easier

[Jamie Strandboge]

Celso and I discussed this in a recent meeting, but adding a bug so it can be tracked.

Currently the store queues reviews with the idea that if one review fails, then subsequent reviews will also fail, so it queues them such that if one fails, the review on the others is blocked. When a reviewer performs an action on the failed-review revision, then the next one is reviewed. This allows the reviewer to, for example, adjust the snap declaration such that all other reviews pass automated review. This is very helpful when a developer uploads the same package for 6 different architectures but it fails due to a missing snap declaration. The reviewer sees the failed review, issues a snap declaration, approves the snap and then the next arch is reviewed, it passes now because there is a snap declaration, and so on until all are unblocked.

The store process for rejecting revisions does not work in the same way because the store doesn't remember the results of the previous review so in the example of the 6 different archs failing in the same way, the reviewer needs to manually reject each of the 6 revisions. For something that uses LP for automatic uploads to edge, things get out of control fast (eg, https://myapps.developer.ubuntu.com/dev/click-apps/5873/) and 10s of failed revisions might queue up. What makes matters worse is that for large snaps, the reviewer has to wait 30 seconds or more for the review to finish.

Celso and I agreed that the store should store the results of (at least) the last click-review such that if the reviewer rejects a revision, then when the next revision is reviewed by the store, the results are compared. If they are the same, auto-reject this revision and move to the next. If they are different, block for human review again.

[林博仁(Buo-ren, Lin) (buo-ren-lin)]

Also, I would like to request manually revoke all review request by the packager.

[Jamie Strandboge (jdstrand)]

This continues to be a problem. Eg, https://dashboard.snapcraft.io/snaps/ssh-tunnel/revisions/13/feedback/ failed on revision 1 and has 174 revisions. I've so far rejected 13 revisions that ended up in the manual review queue with the same error. I don't know how many of the 174 revisions have the same error, but I'm not particularly looking forward to manually rejecting all of these.

[Jamie Strandboge (jdstrand)]

A new one demonstrates the problem: https://dashboard.snapcraft.io/snaps/i2pbrowser. They all (so far have the same failure) and there are 124 revisions left (I've been trying to catch up, but it is a large snap and it takes a long time for the review to complete.

Honestly, I'm surprised all of these are hitting the manual review queue. It seems they are being auto-uploaded or something and defaulting to needing a manual review rather than a human requesting it. Is the logic for adding to manual reviews correct for this snap?

[Facundo Batista (facundo)]

Hello Jamie!

I've started to think about this problem and how to solve it.

Two big questions arise that internally we don't know exactly how to answer. Feedback from you is welcomed.

One question is what we can consier as the "same error". The automatic review process ends with all fine, or a set of errors and warnings. We can consider "same error" between reviews if they have the exactly same *errors* and exactly same *warnings*, and this is our initial approach (which can be relaxed later). However, if you think that we can just check for same set of *errors*, ignoring the *warnings*, or something different, let's talk.

The second question is a little more tricky. It's about the sequence of revisions. If a couple of revisions are uploaded with the same error, it's clear what happens. But in the middle of the sequence, we can have different situations, like an approved revision, or different problems. Let me exemplify these two cases, which will help us to think about the problem.

Case 1: The developer could be releasing...

    rev N from project's master to edge, automatic review ending with problem X
    rev N+1 from master to edge again, having the same problem X
    rev N+2 from a project's branch to stable/fix123, automatic review approving
    rev N+3 again from master to edge, still having problem X

If you go to rev N and manually reject it, it's clear that N+1 should "follow the same fate". But what about N+3?

Case 2: Different problems, mostly because uploading from different project's branches

    rev N with problem X
    rev N+1 with problem X
    rev N+2 with problem Y
    rev N+3 with problem Y
    rev N+4 with problem X
    rev N+5 with problem Y

If you got to rev N and manually reject it, of course we want N+1 to be automatically rejected. N+2 shows a different error, manually intervention is required, and let's say it's also rejected; we want N+3 to follow the automatic rejection too. Then N+4 shows a different problem than N+3, but it's the same one that was manually rejected in the past! Same situation with N+5. What should happen in these cases?

One way to think about these cases and make a decision about the behaviour we want is to forget about "timelines of revisions" and declare that "if somewhen in the past a revision with a specific set of problems is manually rejected, then a new revision that present the same problems, should be rejected too".

What do you think? Thanks for your time!

[Jamie Strandboge (jdstrand)]

"However, if you think that we can just check for same set of *errors*, ignoring the *warnings*, or something different, let's talk."

For now do both since either triggers a manual review.

As for the timelines of revisions or lack thereof, I'm not too concerned about the case of a stable release being approved in between: ie, I'm ok if N+3 is back in the queue when 'rev N+2 from a project's branch to stable/fix123, automatic review approving' happened before it. However, perhaps it makes sense to also look at the channel, such that only autoreject N+1 if it has the same errors, warnings *and* channel as N.

As for the more involved case 2, I'm unsure it is actually desirable to autoreject N+4 and N+5 since it is possible that someone might resubmit with justification to approve. Perhaps that is covered by other code....

The main thing I'd like to see made easier is when edge is hooked up with autouploads and 200 things queue up and end up in a manual review. It is probably sufficient simply to autoreject N+M until something is different and queue that.

[Jamie Strandboge (jdstrand)]

Btw, thank you *so* much for looking into this! :)

[Daniel Manrique (roadmr)]

Unfortunately we can't consider channel at review-tools runtime because we don't know! :( from the store's point of view the flow is:

1- Snap is uploaded, no idea which channel is desired
2- Review tools run
3- Decision, either auto or manual
4- If approved, we're done.

And additionally, separately, via a different API call acting on the already-approved upload:
5- Later, developer decides to publish snap on channel foo.

From the store's point of view, they're separate things; tools such as snapcraft, Launchpad and build.snapcraft.io give the illusion that the channel decision is made at upload time, but behind the scenes they push the snap, wait to receive ack from the store (at step 4 above), then separately do the request to release it to a channel.

Adding "intent to release" as a store-side concept or primitive has been requested in https://bugs.launchpad.net/snapstore/+bug/1684529, once that's ready we might be able to incorporate this additional data point in automated rejection decisions but until then, tools review output is the only input to the "shall I reject automatically?" function/output.

[Facundo Batista (facundo)]

Thanks Jamie for the feedback!

So, in conclusion:

- I'll compare for both errors and warnings

- I'll just check with the immediate preceding revision (will have the effect of not autoreject N+4 and N+5 from the example, and we don't care about "middle approvals")

Thanks again!