Hi Hervé, On 30.07.2025 19:02, Hervé Boutemy wrote: > for "Require at least 1 reviewer for approval before merging", IIUC it > combines 2 steps: > - reject direct commits to maintenance branches: require use of PR + merge > - and PR requires more than self review > > I'm not absolutely against, just dislike: > 1. the overhead > 2. the bad habits this will promote > 3. it will break maven-release-plugin process that commits directly to > maintenance branch > > > explanation: > 1. overhead of for PR + merge: on simple typos, i'll have in addition to > select a PR label to NOT list the PR in the release notes > 2. bad habits: > requiring review even on simple typos, I will find a buddy to quickly pair > review everything small I do (in the same timezone as me, and we'll help each > other at reviewing quickly) > The bad habit comes here: my buddy will quickly review everything, small or > big... > 3. if someone wants to add new features and clarify new steps in the release > process
We had a very similar discussion in the Apache Logging project when we introduced RTC last year [1], which we formally adopted in April [2]. Even if I was the one to push the issue, I was really scared initially that I “killed Log4j” after realizing that many of the +1 voters weren’t available to review PRs. Even the PR that enforced RTC was strongly delayed. However, since then I’ve since grown more confident and satisfied with how it works in practice: - My “PR buddy” may not be able to approve PRs immediately, but we have regular video syncs to go through open issues and reviews. - The `2.x` branch is much cleaner, no longer cluttered with micro-commits that more often than not weren’t ported to `main`. - Copilot in the IDE is great at introducing typos. Ironically, Copilot as a reviewer catches the typos that were introduced. - The workflow encourages broader community involvement. I can more easily ask a contributor to fix a one-liner, so I can retain the ability to review their fixes. - We can enable auto-merge now. That way, once approved, we don’t have to wait for tests to finish to merge a PR manually. About the overhead for typo fixes: - I’ve seen plenty of “just a typo” commits break things. Because they’re small, people don’t always scrutinize them, but a second pair of eyes could have caught those errors. - The overhead encourages batching. Instead of opening a PR for a single typo, you can accumulate fixes in a branch and open one PR when it makes sense. - And of course, reviewing a one-liner typo is about a 10-second task. The effort is proportional to the change. Regarding release process disruptions: You're absolutely right: this is one of the areas where RTC still feels clunky. In Log4j, we create unprotected `release/x.y.z` branches. To prevent long round trips, changes there are not submitted via PR. But I strictly limit those to documentation or changelog updates. After the release, that branch is merged back into `main`, helping us catch and correct any issues that slipped in. Best regards, Piotr References: [1] https://lists.apache.org/thread/6gbos0rn3k4y3wjb1hcgnnols4ogqckl [2] https://lists.apache.org/thread/8ffdw1zqrvwrsdr0ng0m2rgtdnpzg4hk --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@maven.apache.org For additional commands, e-mail: dev-h...@maven.apache.org
