1176321 - Better indicators for review states

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect, P1)

Description

[Dan Minor [:dminor]]

We'll need to develop a user interface in Mozreview for the Autoland to "inbound" (where "inbound" may be a tree other than mozilla-inbound, see bug 1170861) feature.

One consideration is how much logic we want to put into Mozreview about whether the Autoland request can be made. At a minimum, we'll want to ensure that every revision to be landed has received a "ship it".

Since we track Try requests made through Mozreview, we could also put logic here about ensuring that a Try run has occurred for each revision to be landed. The user would then have control over exactly what is tested on Try prior to landing. This can conserve resources, but increase the chance of breakage on landing.

An alternative is to have Autoland request a Try run on the user's behalf and wait for it to succeed prior to attempting to land to an integration branch. In the future, Autoland could potentially use logic from a source like SETA to intelligently choose tests to run to conserve resources.

A third alternative is to combine both, ensuring the user has had some Try coverage on each revision to be landed prior to having Autoland schedule a full Try run prior to landing.

Comment 1

[Dan Minor [:dminor]]

I missed the obvious, that we'll also need to check that the user is a member of the proper LDAP group to be able to land to central.

Comment 2

[Dan Minor [:dminor]]

Based on a discussion with some of the sheriff team today, we've come up with an initial plan for a policy for this, largely based on how "check-in needed" currently works:
* We'll require that either the reviewee or at least one of the reviewers has L3 access in order to be able to autoland to inbound.
* We'll enforce that at least one Try run has been made prior to landing. Interpreting the Try results will be left to humans, the tool won't enforce that the Try run be green or that minimum test coverage has been met.

Some other ideas that were discussed:
* If we can get intelligent selection of tests for a Try run working (e.g. bug 1159869) it would be nice to automatically trigger Try runs when the review is published rather than waiting for a human to do this.
* It would also be nice to not ping people for review until the Try run is complete
* Once we can ingest moz.build metadata for suggested reviewers we could gate on review from module owner or peers rather than just reviewee/reviewer L3 access.

Comment 3

[Steven MacLeod [:smacleod]]

Attachment: MozReview Request: mozreview: include approval in review request dump (Bug 1176321). r?dminor

mozreview: include approval in review request dump (Bug 1176321). r?dminor

Future commits will introduce changes to approval calculation. In order
to test these changes, and show how approval changed with them, we will
start including approval state when dumping review requests.

Comment 4

[Steven MacLeod [:smacleod]]

Attachment: MozReview Request: mozreview: add mach command for associating ldap emails (Bug 1176321). r?dminor

mozreview: add mach command for associating ldap emails (Bug 1176321). r?dminor

In order to test interactions for users with scm levels we add a command
to associate their ldap email. This allows us to test the interactions
without having the user push.

Comment 5

[Steven MacLeod [:smacleod]]

Attachment: MozReview Request: mozreview: implement autoland criteria as an approval hook (Bug 1176321). r?dminor r?gps r?mdoglio

mozreview: implement autoland criteria as an approval hook (Bug 1176321). r?dminor r?gps r?mdoglio

In order to restrict landing to reviewed requests we implement an
approval hook which verifies proper review has taken place. This will
allow checking if a review request may land by looking at the boolean
"approval" field on the review request object / resource.

Comment 6

[Dan Minor [:dminor]]

Comment on attachment 8652102 [details]
MozReview Request: mozreview: include approval in review request dump (Bug 1176321). r?dminor

https://reviewboard.mozilla.org/r/17077/#review15223

Ship It!

Comment 7

[Dan Minor [:dminor]]

Comment on attachment 8652103 [details]
MozReview Request: mozreview: add mach command for associating ldap emails (Bug 1176321). r?dminor

https://reviewboard.mozilla.org/r/17079/#review15229

::: testing/vcttesting/reviewboard/mach_commands.py:522
(Diff revision 1)
> +        from rbtools.api.errors import APIError

Unused import

::: testing/vcttesting/reviewboard/mach_commands.py:523
(Diff revision 1)
> +        root = self._get_root(username="mozreview", password="password")

