Re: [VOTE] Add branch protection rules to Log4j
I am finding this email confusing. I think it is due to the formatting but I am not sure. I am making my best guess. Vote 1: Enable certain branch protection features on the code branches (`2.x` and `main`) of all non-dormant Java repos: I can’t vote on this as I don’t know what the impact of that is. Vote 2. Require conversation resolution before merging. +1 - This just makes sense on a PR. Why would you merge something with open questions? Vote 3. Require linear history (Prevent merge commits from being pushed to code branches. Only "Squash" and similar allowed) I need more discussion on this. Why do I care about merge commits to resolve conflicts (which frequently aren’t conflicting with Git). Vote 4. Require status checks to pass before merging +1 Again - this is on PRs. If we have defined a workflow to automatically perform tests/tasks before allowing a merge I see no reason not to require they all succeed. Ralph > On Apr 2, 2025, at 7:18 AM, Piotr P. Karwasz > wrote: > > Hi all, > > Trying to implement the RTC rules we discussed in September[1] I encountered > some problems due to the limitations of the currently available GitHub > Settings. This is a vote to decide whether or not enable certain branch > protection features on the code branches (`2.x` and `main`) of these repos: > > * l-admin * l-jdk * l-jmx-gui > * l-log4j2 * l-log4j-jakarta * l-log4j-kotlin * l-log4j-samples* > l-log4j-scala * l-log4j-transform * l-log4j-tools * l-parent > * l-slf4j > (i.e. all the Java repos except Chainsaw, Flume and Log4j Audit). The vote > will be open for 176 hours (1 week). Everybody can vote, but only binding > votes will be counted. It will concern the following features separately: > Require a pull request before merging: > [ ] +1, enable this feature > [ ] -1, do not enable this feature > Require conversation resolution before merging: > > [ ] +1, enable this feature > [ ] -1, do not enable this feature > Require linear history (Prevent merge commits from being pushed to code > branches. Only "Squash" and similar allowed): [ ] +1, enable this feature > [ ] -1, do not enable this feature > Require status checks to pass before merging: > > [ ] +1, enable this feature > [ ] -1, do not enable this feature > > Require at least one positive review before merging: > > [ ] +1, enable this feature > [ ] -1, do not enable this feature > > Piotr > > [1] https://lists.apache.org/thread/6gbos0rn3k4y3wjb1hcgnnols4ogqckl >
Re: [VOTE] Add branch protection rules to Log4j
On Wed, Apr 2, 2025 at 7:12 PM Piotr P. Karwasz wrote: > Hi all, > Sorry, my e-mail client reformatted some lines. > So, the concerned repos are all non-dormant Java repos: l-admin, l-jdk, > l-jmx-gui, l-log4j2, l-log4j-jakarta, l-log4j-kotlin, l-log4j-samples, > l-log4j-scala, l-log4j-transform, l-log4j-tools, l-parent. > > Vote 1. Require a pull request before merging: > [ ] +1, enable this feature > [ ] -1, do not enable this feature > +1 > Vote 2. Require conversation resolution before merging: > [ ] +1, enable this feature > [ ] -1, do not enable this feature > +1 (Granted that the conversation author responds within a reasonable time frame, i.e., max. 1 week.) > Vote 3. Require linear history (Prevent merge commits from being pushed > to code branches. Only "Squash" and similar allowed): > [ ] +1, enable this feature > [ ] -1, do not enable this feature > +999 All major F/OSS projects work like this, including OpenJDK. A majority of PRs contain several noise commits; "apply review suggestions", "redo stuff", "fix typos", etc. They bring no value but pollute the history. Plus, `cherry-pick`ing and `revert`ing merge PRs are more difficult compared to squashed ones. Gentlemen, *please please please squash your commits before merging a PR!* > Vote 4. Require status checks to pass before merging: > [ ] +1, enable this feature > [ ] -1, do not enable this feature > +1 Even though it sounds nice, there were several occasions in the past where the CI is broken for all jobs, or for some particular platform, etc. > Vote 5. Require at least one positive review before merging: > [ ] +1, enable this feature > [ ] -1, do not enable this feature > +999 (Assuming you meant "Approval" with "positive review".)
Re: [VOTE] Add branch protection rules to Log4j
Hi, Vote 1. Require a pull request before merging: +1 Vote 2. Require conversation resolution before merging: +1 Vote 3. Require linear history (Prevent merge commits from being pushed to code branches. Only "Squash" and similar allowed): -0.9 I really like a linear history, but I see some problems when enforcing it always: - when using rebase (in the Github frontend) the commit signatures get lost - sometimes I create two (or more) distinct commits for one pull request (e.g. adjusting the code style in one file vs. making the actual change for the bug/feature) - I want to keep them separate in the history - when you split a file (or class) in two separate ones (and want to keep the line history for both) git checkout -b split git mv BigClass.cs SmallerClass.cs git commit -m "duplicated file“ git checkout HEAD~ BigClass.cs git checkout - git merge --no-ff split -> when you squash the feature branch into master the line history will be gone I think we should use squash or rebase whenever it is possible, but leave it to the reviewer for special circumstances. Vote 4. Require status checks to pass before merging: +1 Vote 5. Require at least one positive review before merging: +1 Regards. Jan
Re: [VOTE] Add branch protection rules to Log4j
On Fri, Apr 4, 2025 at 4:07 AM Volkan Yazıcı wrote: > > On Wed, Apr 2, 2025 at 7:12 PM Piotr P. Karwasz > wrote: > > > Hi all, > > Sorry, my e-mail client reformatted some lines. > > So, the concerned repos are all non-dormant Java repos: l-admin, l-jdk, > > l-jmx-gui, l-log4j2, l-log4j-jakarta, l-log4j-kotlin, l-log4j-samples, > > l-log4j-scala, l-log4j-transform, l-log4j-tools, l-parent. > > > > Vote 1. Require a pull request before merging: > > [ ] +1, enable this feature > > [ ] -1, do not enable this feature > > > Abstain. > > > > Vote 2. Require conversation resolution before merging: > > [ ] +1, enable this feature > > [ ] -1, do not enable this feature > > > Abstain. > > > > Vote 3. Require linear history (Prevent merge commits from being pushed > > to code branches. Only "Squash" and similar allowed): > > [ ] +1, enable this feature > > [ ] -1, do not enable this feature > > > -1 While I usually squash all PRs, sometimes a PR contains worthwhile "steps", like one commit for cleaning up something while the real fix is a focused commit. So requiring this is silly as a general rule IMO. You could argue that the cleaning up commit or the changes that are not directly related should in a different PR, but then that's even more noise. This vote items conflates two topics I think: (1) merge commits (a pain), and (2) PRs with multiple commits (can be good). > > All major F/OSS projects work like this, including OpenJDK. A majority of > PRs contain several noise commits; "apply review suggestions", "redo > stuff", "fix typos", etc. They bring no value but pollute the history. > Plus, `cherry-pick`ing and `revert`ing merge PRs are more difficult > compared to squashed ones. Gentlemen, *please please please squash your > commits before merging a PR!* > > > > Vote 4. Require status checks to pass before merging: > > [ ] +1, enable this feature > > [ ] -1, do not enable this feature > > Abstain. The "status checks" are not defined in this vote. > > +1 > > Even though it sounds nice, there were several occasions in the past where > the CI is broken for all jobs, or for some particular platform, etc. > > > > Vote 5. Require at least one positive review before merging: > > [ ] +1, enable this feature > > [ ] -1, do not enable this feature Abstain. Gary > > > > +999 (Assuming you meant "Approval" with "positive review".)
