I have a feeling this is going to end up being a topic on our next video chat.
I do appreciate your desire to use PRs for learning. I think you could do that even if approvals aren’t required. Having PR’s sit for a month before they can be committed is way too long. At my employer PRs are rarely open more than a couple of hours. With the distributed nature of the ASF everything takes longer. Couple that with us all doing it when we have time and even getting a single approval could take days. For “normal” stuff that is just way too slow. As I said, I am definitely in favor of requiring all commits to be via PRs. I am also in favor of requiring a PR to have a successful build before it can be merged. I would also be fine with saying that every PR included in a release must be approved by someone before the release can be cut. There is no reason PRs cannot be approved even after they are merged. But that is as far as I would want to go with requirements. Ralph > On Jan 27, 2022, at 3:00 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? >>>>>>>> >>>>>>> >>>>>> >>>>> >>> >>
