You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@logging.apache.org by Volkan Yazıcı <vo...@yazi.ci> on 2022/01/26 20:25:23 UTC
LOG4J2-3260 Missing branch protection settings
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?
Re: LOG4J2-3260 Missing branch protection settings
Posted by Gary Gregory <ga...@gmail.com>.
I've said my bit, I'll wait for community consensus for now. Does changing
any of this requires a VOTE and if s, under what veto rules?
Gary
On Fri, Jan 28, 2022, 10:26 Volkan Yazıcı <vo...@yazi.ci> wrote:
> 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 <ck...@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.goers@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 <ga...@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 <
> garydgregory@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 <ck...@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ı <vo...@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?
> > > > >>>>>
> > > > >>>>
> > > > >>>
> > > > >>
> > > >
> > >
> >
>
Re: LOG4J2-3260 Missing branch protection settings
Posted by Ralph Goers <ra...@dslextreme.com>.
See below
> On Jan 28, 2022, at 8:26 AM, Volkan Yazıcı <vo...@yazi.ci> wrote:
>
> I totally agree with Carter's points.
>
> Given the current state of discussion, I propose the following:
>
> - Every changes goes into a PR
+1
> - Merge requires a successful CI build
+1
> - Merge requires at least one “approval”
-1 I agree with Gary on this. I do not want to move to full RTC.
>
> Here "approval" can be fulfilled by following means:
>
> - The committer himself (that is, in practice, no approval/review is
> needed)
Say what? I wasn’t aware you could approve your own stuff. I guess that
negates the -1 but I don’t see the point. I’d want to verify that you can
approve your own stuff before doing this.
> - 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.
That is actually a PMC meeting. If we were going to discuss this on a video call
It should be open to anyone to attend.
Ralph
Re: LOG4J2-3260 Missing branch protection settings
Posted by Volkan Yazıcı <vo...@yazi.ci>.
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 <ck...@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 <ra...@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 <ga...@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 <ga...@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 <ck...@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ı <vo...@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?
> > > >>>>>
> > > >>>>
> > > >>>
> > > >>
> > >
> >
>
Re: LOG4J2-3260 Missing branch protection settings
Posted by Ralph Goers <ra...@dslextreme.com>.
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 <ck...@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 <ra...@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 <ga...@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 <ga...@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 <ck...@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ı <vo...@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?
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>
>>
Re: LOG4J2-3260 Missing branch protection settings
Posted by Carter Kozak <ck...@ckozak.net>.
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 <ra...@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 <ga...@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 <ga...@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 <ck...@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ı <vo...@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?
> > >>>>>
> > >>>>
> > >>>
> > >>
> >
>
Re: LOG4J2-3260 Missing branch protection settings
Posted by Matt Sicker <bo...@gmail.com>.
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 <ra...@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 <ga...@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 <ga...@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 <ck...@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ı <vo...@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?
> >>>>>
> >>>>
> >>>
> >>
>
Re: LOG4J2-3260 Missing branch protection settings
Posted by Ralph Goers <ra...@dslextreme.com>.
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 <ga...@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 <ga...@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 <ck...@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ı <vo...@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?
>>>>>
>>>>
>>>
>>
Re: LOG4J2-3260 Missing branch protection settings
Posted by Gary Gregory <ga...@gmail.com>.
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 <ga...@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 <ck...@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ı <vo...@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?
>> > >
>> >
>>
>
Re: LOG4J2-3260 Missing branch protection settings
Posted by Gary Gregory <ga...@gmail.com>.
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 <ck...@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ı <vo...@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?
> > >
> >
>
Re: LOG4J2-3260 Missing branch protection settings
Posted by Carter Kozak <ck...@ckozak.net>.
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ı <vo...@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?
> >
>
Re: LOG4J2-3260 Missing branch protection settings
Posted by Gary Gregory <ga...@gmail.com>.
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ı <vo...@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?
>
Re: LOG4J2-3260 Missing branch protection settings
Posted by Matt Sicker <bo...@gmail.com>.
I'd be on board with this. It might be worth having some rule that if
a committer's PR goes unreviewed for over a certain period of time,
then the committer can just merge the PR after it passes the automated
checks. I don't want this project's development to end up stalled
because everyone is either too busy or too inexperienced to review PRs
from each other, but I'd love for us to use a better UI for doing
these reviews in the first place.
On a related note, it'd be great if PRs and commits didn't generate
like 3+ duplicate emails each.
On Wed, Jan 26, 2022 at 3:40 PM Carter Kozak <ck...@ckozak.net> wrote:
>
> I'd love to start using the github PR workflow for our contributions. I've been using them for my changes for a while and find it much easier than running a full build locally to verify each change on my development system.
> While we've historically used "commit then review", I find it much easier to consolidate review and discussion on pull requests, which results in cleaner history, and clearer historical context in one place when changes are requested. The downside is that it means merging changes is blocked on other committers, so we must be cognizant of that when PRs are opened, keeping a balance of timely reviews and patience.
>
> While this isn't an issue for our own changes, we may need to figure something out to improve the experience of adding changelog entries for third-party contributions, however those are a minority and I wouldn't block on solving that problem.
>
> Carter
>
> On Wed, Jan 26, 2022, at 15:25, Volkan Yazıcı 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?
> >
>
Re: LOG4J2-3260 Missing branch protection settings
Posted by Carter Kozak <ck...@ckozak.net>.
I'd love to start using the github PR workflow for our contributions. I've been using them for my changes for a while and find it much easier than running a full build locally to verify each change on my development system.
While we've historically used "commit then review", I find it much easier to consolidate review and discussion on pull requests, which results in cleaner history, and clearer historical context in one place when changes are requested. The downside is that it means merging changes is blocked on other committers, so we must be cognizant of that when PRs are opened, keeping a balance of timely reviews and patience.
While this isn't an issue for our own changes, we may need to figure something out to improve the experience of adding changelog entries for third-party contributions, however those are a minority and I wouldn't block on solving that problem.
Carter
On Wed, Jan 26, 2022, at 15:25, Volkan Yazıcı 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?
>