Please add a comment explaining which user this is.

Comment 8

[Mauro Doglio [:mdoglio]]

Comment on attachment 8652104 [details]
MozReview Request: mozreview: implement autoland criteria as an approval hook (Bug 1176321). r?dminor r?gps r?mdoglio

https://reviewboard.mozilla.org/r/17081/#review15261

Ship It!

Comment 9

[Dan Minor [:dminor]]

Comment on attachment 8652104 [details]
MozReview Request: mozreview: implement autoland criteria as an approval hook (Bug 1176321). r?dminor r?gps r?mdoglio

https://reviewboard.mozilla.org/r/17081/#review15251

::: hgext/reviewboard/tests/test-review-request-approval.t:12
(Diff revision 1)
> +Create both an l3 and l1 user so we can't test approval in each case

should be "can test approval in each case" ?

::: hgext/reviewboard/tests/test-review-request-approval.t:17
(Diff revision 1)
> +  $ rbmanage dump-user level3 > /dev/null

Please add a comment indicating what the desired side-effect of this operation is, or add a "populate-user" mach command so that this is more readable.

::: pylib/mozreview/mozreview/hooks.py:61
(Diff revision 1)
> +            # private or something has gone seriously wrong.

if it's not private, we should log something here.

::: pylib/mozreview/mozreview/hooks.py:77
(Diff revision 1)
> +

Please add a comment explaining that we are only looking for a single valid "Ship It!" and it's up to the user to decide whether they want to wait for "Ship It!"'s from the other reviewers.

::: pylib/mozreview/mozreview/hooks.py:159
(Diff revision 1)
> +            return True

One possibility would be to return the review here instead of True, we could then use this function when autolanding to log exactly which review we based the decision to autoland on, which might be nice information to have given your timing concerns above.

Comment 10

[Gregory Szorc [:gps]]

Comment on attachment 8652104 [details]
MozReview Request: mozreview: implement autoland criteria as an approval hook (Bug 1176321). r?dminor r?gps r?mdoglio

https://reviewboard.mozilla.org/r/17081/#review15365

::: hgext/reviewboard/tests/test-review-request-approval.t:34
(Diff revision 1)
> +  $ hg --config bugzilla.username=[e-mail] push > /dev/null

This will get bitrotted by the switch to API keys.

::: pylib/mozreview/mozreview/hooks.py:14
(Diff revision 1)
>  class MozReviewApprovalHook(ReviewRequestApprovalHook):

I had to look up the purpose of ReviewRequestApprovalHook when reviewing this. Could you add a comment saying this flag is exposed on review requests via the API to determine whether a review request is fit for landing?

::: pylib/mozreview/mozreview/hooks.py:155
(Diff revision 1)
> +        if review.timestamp <= diffset_history.last_diff_updated:

This relies on system clocks, which are notoriously unreliable. I don't suppose there is an auto-incrementing primary key in the database we can use instead?

Comment 11

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/17081/#review15365

> This relies on system clocks, which are notoriously unreliable. I don't suppose there is an auto-incrementing primary key in the database we can use instead?

There is nothing of the sort we can use. This ordering is what Review Board displays in the UI (with the list of reviews and changedescriptions), so at least we'll be consistent with what is displayed.

Comment 12

[Steven MacLeod [:smacleod]]

Comment on attachment 8652103 [details]
MozReview Request: mozreview: add mach command for associating ldap emails (Bug 1176321). r?dminor

mozreview: add mach command for associating ldap emails (Bug 1176321). r?dminor

In order to test interactions for users with scm levels we add a command
to associate their ldap email. This allows us to test the interactions
without having the user push.

Comment 13

[Steven MacLeod [:smacleod]]

Comment on attachment 8652104 [details]
MozReview Request: mozreview: implement autoland criteria as an approval hook (Bug 1176321). r?dminor r?gps r?mdoglio

mozreview: implement autoland criteria as an approval hook (Bug 1176321). r?dminor r?gps r?mdoglio

In order to restrict landing to reviewed requests we implement an
approval hook which verifies proper review has taken place. This will
allow checking if a review request may land by looking at the boolean
"approval" field on the review request object / resource.

