I totally agree with Carter's points. Given the current state of discussion, I propose the following:
- Every changes goes into a PR - Merge requires a successful CI build - Merge requires at least one "approval" Here "approval" can be fulfilled by following means: - The committer himself (that is, in practice, no approval/review is needed) - By required reviewers + (optional) time-window (on timeout, issuer can approve himself and get it merged) Is this something we can all agree on? Gary, if I am not mistaken, all this approach asks from you in addition to what you are already doing is PR + CI checks. Is that a compromise you can make? Ralph, I have read your remark about waiting for the next online meeting, but that is on Feb 27, and I think we better shouldn't wait for a month to get this rolling. On Thu, Jan 27, 2022 at 11:03 PM Carter Kozak <cko...@ckozak.net> wrote: > Thank you for expanding upon this, Gary. The reasons I enjoy working on > open-source projects are rooted in trust and community, but lead me to a > different conclusion. Let me elaborate: > At work I interact with a set of developers who have a great deal of > experience working on our software, and over time grow to adopt similar > sets of biases. Working on open-source projects gives me the opportunity to > learn from a much broader set of strong, highly-motivated individuals with > a more diverse set of backgrounds. Asking questions on PRs is an invaluable > tool to learn things that I wouldn't otherwise discover, as well as to > ensure that my mental model of the project is based on current information. > > The eyes of the world are on log4j right now[1], and it's on us to take > ownership of security given the criticality of the project. We should > commit to radical incrementalism and experiment with different controls and > processes to move the needle on security while balancing the needs of the > project. For the first step, I propose that we enable branch protections on > release-2.x and require at least one approval before merge. We can > time-bound this to a month and then re-evaluate based on our experience. We > have the unique opportunity to lead open source efforts in supply chain > security[2][3], and we should take full advantage of it. > > Carter > > 1. > https://www.whitehouse.gov/omb/briefing-room/2022/01/26/office-of-management-and-budget-releases-federal-strategy-to-move-the-u-s-government-towards-a-zero-trust-architecture/ > 2. > https://www.whitehouse.gov/briefing-room/presidential-actions/2021/05/12/executive-order-on-improving-the-nations-cybersecurity/ > 3. > https://www.whitehouse.gov/briefing-room/statements-releases/2022/01/13/readout-of-white-house-meeting-on-software-security/ > > On Thu, Jan 27, 2022, at 14:36, Matt Sicker wrote: > > 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? > > > >>>>> > > > >>>> > > > >>> > > > >> > > > > > >
