feat: cancel in-progress reviewbot pushes before new ones are started - #3590
Conversation
|
A downside of this is that the build on Phabricator would show up as pending, maybe there's some sort of "cancelled" result we could upload to Phab? |
|
Regarding Ben's question, @La0 can you think of another way to retrieve the task group? |
It would, but only on the previous Build, which probably doesn't matter...? In any case, I'll see if I can update the state in Phab, as it would be the Proper (tm) think to do. |
Currently i think it's the only way. But it would not be complicated to store the task group in the backend next to Then you could fetch that info through the revision_diffs_list endpoint |
This was originally motivated by mozilla#3578, but it's valid and worthwhile on its own. Aside from the very roundabout way we have to find task group ids, this is pretty straightforward: simply find and cancel all previous task groups for the revision. This avoids doing work for something that's already stale. If someone knows of a way to find the task group id of the reviewbot push other than the roundabout way it's happening here, I'd be very happy to switch to it. I could not find any way to pull it (or even the treeherder link) through the API directly; the only thing that seems to available there is the task id of the initial code review task. In addition to the unit tests, I managed to run code review bot locally as some sort of integration test. I didn't have it actually cancel any tasks, but I _think_ I've done enough to make this landable. Ideally we can test in a non-prod environment before production.
581e8b8 to
7d8b26d
Compare
|
I managed to get a proper test done locally. I'm pretty darn confident this will work in production. You can see it in action in this revision: https://phabricator.services.mozilla.com/D320141. The previous diff had its buildable cancelled: https://phabricator.services.mozilla.com/D320141?id=1363741, and the try push for it was also cancelled: https://treeherder.mozilla.org/jobs?repo=try&revision=27b4f18dc9a63fa448de26b353d9ac1dbfb5b28c. Latest update here contains a couple of notable changes:
As part of testing I also discovered that there is a theoretical scenario where cancellation is missed when a second update is made to the revision in short order, and the previous try push doesn't have a decision task yet. I'm choosing not to care about this, because this scenario is very unlikely once try pushes fire off of github events in the not too distant future. It would be good to fix still, but in that world it's much less critical.
I didn't implement this. I can as part of this if necessary, but I would prefer to do it in a follow-up later, if that's OK. |
| # * Find the log for each Build Target, which will contain a taskId of Code Review task | ||
| # * Pull the Code Review task log to fetch the treeherder link with the revision pushed to Try in it | ||
| # * Look up the decision task id in the task index via the revision |
There was a problem hiding this comment.
One way to at least cut this short would be to add the decision task ID to the publication task, so you could read the publication task definition instead of having to parse the logs.
There was a problem hiding this comment.
Is the publication task the one that posts the results back to phab? If so, that means we'll be getting the information too late to matter? (If it's the one that runs to make the try push, that's too early...we don't have the decision task id yet.)
marco-c
left a comment
marco-c
left a comment
There was a problem hiding this comment.
LGTM, with the caveat that this only works with Phabricator and Mercurial, and doesn't with GitHub PRs and git or Phabricator and git (which is not supported yet but will soon).
mozilla/code-review#3590 added support for cancelling previous reviewbot pushes when new ones are made. To do this, the tasks need to be able to cancel tasks from prior pushes; this requires both seal-task-group and cancel-task-group. Related to https://bugzilla.mozilla.org/show_bug.cgi?id=2065267
4 participants
This was originally motivated by #3578, but it's valid and worthwhile on its own.
Aside from the very roundabout way we have to find task group ids, this is pretty straightforward: simply find and cancel all previous task groups for the revision. This avoids doing work for something that's already stale.
If someone knows of a way to find the task group id of the reviewbot push other than the roundabout way it's happening here, I'd be very happy to switch to it. I could not find any way to pull it (or even the treeherder link) through the API directly; the only thing that seems to available there is the task id of the initial code review task.
In addition to the unit tests, I managed to run code review bot locally as some sort of integration test. I didn't have it actually cancel any tasks, but I think I've done enough to make this landable. Ideally we can test in a non-prod environment before production.