I think I'd lean toward a CTR by default policy where larger changes should go through a PR. Simple cleanups, refactorings, and other easy things shouldn't require more friction than the CTR plus release vote process. I'd even suggest using PRs wherever you're not already fairly comfortable in the codebase. For example, if I want to contribute changes to JsonTemplateLayout, I'd likely want to make a PR so I could get Volkan to review it. I'd also suggest using PRs wherever you desire more explicit feedback from the team.
Possibly unrelated, but I think if we fixed the triple-notification issue when commits come in so that we only see the one email, then that would make it easier to review code as it's committed. I don't think I could be convinced to fully switch to RTC, though I could see that being useful for legacy branches (e.g., backport fixes for the Java 6 and Java 7 branches, and maybe even the release-2.x branch as we get closer to 3.0). On Thu, Jan 27, 2022 at 9:49 AM Ralph Goers <ralph.go...@dslextreme.com> wrote: > > If I was asked to vote on moving to RTC I don’t know if I would be -1 but my > vote would be negative. Largely for the same reason as Gary. Yes, stuff has > been committed that I wish wasn’t but I can guarantee RTC wouldn’t have > fixed that. It is simply that I couldn’t review the commit in a timely manner. > That would happen whether it is RTC or CTR. > > OTOH. If you want to move to a policy where every commit has to be done > through a PR that doesn’t require any approvals I could probably go for that. > That would leave a record in GitHub that is easier to wade through and > eview than email is. It also isn’t much harder to do. It could also have the > advantage of running a build against it through CI tooling. I would also be > in favor of saying merges should be prevented until a successful build of > the PR is completed. I have found too many times that people are > committing stuff and not doing a full rebuild so this would ensure that > doesn’t > happen. And the extra time a CI build would take doesn’t seem like too > much of a burden to me. > > In short, the only part about this I don’t like is having to wait for a > review. > We all pretty much know the difference between something that should > be reviewed and something that doesn’t require it and follow that practice. > > Ralph > > > > > On Jan 27, 2022, at 7:16 AM, Gary Gregory <garydgreg...@gmail.com> wrote: > > > > Crater and all, > > > > Let me further elaborate that one of the attractions here is the Apache > > value of community over code, and for me, commit-then-review, > > exemplifies that. Ironically, I know people here a lot less personally and > > professionally than I do people I work with; but, I trust you, I trust > > everyone on the PMC. We can make mistakes, we fix them, we can revert > > commits, and so on, and it is through all of these activities that we build > > a community, our community. For me, review-then-commit, says "I don't trust > > you". It feels like work. At that point, we might as well start our own > > company and provide paid support and development for Log4j forks or > > whatever else we want, then we can put in all the processes anyone wants. > > > > Gary > > > > On Thu, Jan 27, 2022 at 9:02 AM Gary Gregory <garydgreg...@gmail.com> wrote: > > > >> Hi Carter, > >> > >> I think we'll have to agree to disagree, especially if you want RTC just > >> to add a single getter method, as your example shows. At work we use RTC > >> for everything, no exceptions, and that's all good and works for our team, > >> _at work_. This is not what I want to do in my free time. > >> > >> Gary > >> > >> On Wed, Jan 26, 2022 at 8:01 PM Carter Kozak <cko...@ckozak.net> wrote: > >> > >>> What if RTC only applied to the primary branch, release-2.x? We've had > >>> changes like this[1] which I discovered after the fact and wish we'd had a > >>> chance to discuss before it merged. Pushing changes prior to review is > >>> faster and easier for the committer, but ultimately creates an arduous > >>> process for reviewers. I've found it harder to have retroactive > >>> conversations about changes that have already merged. > >>> > >>> -ck > >>> > >>> 1. https://lists.apache.org/thread/qn9b22fqv4yzg6jb114ovso40fnc1k0n > >>> > >>> On Wed, Jan 26, 2022, at 16:22, Gary Gregory wrote: > >>>> Not for me, this changes CTR to RTC, which is fine for work$, but too > >>> slow > >>>> for me here. When I find time for FOSS, I just want to go and do it, not > >>>> get bogged down in process. RTC is fine for a new medium to large > >>> feature, > >>>> but not for everything. Right now, I do something in release-2.x, then > >>>> cherry-pick to master, already a PITA because of the JPMS mess, now it > >>> has > >>>> to be a 4 step process instead of 2? Pass. > >>>> > >>>> Gary > >>>> > >>>> > >>>> On Wed, Jan 26, 2022, 15:25 Volkan Yazıcı <vol...@yazi.ci> wrote: > >>>> > >>>>> According to the OSSF Scorecards <https://github.com/ossf/scorecard>, > >>> we > >>>>> are missing two check marks (LOG4J2-3260 > >>>>> <https://issues.apache.org/jira/browse/LOG4J2-3260>) there: > >>>>> > >>>>> 1. Require code review (every change goes into a PR and requires at > >>>>> least one reviewer) > >>>>> 2. Require a CI status check > >>>>> > >>>>> Even though I admit with the convenience of freedom we have right > >>> now, I > >>>>> personally find it difficult to keep track of what is going in and > >>> out. > >>>>> This convention does not aim to obstruct the development progress, but > >>>>> rather improve the overall code quality and spread the know-how in a > >>>>> scalable way. Hence, I want to implement this feature on > >>> `release-2.x` and > >>>>> `master` branches. Thoughts? > >>>>> > >>>> > >>> > >> >
