Closed Bug 1086645 Opened 6 years ago Closed 5 years ago

Auto close review requests when they are autolanded

Categories

(MozReview Graveyard :: General, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: gps, Assigned: dminor)

References

Details

Attachments

(2 files)

It would be nice if review requests were closed/submitted automatically when Mercurial publishes a changeset.

It should be possible add a "post push hook" that looks at which changesets had their phase bumped to public, correlate that with review requests, and update Review Board appropriately.

Ideally, this would run from a central server. However, the published changeset likely has a different hash than what was in Review Board. So, we'll likely need to use the client's obsolescence data to drive this.
This would be a huge UX win and would be a compelling advantage over Bugzilla. Higher-than-average priority.
Priority: -- → P2
gps, I'm interested in picking this up. I think I have a handle on most of this, but I was wondering if you could give me some hints on how to find/use the obsolescence data?
Assignee: nobody → dminor
Status: NEW → ASSIGNED
Flags: needinfo?(gps)
There's a lot of moving parts to this.

Since SHA-1s often change between the review commit and the final landing commit (adding r= to commit message, rebasing, etc), Review Board almost certainly doesn't have access to the final SHA-1 that landed nor (currently) knows how to map a final SHA-1 back to what's in Review Board.

We don't *yet* have obsolescence data stored anywhere persistent. Instead, when hg clients send reviews, they include the previous SHA-1s for the changesets that are currently getting reviewed (https://hg.mozilla.org/hgcustom/version-control-tools/file/adef76133881/hgext/reviewboard/client.py#l273).

To implement "auto close review requests," we need a few things depending on how the landing occurs.

If Autoland does the landing, it should know the review request ID and can close it. Pretty straight forward.

If a normal client does the landing, things get a bit more complicated. We need to hook into `hg push ssh://hg.mozilla.org/*` and inform MozReview which changesets were landed and (if available) the set of predecessor changesets from obsolescence data.

In *some* cases, a client will know which Review Request ID corresponds to which changesets. When the client pushes to MozReview, it stores a local file containing this metadata. However, we don't attempt to keep that metadata up to date. That feature is somewhat half baked. I have mad scientist plans for it in the future such as importing review comments into your local version control (dinosaurs who do code review in text editors want this).

Anyway, I think what we may want to do is create an API on MozReview that basically is "I just did a landing." This API would take the URL things were landed to, the set of changesets that were landed, predecessor changesets for these (available), etc and then would search MozReview for all relevant review requests and update things accordingly.

Considerations:

1) I can't recall if it is easy to access the review request SHA-1 in the current DB model. If we have to decode extra_data for each review request to find which review requests are relevant, that won't scale. This would require sending review request IDs from the client. That would be difficult because the client may not know the review request ID! Maybe we could use the extracted bug number from the pushed changesets to filter considered review requests?

2) Mercurial clients on Windows can't use the json module (yet - RyanVM hopefully has a fix for MozillaBuild soonish). This means they can't use RBTools. Requests to the Review Board API must thus be proxied through the Mercurial server (this is how sending commits to review works today). It is an ugly hack.

There's lots of little hazards involved when implementing this feature. (Which is why it hasn't been done yet.)

I dare say it, but I think working on Autoland integration in MozReview might be a better time investment. It bypasses the hard commit -> review request mapping problem almost entirely.

Something that would make the mapping problem easier is if we had the review request ID in every commit. We probably should start annotating commit messages with the MozReview review request ID or URL. We probably should have started doing this yesterday. A good place to start would be to have the Mercurial extension rewrite commit messages automatically. Then, when pushing, we can extract the review request out of the commit and target a special review request for closing.

You probably didn't think it would be this difficult, did you?
Flags: needinfo?(gps)
Depends on: 1128039
Yep, too difficult for my first MozReview bug :)

One consideration is that if we do things through Autoland, the current endpoint we hit when we land something to try could also be used for landing to an "inbound" tree and used to close the review. Fwiw, my vote would be to support this through Autoland only and avoid the complexity described above.
Assignee: dminor → nobody
Status: ASSIGNED → NEW
I, too, wish Autoland were handling all the landings :)

