That answers part of it. Is there a deterministic flag we could use for approval?
What about followup disapproval? That is a race condition. Do we want 2 approvals? Maybe for released branches? Because they are essentially bug fixes. Thanks Walter > On Nov 10, 2026, at 1:35 AM, James Daugherty via dev <[email protected]> > wrote: > > 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 >> >>
smime.p7s
Description: S/MIME cryptographic signature
