Thanks for posting. I was thinking about posting something myself. With respect to number of approvals specifically, the answer is 1. This is what's been used in every project and company I've ever worked with that does code reviews. As long as there's a PR and a history, we don't gain a lot by using more than one. There can be more than one if more people want to review, but it's not required.
Most often committers merge their own PRs after approval. That's appropriate since occasionally a committer sees issues in an approved PR they want to fix before merging. Non-committer PRs must be merged by a committer. Sometimes approved PRs get left by the wayside because no one's paying attention, and that's something to fix. On Thu, Feb 6, 2025 at 7:51 PM Matthias Bünger <runningj...@web.de.invalid> wrote: > > Hi all, > I would like to bring the (as of my understanding) not yet finished > discussion about the commit/review policy with Github from a _merged_ GH > PR [1] to the mailing list, as suggest by Hervé: > > As a short summuary: > * In times of SVN there was "Commit Then Review" (CTR) policy > * Since Github, this has changed to > ** We encourage PR for better release notes and literally security > ** PMC and committers can do smaller changes , like typos, without a PR > > --- > > I personally like PR for all changes (combined with branch protection), > but I totally understand the argument that in general experienced > project members (of any OSS) are responsible enough to create PR for > "real changes", but might want to quick fix typos without one. > However we all know that PRs are not only for checking about > correctness, but also about if code is easy to understand (like > meaningful variable names) and knowlegde transfer between people. Ofc > these points to not apply on "I quickly fix a typo"-commits. > > Hervé suggested to discuss this topic and upate the current policy to > the changes brought by the switch from SVN to Github. He also suggested > to have a more relaxed one in the start and be more strict in the > future. So fow now, I'll start with the following propal: > > ---- > * All contributer and committer must always open a PR regarless of the > change they want to provide > * PMC members are allowed to do small changes, not noteworthy in release > notes (like typos) without a PR. All other changes need to be reviewed > through a PR before merge. > * There is no main branch protection yet, but might be introduced at any > point in the future. > > ---- > > I would also add the point of "Whats the numbers of approvals to merge a > PR?" to this discussion. As mentioned on Slack I'm not sure if we > need/want something like this, but I think it fits well into this discussion > > I saw PRs merged more or less instantly after one approval by a PMC, > others stay open even with multiple PMC approvals. > On release votes there is a minimum PMC quora rule to be allowed to > release, but I‘m unaware of how it‘s decided that (proposed) changes > should be implemented / PRs get accepted. > I'm also maintainer in another small open source project (JUnit Pioneer) > we decided that at least two maintainers have to approve a PR, before > merge. If the PR is opened by a maintainer, this counts as one approval. > For Maven, if we want somithing similar that could be like a) 2 > Committers + 1 PMC or b) 2 PMC have to approve. > > > Let's have a constructive discussion here on the ML. > > @Hervè I'm open to update site when we have agreed on any result :) > > Greeting > Matthias > > [1] https://github.com/apache/maven-site/pull/569 > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@maven.apache.org > For additional commands, e-mail: dev-h...@maven.apache.org > -- Elliotte Rusty Harold elh...@ibiblio.org --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@maven.apache.org For additional commands, e-mail: dev-h...@maven.apache.org
