Hi Walter, Can you please prepend '[DISCUSS]' instead of proposal to future subjects? I have email filters set up for standard subject prefixes and using '[DISCUSS]' would help with visibility.
Regarding your request, I oppose this. 1. Trivial changes should be mergeable - a simple test change like this: https://github.com/apache/grails-core/pull/16903 for example or upgrading a library that we reviewed in another repo (i.e. grails publish) 2. During releases, I have had to fix the build to get the release moving. This would be impossible to do if we enable this. The most common example is a bad merge found only during release verification or when encountering a reproducibility issue. 3. If subsequent changes are made after an approval, I would consider that approval stale unless it's an obvious follow-up. I.e. Someone asked to update the docs, and no subsequent review is needed. I would support enabling the stale approval feature of GitHub so the PR approval status would reflect the most recent approval state - that wouldn't prevent us from merging, but it would probably fix the issues you've had. -James On Tue, Nov 10, 2026 at 3:54 AM Walter Duque de Estrada via dev < [email protected]> wrote: > 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 > >
