Enforcing “Give time for other maintainers to review”

[reitermarkus]

Given I already broke this rule as I didn't realize #8989 was already merged, I have some ideas on how to actually enforce this.

1. Required approvals

Require at least 1 approval for pull requests. In order not to make this block pull requests, after 24 hours, @BrewTestBot will automatically approve pull requests. Only pull requests by maintainers are auto-approved.

2. Giving other maintainers time to review

Add a workflow which only passes after a pull request has been open 24h. An approval will trigger a re-run of this workflow, i.e. this should pass after automatic approval by @BrewTestBot.

3. Allow critical fixes to be merged immediately

1. and 2. should somehow be able to bypassed for critical fixes, e.g. by applying a critical label.

4. Avoid wasting time waiting for CI

Approved pull requests are automatically merged once they pass CI. Only pull requests by maintainers are auto-merged.

[MikeMcQuaid]

Require at least 1 approval for pull requests. In order not to make this block pull requests, after 24 hours, @BrewTestBot will automatically approve pull requests. Only pull requests by maintainers are auto-approved.

Would like an exception for this for PRs that are reporting bugs. This is easyish if a bug is reported on GitHub but not so much if it's on Twitter/Stackoverflow/Discourse. Perhaps a /Fixes #\d+/ or %r{Fixes https?://} could detect this?

Similarly: reverts shouldn't require approvals if they are fixing breakage.

Add a workflow which only passes after a pull request has been open 24h. An approval will trigger a re-run of this workflow, i.e. this should pass after automatic approval by @BrewTestBot.

👍🏻

Approved pull requests are automatically merged once they pass CI. Only pull requests by maintainers are auto-merged.

I'm 👍🏻 on this provided we have the usual automerge-skip label (or similar) in order to avoid higher-risk things being deployed when a given maintainer might not be around to fix them.

[reitermarkus]

Similarly: reverts shouldn't require approvals if they are fixing breakage.

This can be handled by @BrewTestBot immediately approving if the pull request is labelled critical.

automerge-skip label (or similar)

We already have a do not merge label, this should of course also prevent auto-merging.

[Rylan12]

Love this idea and great work so far, @reitermarkus!

One suggestion:

Right now, it appears that the Triage / review workflow shows up in a failed state when the review period isn't met. This, I assume, is done so that merging isn't allowed until the 24 hour period has expired (or the critical label has been applied).

I don't this is an appropriate use of the CI indicator. To me, the CI indicator should be used to indicate whether the PR contains valid changes or whether it breaks existing functionality (plus the various other things we check). I think there's a difference between "this PR isn't ready because it doesn't work properly" and "this PR isn't ready because a maintainer hasn't looked at it yet."

I think this will be most impactful on my own PRs. At a glance, I can see the green check or red x and know who/what the PR is waiting on. If it's green, great! I just need to wait for another maintainer or the review period. If it's red, I immediately know that I need to revisit the PR to fix something. If the indicator is red even when tests pass, I need to spend some more time figuring out which workflow failed and whether or not that is something I'm responsible for handling.