If we go the route of annotating commit messages with review metadata, we can also have bots close review requests automatically. This should be far more reliable than requiring people run extensions on their client. Especially with Git users and their git-remote-hg workflows.
Depends on: 1128977
We have too many P1s, so I'm spreading out the priorities.  P3 -> P4, P2 -> P3, and some portion of P1s will become P2.
Priority: P2 → P3
It's difficult to close reviews when they are published, but we can easily do this when autoland reports that the changes have landed. We should also make sure the automation menu is disabled on closed reviews.
Assignee: nobody → dminor
Status: NEW → ASSIGNED
Summary: Auto close review requests when they are published → Auto close review requests when they are autolanded
Blocks: 1220214
mozreview: Auto close review requests when they are autolanded (bug 1086645) r?mdoglio

If we're landing to an "inbound" repository we should automatically close the
review request if the landing succeeds.
Attachment #8683241 - Flags: review?(mdoglio)
mozreview: Do not allow autolanding from closed review requests (bug 1086645) r?mdoglio
Attachment #8683242 - Flags: review?(mdoglio)
Comment on attachment 8683242 [details]
MozReview Request: mozreview: Do not allow autolanding from closed review requests (bug 1086645) r=mdoglio

https://reviewboard.mozilla.org/r/24231/#review21861

::: pylib/mozreview/mozreview/tests/test-autoland-inbound.py:145
(Diff revision 1)
> +        # We should not be able to autoland from a close review request

s/close/closed
Attachment #8683242 - Flags: review?(mdoglio) → review+
Attachment #8683241 - Flags: review?(mdoglio) → review+
Comment on attachment 8683241 [details]
MozReview Request: mozreview: Auto close review requests when they are autolanded (bug 1086645) r=mdoglio

https://reviewboard.mozilla.org/r/24229/#review21859

::: pylib/mozreview/mozreview/autoland/resources.py:442
(Diff revision 1)
> +            if autoland_request[0].repository_url == landing_repo:

I would rather assert this since it would be an unexpected error worth a 500. On this endpoint with an autoland request id and a successful outcome you should always be able to close the review request.

::: pylib/mozreview/mozreview/tests/test-autoland-try.py:126
(Diff revision 1)
> +        # We should not have closed the review automatically

does this apply to autoland_try as well?
https://reviewboard.mozilla.org/r/24229/#review21859

> I would rather assert this since it would be an unexpected error worth a 500. On this endpoint with an autoland request id and a successful outcome you should always be able to close the review request.

This endpoint is used for both 'inbound' and 'try' results, so we don't always want to close things here. We have two different trigger endpoints but a shared result endpoint and a lot of code in the triggers that is almost the same, but not quite :/.
https://reviewboard.mozilla.org/r/24229/#review21889

::: pylib/mozreview/mozreview/tests/test-autoland-try.py:126
(Diff revision 1)
> +        # We should not have closed the review automatically

nevermind, I misread the comment there
Comment on attachment 8683241 [details]
MozReview Request: mozreview: Auto close review requests when they are autolanded (bug 1086645) r=mdoglio

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24229/diff/1-2/
Attachment #8683241 - Attachment description: MozReview Request: mozreview: Auto close review requests when they are autolanded (bug 1086645) r?mdoglio → MozReview Request: mozreview: Auto close review requests when they are autolanded (bug 1086645) r=mdoglio
Attachment #8683242 - Attachment description: MozReview Request: mozreview: Do not allow autolanding from closed review requests (bug 1086645) r?mdoglio → MozReview Request: mozreview: Do not allow autolanding from closed review requests (bug 1086645) r=mdoglio
Comment on attachment 8683242 [details]
MozReview Request: mozreview: Do not allow autolanding from closed review requests (bug 1086645) r=mdoglio

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24231/diff/1-2/
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Product: Developer Services → MozReview
You need to log in before you can comment on or make changes to this bug.