Avoid need for closing and then re-opening PRs for checking a PR's CI again for an updated master branch

[steffahn]

As commented here #1881 (comment)

I'm wondering: would adding a CI trigger for auto_merge_enabled type of the pull_request event, besides the implicit default list of [opened, synchronize and reopened], avoid the need for such re-opening?

Potential downsides: Runs CI more often (presumably also doing it again on/before merges where checks had already passed).¹

Potential (additional) upside: The Dry-run check results comment gets updated another time, making the documented diffs in the comment thread more accurate.

Footnotes

1. (A possible alternative against this could be to go for auto_merge_disabled instead...) ↩

[github-actions]

Dry-run check results

[WARN  sync_team] sync-team is running in dry mode, no changes will be applied.
[INFO  sync_team] synchronizing github

[Kobzol]

What's the exact issue here? When a PR is returned from the merge queue with a failure, CI doesn't run automatically?

[steffahn]

It is when: Checks had been failing because of issues unrelated to the PR in question (in this case someone had renamed their GH account) so the PR doesn’t even make it to the merge queue in the first place.

The UX is also nicer; one can (assuming this works the way I think it should) just press “merge when ready” on a PR with Red CI (that’s red due to master branch failures you know were just resolved) and it does the reasonable thing of “it just works”, you don’t need to know the workaround of “do a rebase” (which is annoying since it rewrites all the commits without need; even though it’s the solution that seems most obvious from the Github UI here) or “close and then re-open” (which is a workaround you have to know in order to apply it; and it also looks a bit weird in the PR history IMHO and possibly confusing to people unfamiliar with the reasons why it’s done, especially given the PR CI failures aren’t really visible anymore in-thread after the fact).

And also in such a case, earlier – i.e. while the master branch hasn’t had the issue of “person was renamed” fixed yet – there’s also the problem that PRs with green CI can be added to the merge queue even though there’s no chance they’d succeed yet anyway without the problems on master being fixed, so with this they can avoid the unnecessary round-trip to the merge queue (at the cost of it taking the additional 1min of PR-CI “Test” to add a PR to a currently-empty merge queue in the good successful case).

[Kobzol]

at the cost of it taking the additional 1min of PR-CI “Test” to add a PR to a currently-empty merge queue in the good successful case

This is a bit annoying, but seems like the other benefits should outweigh the extra cost, IMO. +1

[steffahn]

When a PR is returned from the merge queue with a failure

this would also be a time point one could target with an event type (“dequeued”) however I’m failing to imagine a scenario where this would be usef…– oh well, I guess it’d be useful in order to turn CI red upon returning from the merge queue, so the UI could indicate that sending it to the queue again shouldn’t work.

Though with the auto_merge_enabled trigger like this, it should instead not make it to the queue in the first place in most cases, and when it did – and failed – then when you were to try to re-send it would re-check again then at least.

[steffahn]

seems like the other benefits should outweigh the extra cost, IMO. +1

yes, that's what I thought. And if it turns out it’s not nicer, it’s very easy to revert back to how it was before :-)

---

we should already get to experience what the successful case UX is like on this PR itself

[steffahn]

Nevermind any of these downsides. Because documentation sucks, I ran the whole setup in a test repository now, and it turns out that “auto_merge_enabled” only even fires to begin with if the checks hadn’t already been passing; i.e. if they had then “merge when ready” actually does directly add a PR to the queue, not re-running CI.

[steffahn]

This also means that running CI on de-queue sounds like a reasonable addition, I'll keep testing a bit.