To avoid the temptation to merge green PRs before the 24 hour period, we should require an approval to merge the PR (actually, it looks like we're already doing this). That way, we can have separate indicators for "ready to merge because the code doesn't break things" and "ready to merge because a maintainer has looked at this" (and we won't merge until both conditions are met).

This is the workflow used in homebrew/core currently (not sure about e.g. homebrew/cask) and I think it's very clear from looking at the PR what state it's in while still restricting merges in the appropriate way.

---

Okay, now some thoughts about the ideas above:

Require at least 1 approval for pull requests. In order not to make this block pull requests, after 24 hours, @BrewTestBot will automatically approve pull requests. Only pull requests by maintainers are auto-approved.

👍 I agree with Mike that there should be the ability to have exceptions. I think the critical label works just fine. It would be nice if it could automatically determine from a Fixes ... line that the PR was a bugfix but, to be honest, that's just me being lazy since it takes very little effort to add the critical label. I would maybe consider adding similar functionality for other labels such as bug fix, but, again, this is just extra stuff that isn't crucial

Add a workflow which only passes after a pull request has been open 24h. An approval will trigger a re-run of this workflow, i.e. this should pass after automatic approval by @BrewTestBot.

👎 for the reasons listed above (I wrote that before going through these questions thoroughly enough). To be clear. 👍 on the workflow but 👎 on having it fail.

1. and 2. should somehow be able to bypassed for critical fixes, e.g. by applying a critical label.

👍 As mentioned above, it might be nice to have some autodetection/other labels but not worth spending time on

Approved pull requests are automatically merged once they pass CI. Only pull requests by maintainers are auto-merged.

👎 Some PRs in this repo are pretty substantial and could use reviews from multiple maintainers. I don't think it makes sense to auto-merge after one approval as we do in homebrew/core because the changes made here tend to be more impactful. As a PR author, I may want to get additional reviews or give the PR more time for testing even after the changes have been approved.

Additionally, if I'm merging a more major PR, I want to make sure that I'll be awake and available if there's an issue. With PR being merged automatically, I'd likely have PRs that are auto-merged while I'm asleep when the Europe maintainers are starting to work. I may not see any notifications of failures or any issue for several more hours which makes it harder to understand what went wrong and help debug.

(sorry for the very long comment)

[iMichka]

I really like the new workflow. I agree that green checks are needed whatever happens, as it is a sign for me that the PR has passed CI and I can have a look at it.

[reitermarkus]

To avoid the temptation to merge green PRs before the 24 hour period, we should require an approval to merge the PR (actually, it looks like we're already doing this).

What if there is an approval after 1 hour already? Should the PR be mergeable then? That's the one thing which isn't quite clear to me from #8989. Is it 24h AND an approval or 24h OR an approval?

[reitermarkus]

This is the workflow used in homebrew/core currently (not sure about e.g. homebrew/cask)

I cask repos, we have require 1 approval and all approved PRs are auto-merged if they pass CI.

[MikeMcQuaid]

What if there is an approval after 1 hour already? Should the PR be mergeable then? That's the one thing which isn't quite clear to me from #8989. Is it 24h AND an approval or 24h OR an approval?

The intention was 24h unless critical. Approvals don't really come into it, as far as I'm concerned, but they could.

[reitermarkus]

The intention was 24h unless critical. Approvals don't really come into it

Then I think the current solution is the only way to actually enforce that a PR is not mergeable before 24h.

I agree that a red status check is not as inviting as a green one, but that shouldn't stop you from reviewing a PR.

[Rylan12]

Hmm, I see the problem. I guess I'm still leaning toward the 24h OR an approval but possibly just because of this issue with the CI result.

Here's an alternative idea: Add @BrewTestBot as a codeowner for all files and require a codeowner approval to merge. That way, even if a PR is approved by a maintainer, it will still need to wait the 24 hours for @BrewTestBot to give the necessary approval (this could, of course, be bypassed using the critical label to get an immediate approval).

Assuming we stick with the current workflow, I think the message should be altered. Right now it just says: Triage / review (pull_request_target) Failing after 4s — review. I think a message like Triage / review (pull_request_target) Failing after 4s — This check will succeed once the PR has been open for at least one business day. Not sure what the character limit for that is, but something that explains that there isn't anything to be done by the author who may not understand why their PR is failing.

Additionally, is approval from @BrewTestBot necessary if CI is going to fail during that time frame? Probably doesn't matter much but it feels like we have two different ways of indicating the same thing which is probably unecessary.

---

As a maintainer, I can adjust (although it will bother me). Failing CI hasn't stopped me from reviewing PRs in the past and I don't expect that will change, so I'm more worried about the status being confusing for PR authors. I'm not sure how this is implemented for non-maintainer PRs. I assume there is no auto-approval (based on discussion above). Do those PRs still need to wait 24 hours? I would think yes (aside from critical fixes). Does this work the same way minus an automatic approval?

I think some documentation could be added about this to help out maintainers who aren't following this issue. I've already seen maintainers try to remove the waiting for feedback label, I assume because they didn't know what it meant. Depending on what the experience for non-maintainers is, it may also be helpful to add some documentation to explain this workflow. I'd be willing to work on some documentation but I don't feel like I have a complete grasp on the workflow yet.

[reitermarkus]

Add @BrewTestBot as a codeowner for all files

That might work, didn't think about that.

I'm not sure how this is implemented for non-maintainer PRs.

Not at all. For these, a simple approval by a maintainer is enough.

[Rylan12]

Not at all. For these, a simple approval by a maintainer is enough.

While I agree that there shouldn't be auto-approvals for non-maintainers, shouldn't the 24-hour rule still apply? If anything, I think we'd want these PRs to have a longer review period (although I'm not suggesting we actually implement a longer one)

[MikeMcQuaid]

You don't even need to have @BrewTestBot as a codeowner as long as it's approving PRs. That means we can just enable the "only allow merge with one approval" and remove the red CI status, I think.

Not at all. For these, a simple approval by a maintainer is enough.

I agree with this. While in theory we can get wide-sweeping, controversial changes from non-maintainers in my experience it's pretty uncommon. It also "feels good" to have your contributions merged speedily (for non-maintainers), too.