I get it that this is the same as used to be with Phab, but maybe we can start being a bit tighter and use the tool’s functionality fully?
We discussed protecting the main branch for tests passing, and as soon as we deem them stable enough, which I’m in strong favour. But I think we also ought to do the same (eventually) for approval and change requests.
But for now, if we allow people (with write access) to bypass the rules, we would still retain the same behaviour, but Github shows an unmistakable warning, avoiding basic mistakes:
As long as it’s bypassable (which from your screenshot it seems it is), then having the extra barrier to avoid mistakenly merging while changes are still requested seems like a great idea to me.
As long as there is a bypass that seems fine to me (I’ve seen reviewer disappearing in the past after leaving some trivial comment marking a revision as “requiring changes”).
Yes, it is bypassable and indeed, it’s mostly to avoid pressing the big green button by accident (or prematurely).
I have merged the wrong PR before when reviewing multiple tabs and just pressing the wrong green button. With LLVM’s protection of no deletion, this creates a lot of noise.
Once we moved our projects to require approval and CI passing, I now only “force merge” when I’m pretty sure what I’m doing (like broken CI or the reviewer accepted offline and went on holiday).
The only etiquette I’d ask is to have a clear communication that this is happening, like pings and waits and other approvers acknowledging the bypass, but that’s no different than what we did in Phab, so not changing behaviour at all, just making it easier to follow the status quo.
I don’t think we have consenus on how to use the “conversation resolution” button, as @rovka raised. It also doesn’t work if the author can resolve all conversations themselves (I don’t know if there’s a setting to prevent this), since they could just mark the conversation as resolved and submit (I’ve already seen multiple occasions where people have marked a conversation as resolved, even though it wasn’t in my view).
No, direct push is a form of bypassing. If the writers have permission to bypass the rules, they can also push directly.
The only difference is that you get a “warning” on every manual push saying you’re bypassing the rules, which is fair, since you’re not getting a review not validating on the CI, so…
I would be in favor of adding these checks right now. I think ‘Require conversation resolution before merging’ and the status checks need more discussion.
@tstellar would this also enables to turn on the “auto-merge” feature? That would be fantastic
What is the downside of these? They are just printing a warning and requiring an extra click from the people who do the merging right?
One extra click when merging seems like extremely low extra burden to me.
For the conversation resolution, do we have agreement yet on who should be resolving these? I just think we should have some kind of official policy before we make it a required check. I don’t want clicking the bypass box to become routine if people are unsure what to do about the unresolved conversations.
For the status checks, do we have an agreed upon check that we want to be blocking?
Right. I think the key here is to differentiate between mistakes and intentions. Github sees it as “requirement”, but with the ability to bypass, we see it as “an extra sanity check”. To be clear, my proposal is to avoid mistakes, not to change the culture of reviews.
“Requiring” PRs, approvals, CI passing, with the explicit configuration to bypass those rules for all writers, creates an extra step to merge something that may not be ready, so you only do it when you “know what you’re doing”.
Particularly on the “require PR” option, this is only “needed” here because the “require approvals” is nested inside it. I’d be fine with just requiring approvals from the PRs that are actually submitted, so we wouldn’t get warnings on direct pushes, but the “cost” of this is even less for direct pushes: there are no extra steps, just a warning after the push has finished.
The two main “issues” I can foresee in enabling these options are:
People get annoyed of the extra click on the PR. This would only “pile up” if too many people did that, and that’d mean our reviews and CI checks are being ignored, which to me, is a bad sign and we want to know if this is the case.
Users start to rely on approvals and CI to be green and get upset if someone else merges before approval and they have no chance to review. This is the main reason why we have “require approvals”, but it’s also not bullet proof. I think here is mostly about how the community moulds its behaviour around the rules.
I think enabling these options is so low cost and so easy to revert that it’s definitely worth a shot. At the very least it will give us a feeling for how the community has adapted to Github and help us make the experience better.
