Hi all, I would like to propose that we make GitHub enforce our review rule on the release branches, instead of relying on each of us to check by hand.
WHAT HAPPENED
I merged two pull requests into 8.0.x that were no longer current. I work from
the command line all the time, so I was not looking at the pull request page in
GitHub, only checking the latest status. The two cases:
- #16894 was merged on an approval given to an older commit. A newer commit had
been pushed, and the reviewer had written afterwards that it was not approved.
- #16773 was merged when its latest approval was on an earlier commit than the
one merged, and with eight review threads still open.
Nothing stopped either merge, and that is not a one-off slip. It is how the
repository is set up today.
Moving forward, and until we do what I suggest below, I will look at the pull
request status in GitHub before every merge. But that does not remove the issue
we have: we only have good will, and nothing prevents a mistake like this.
WHAT THE REPOSITORY ENFORCES TODAY
The ruleset "Default Branch Protection" in our .asf.yaml covers the default
branch and every *.x branch. It forbids deleting the branch and force-pushing
to it, and nothing else. In particular:
- A pull request does not need an approval to be merged.
- A review that requests changes does not block the merge. #16845 shows
"changes requested" and GitHub still reports it as mergeable.
- An approval stays valid after new commits are pushed.
- Open review threads do not block the merge.
- GitHub's review decision can still read APPROVED after a reviewer retracts in
a later comment, because a plain comment does not cancel an approval.
Anyone with write access can merge any pull request at any time. We have been
relying on every committer to re-read the reviews just before clicking merge,
and as the pace has picked up that has stopped being reliable.
PROPOSAL
Extend the existing ruleset in .asf.yaml with these rules:
github:
rulesets:
- name: "Default Branch Protection"
type: branch
branches:
includes:
- "~DEFAULT_BRANCH"
- "*.x"
excludes: []
bypass_teams:
- root
restrict_deletion: true
restrict_force_push: true
required_conversation_resolution: true
required_pull_request_reviews:
required_approving_review_count: 1
dismiss_stale_reviews: true
What each new line does:
- required_approving_review_count: 1 means a pull request cannot be merged
without one approval. As I understand GitHub's behaviour, with required reviews
on, a reviewer who requests changes also blocks the merge until they approve or
the review is dismissed.
- dismiss_stale_reviews: true means pushing a new commit clears earlier
approvals, so an approval always refers to the code that gets merged.
- required_conversation_resolution: true means a pull request cannot be merged
while a review thread is unresolved.
The bypass for the root team, and the existing deletion and force-push rules,
stay as they are.
COSTS I WANT TO BE UPFRONT ABOUT
- Every pull request into the default branch and the *.x branches would need an
approval. That includes the merge-up pull requests between release branches,
Dependabot updates and large stacked series. Please tell me if any of those
should be exempt.
- A committer could no longer merge their own pull request without a second
reviewer.
- When a reviewer comments after approving, the approval has to be given again.
I think that is the right behaviour, but it means a little more clicking.
- A review comment never blocks a merge today, even when it says the change is
not approved; only a "Request changes" review does. If this goes in, I would
suggest we agree to use "Request changes" whenever we mean "not ready".
THINGS I HAVE NOT VERIFIED
- I took the key names from the ASF schema (apache/infrastructure-asfyaml). Its
README documents the same keys for classic branch protection word for word, but
the ruleset section I could only read in summary. I expect Infra's bot
validates the file when it is merged, but Infra should confirm that these keys
are accepted for rulesets.
- I have not confirmed in GitHub's documentation that a review requesting
changes blocks the merge once required approvals are on. I believe it is
GitHub's standard behaviour, but please treat it as unverified.
- I do not know whether the ASF allows this project to enforce reviews this
way. I am raising it here first, as it affects every committer.
QUESTIONS
1. Are we agreed that one approval should be required before merging into 8.0.x
and the other *.x branches?
2. Should stale approvals be dismissed on push, and should open threads block
the merge?
3. Which kinds of pull request, if any, should be exempt?
4. If we agree, shall I open the .asf.yaml pull request, and who should review
it?
Thanks,
Walter
smime.p7s
Description: S/MIME cryptographic signature