Comment 14

[Steven MacLeod [:smacleod]]

Attachment: MozReview Request: mozreview: display approval information in the commits table (Bug 1176321). r?dminor r?mdoglio

mozreview: display approval information in the commits table (Bug 1176321). r?dminor r?mdoglio

Now that we generate approval information for each review request we can
use this to show a status icon for each commit. This replaces the old
count of all ship-its on a review request which used to be displayed.

I chose to use r+ and r? to indicate approved / not approved since
Mozillians are familiar with that concept. While the semantics of
MozReview's approval don't match r+/? it should be an improvement
over the current status and we can iterate.

Comment 15

[Steven MacLeod [:smacleod]]

Comment on attachment 8653018 [details]
MozReview Request: mozreview: display approval information in the commits table (Bug 1176321). r?dminor r?mdoglio

mozreview: display approval information in the commits table (Bug 1176321). r?dminor r?mdoglio

Now that we generate approval information for each review request we can
use this to show a status icon for each commit. This replaces the old
count of all ship-its on a review request which used to be displayed.

I chose to use r+ and r? to indicate approved / not approved since
Mozillians are familiar with that concept. While the semantics of
MozReview's approval don't match r+/? it should be an improvement
over the current status and we can iterate.

Comment 16

[Dan Minor [:dminor]]

Comment on attachment 8652103 [details]
MozReview Request: mozreview: add mach command for associating ldap emails (Bug 1176321). r?dminor

https://reviewboard.mozilla.org/r/17079/#review15419

Ship It!

Comment 17

[Dan Minor [:dminor]]

Comment on attachment 8653018 [details]
MozReview Request: mozreview: display approval information in the commits table (Bug 1176321). r?dminor r?mdoglio

https://reviewboard.mozilla.org/r/17341/#review15425

Comment 18

[Dan Minor [:dminor]]

Comment on attachment 8653018 [details]
MozReview Request: mozreview: display approval information in the commits table (Bug 1176321). r?dminor r?mdoglio

https://reviewboard.mozilla.org/r/17341/#review15427

Ship It!

Comment 19

[Dan Minor [:dminor]]

Comment on attachment 8652104 [details]
MozReview Request: mozreview: implement autoland criteria as an approval hook (Bug 1176321). r?dminor r?gps r?mdoglio

https://reviewboard.mozilla.org/r/17081/#review15429

Ship It!

Comment 20

[Gregory Szorc [:gps]]

Comment on attachment 8652104 [details]
MozReview Request: mozreview: implement autoland criteria as an approval hook (Bug 1176321). r?dminor r?gps r?mdoglio

https://reviewboard.mozilla.org/r/17081/#review15431

Ship It!

Comment 21

[Mauro Doglio [:mdoglio]]

Comment on attachment 8653018 [details]
MozReview Request: mozreview: display approval information in the commits table (Bug 1176321). r?dminor r?mdoglio

https://reviewboard.mozilla.org/r/17341/#review15433

Ship It!

Comment 22

[Mauro Doglio [:mdoglio]]

Comment on attachment 8653018 [details]
MozReview Request: mozreview: display approval information in the commits table (Bug 1176321). r?dminor r?mdoglio

https://reviewboard.mozilla.org/r/17341/#review15435

Comment 23

[Mauro Doglio [:mdoglio]]

Comment on attachment 8653018 [details]
MozReview Request: mozreview: display approval information in the commits table (Bug 1176321). r?dminor r?mdoglio

https://reviewboard.mozilla.org/r/17341/#review15437

Ship It!

Comment 24

[Steven MacLeod [:smacleod]]

remote:   https://hg.mozilla.org/hgcustom/version-control-tools/rev/d47503e7db78
remote:   https://hg.mozilla.org/hgcustom/version-control-tools/rev/b01905e972c1
remote:   https://hg.mozilla.org/hgcustom/version-control-tools/rev/a4829943623c
remote:   https://hg.mozilla.org/hgcustom/version-control-tools/rev/ade76ddb26fa