You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by David Arthur <mu...@gmail.com> on 2025/03/05 20:06:46 UTC
Protected Branches and Required Checks
We had a hiccup today where a PR was merged due to a false positive "All
checks have passed" message in the UI. This message was displayed because
the labelling workflows had run and were successful. So, really the message
was correct -- all checks that had been run were successful. The problem
was, our CI was not among the checks that had run.
This incident pointed out a deficiency in our PR workflow. Essentially, we
have to remember to set the "ci-approved" label and we need to ensure that
the CI checks are among the "passed" status checks before merging.
To remedy this, I've added a branch protection for trunk which defines a
required status check "build / CI checks completed". This check is set by a
job that runs at the end of our CI workflow. This means we cannot merge a
PR unless the CI has run.
Likely this means *all extant PRs need to merge in trunk* to run this new
"CI checks completed" job. Sorry for the noise, but I figured it was best
to rip the bandaid off now...
Thanks!
David A
P.S., I also added our release branches as protected branches, but did not
add any branch protections rules. This was done to prevent forced pushing
to these branches which we honestly should have done long ago.
--
David Arthur
Re: Protected Branches and Required Checks
Posted by David Arthur <mu...@gmail.com>.
Ok, I think everything is working as expected now.
On Friday, I worked with Infra to set up rulesets for our repo. These are
different from the branch protections that can be configured with
.asf.yaml. Rulesets are quite a bit more flexible than the legacy branch
protections. The rulesets we have are described in
https://issues.apache.org/jira/browse/INFRA-26603.
Ruleset 1 ("branches")
* Applies to trunk, "3.*", and "4.*" branches
* Disable force push
* Disable deletion
* Require linear history
This ruleset cannot be bypassed.
Ruleset 2 ("PRs")
* Applies to trunk
* Require a PR for changes
* Require at least 1 reviewer for PRs
* Only allow squash merges
* Require "build / CI checks completed" to be successful -- do not require
the branch to be up-to-date
This "PRs" ruleset can be bypassed by any committer.
---
The practical impact of these changes is that it will be obvious if a PR
has not run the full CI. Instead of the big green check mark, you'll see
something like:
[image: image.png]
*We should avoid bypassing the merge requirements whenever possible*.
Think of it as a "break glass in case of emergency" kind of action. It
should not be done without careful consideration. I understand we may have
PRs that get "stuck" behind flaky tests, but we should try rebuilding
rather than forcing it through. If we allow ourselves to skip the merge
requirements, I fear it will allow us to start ignoring flaky failures more
and more (i.e., broken window theory).
[image: image.png]
*This is what we want to see ^*
Thanks!
David A
On Thu, Mar 6, 2025 at 1:03 PM Ismael Juma <me...@ismaeljuma.com> wrote:
> Thanks David. I think you're saying that the second check only applies to
> PRs and not to direct pushes to trunk. If so, I understand now.
>
> Ismael
>
> On Thu, Mar 6, 2025 at 9:26 AM David Arthur <mu...@gmail.com> wrote:
>
> > Sorry, let me clarify that point.
> >
> > There are two proposed rulesets (full details in the INFRA ticket):
> >
> > 1) No force push on trunk or release branches
> > 2) Commits require a PR with a passing status check
> >
> > Ruleset 1 cannot be bypassed. Ruleset 2 can be bypassed by committers.
> This
> > means we should still have direct push access, but cannot force push.
> >
> > The UI will give us an explicit visual warning if we try to merge a PR
> > without passing checks.
> >
> > Does this make sense?
> >
> > -David
> >
> >
> >
> > On Thu, Mar 6, 2025 at 11:47 AM Ismael Juma <me...@ismaeljuma.com> wrote:
> >
> > > Hi David,
> > >
> > > I don't follow the following:
> > >
> > > * Require all changes to trunk to flow through a PR
> > >
> > >
> > > That's exactly the case Chia-Ping was asking about that you said is
> _not_
> > > expected.
> > >
> > > Ismael
> > >
> > >
> > > On Thu, Mar 6, 2025 at 8:01 AM David Arthur <mu...@gmail.com> wrote:
> > >
> > > > Indeed, what I was trying to achieve is not possible with the legacy
> > > > branch protections that are configured via asf.yaml. I have reverted
> > that
> > > > change.
> > > >
> > > > I filed https://issues.apache.org/jira/browse/INFRA-26603 which
> should
> > > > let us accomplish the following:
> > > >
> > > > * Protect trunk and release branches from force push and deletion
> > > > * Require all changes to trunk to flow through a PR
> > > > * Do not require that PRs are up-to-date
> > > > * Require passing status check "build / CI checks completed" for PRs
> > > > * Allow any committer to bypass the required check for a PR (for
> > > > emergencies)
> > > >
> > > > I think this strikes a good balance between safety and convenience,
> and
> > > is
> > > > a definite improvement over what we have now (which is no
> protections).
> > > >
> > > > -David
> > > >
> > > >
> > > > On Thu, Mar 6, 2025 at 9:06 AM David Arthur <mu...@gmail.com>
> wrote:
> > > >
> > > >> Hm, this wasn't my intention with this change. I was expected two
> > things
> > > >> to change:
> > > >>
> > > >> 1) PRs could only be merged if a required check passed
> > > >> 2) Forced pushes were disabled
> > > >>
> > > >> I was not expecting regular push to be disabled since there is a
> > > separate
> > > >> config for that (Screenshot from a different repo)
> > > >>
> > > >> [image: image.png]
> > > >>
> > > >>
> > > >> I'll get clarification from Infra about how this all works within
> the
> > > >> context of asf.yaml.
> > > >>
> > > >> -David
> > > >>
> > > >> On Thu, Mar 6, 2025 at 3:48 AM Ismael Juma <me...@ismaeljuma.com>
> wrote:
> > > >>
> > > >>> Yes, that's the implication of this change.
> > > >>>
> > > >>> Ismael
> > > >>>
> > > >>> On Thu, Mar 6, 2025 at 12:38 AM Chia-Ping Tsai <
> chia7712@apache.org>
> > > >>> wrote:
> > > >>>
> > > >>> > hi David
> > > >>> >
> > > >>> > It appears we can't push reverted commits to trunk from our local
> > > >>> > repository. See the following error message.
> > > >>> >
> > > >>> > remote: Resolving deltas: 100% (15/15), completed with 11 local
> > > >>> objects.
> > > >>> > remote: error: GH006: Protected branch update failed for
> > > >>> refs/heads/trunk.
> > > >>> > remote:
> > > >>> > remote: - Changes must be made through a pull request.
> > > >>> > remote:
> > > >>> > remote: - Required status check "build / CI checks completed" is
> > > >>> expected.
> > > >>> > To github.com:apache/kafka.git
> > > >>> > ...
> > > >>> >
> > > >>> > Does this mean we must revert code via pull requests in the
> future?
> > > >>> >
> > > >>> > Best,
> > > >>> > Chia-Ping
> > > >>> >
> > > >>> > On 2025/03/06 04:30:22 David Arthur wrote:
> > > >>> > > Ok looks like Infra’s manual GitHub config change got undone.
> I
> > > see
> > > >>> that
> > > >>> > > this PR https://github.com/apache/kafka/pull/19120 is
> approved,
> > > but
> > > >>> > can't
> > > >>> > > be merged :(
> > > >>> > >
> > > >>> > > I filed https://issues.apache.org/jira/browse/INFRA-26601,
> > > hopefully
> > > >>> > > someone gets to it soon.
> > > >>> > >
> > > >>> > >
> > > >>> > >
> > > >>> > >
> > > >>> > > On Wed, Mar 5, 2025 at 23:19 David Arthur <mu...@gmail.com>
> > > wrote:
> > > >>> > >
> > > >>> > > > The up-to-date requirement should not be there (for the
> reasons
> > > you
> > > >>> > > > mentioned). There was a bug with the Infra asf.yaml parser,
> so
> > > >>> Infra
> > > >>> > > > manually removed the up-to-date requirement.
> > > >>> > > >
> > > >>> > > > If the checks all pass and the PR is approved, it should be
> > > >>> mergeable
> > > >>> > > > up-to-date or not. Let me know if this isn’t the case.
> > > >>> > > >
> > > >>> > > > Prior to the PR being approved, the UI looks like it will
> force
> > > >>> you to
> > > >>> > > > merge in trunk, but it shouldn’t.
> > > >>> > > >
> > > >>> > > > David A
> > > >>> > > >
> > > >>> > > >
> > > >>> > > > On Wed, Mar 5, 2025 at 21:58 Chia-Ping Tsai <
> > chia7712@gmail.com>
> > > >>> > wrote:
> > > >>> > > >
> > > >>> > > >> hi David
> > > >>> > > >>
> > > >>> > > >> Thank you for this protection, and I fully agree that we
> need
> > to
> > > >>> avoid
> > > >>> > > >> exceptional merges as much as possible.
> > > >>> > > >>
> > > >>> > > >> For another, It seems we also require PRs to be up-to-date,
> > > which
> > > >>> is
> > > >>> > good.
> > > >>> > > >> However, the side effect is cache misses. I recall you've
> > done a
> > > >>> lot
> > > >>> > of
> > > >>> > > >> work on improving the cache, so I'm wondering if this
> > protection
> > > >>> > conflicts
> > > >>> > > >> with cache usage.
> > > >>> > > >>
> > > >>> > > >> Best,
> > > >>> > > >> Chia-Ping
> > > >>> > > >>
> > > >>> > > >> David Arthur <mu...@gmail.com> 於 2025年3月6日 週四 上午4:07寫道：
> > > >>> > > >>
> > > >>> > > >> > We had a hiccup today where a PR was merged due to a false
> > > >>> positive
> > > >>> > "All
> > > >>> > > >> > checks have passed" message in the UI. This message was
> > > >>> displayed
> > > >>> > > >> because
> > > >>> > > >> > the labelling workflows had run and were successful. So,
> > > really
> > > >>> the
> > > >>> > > >> message
> > > >>> > > >> > was correct -- all checks that had been run were
> successful.
> > > The
> > > >>> > problem
> > > >>> > > >> > was, our CI was not among the checks that had run.
> > > >>> > > >> >
> > > >>> > > >> > This incident pointed out a deficiency in our PR workflow.
> > > >>> > Essentially,
> > > >>> > > >> we
> > > >>> > > >> > have to remember to set the "ci-approved" label and we
> need
> > to
> > > >>> > ensure
> > > >>> > > >> that
> > > >>> > > >> > the CI checks are among the "passed" status checks before
> > > >>> merging.
> > > >>> > > >> >
> > > >>> > > >> > To remedy this, I've added a branch protection for trunk
> > which
> > > >>> > defines a
> > > >>> > > >> > required status check "build / CI checks completed". This
> > > check
> > > >>> is
> > > >>> > set
> > > >>> > > >> by a
> > > >>> > > >> > job that runs at the end of our CI workflow. This means we
> > > >>> cannot
> > > >>> > merge
> > > >>> > > >> a
> > > >>> > > >> > PR unless the CI has run.
> > > >>> > > >> >
> > > >>> > > >> > Likely this means *all extant PRs need to merge in trunk*
> to
> > > run
> > > >>> > this
> > > >>> > > >> new
> > > >>> > > >> > "CI checks completed" job. Sorry for the noise, but I
> > figured
> > > >>> it was
> > > >>> > > >> best
> > > >>> > > >> > to rip the bandaid off now...
> > > >>> > > >> >
> > > >>> > > >> > Thanks!
> > > >>> > > >> > David A
> > > >>> > > >> >
> > > >>> > > >> > P.S., I also added our release branches as protected
> > branches,
> > > >>> but
> > > >>> > did
> > > >>> > > >> not
> > > >>> > > >> > add any branch protections rules. This was done to prevent
> > > >>> forced
> > > >>> > > >> pushing
> > > >>> > > >> > to these branches which we honestly should have done long
> > ago.
> > > >>> > > >> >
> > > >>> > > >> >
> > > >>> > > >> >
> > > >>> > > >> >
> > > >>> > > >> >
> > > >>> > > >> > --
> > > >>> > > >> > David Arthur
> > > >>> > > >> >
> > > >>> > > >>
> > > >>> > > >
> > > >>> > >
> > > >>> >
> > > >>>
> > > >>
> > > >>
> > > >> --
> > > >> David Arthur
> > > >>
> > > >
> > > >
> > > > --
> > > > David Arthur
> > > >
> > >
> >
> >
> > --
> > David Arthur
> >
>
--
David Arthur
Re: Protected Branches and Required Checks
Posted by Ismael Juma <me...@ismaeljuma.com>.
Thanks David. I think you're saying that the second check only applies to
PRs and not to direct pushes to trunk. If so, I understand now.
Ismael
On Thu, Mar 6, 2025 at 9:26 AM David Arthur <mu...@gmail.com> wrote:
> Sorry, let me clarify that point.
>
> There are two proposed rulesets (full details in the INFRA ticket):
>
> 1) No force push on trunk or release branches
> 2) Commits require a PR with a passing status check
>
> Ruleset 1 cannot be bypassed. Ruleset 2 can be bypassed by committers. This
> means we should still have direct push access, but cannot force push.
>
> The UI will give us an explicit visual warning if we try to merge a PR
> without passing checks.
>
> Does this make sense?
>
> -David
>
>
>
> On Thu, Mar 6, 2025 at 11:47 AM Ismael Juma <me...@ismaeljuma.com> wrote:
>
> > Hi David,
> >
> > I don't follow the following:
> >
> > * Require all changes to trunk to flow through a PR
> >
> >
> > That's exactly the case Chia-Ping was asking about that you said is _not_
> > expected.
> >
> > Ismael
> >
> >
> > On Thu, Mar 6, 2025 at 8:01 AM David Arthur <mu...@gmail.com> wrote:
> >
> > > Indeed, what I was trying to achieve is not possible with the legacy
> > > branch protections that are configured via asf.yaml. I have reverted
> that
> > > change.
> > >
> > > I filed https://issues.apache.org/jira/browse/INFRA-26603 which should
> > > let us accomplish the following:
> > >
> > > * Protect trunk and release branches from force push and deletion
> > > * Require all changes to trunk to flow through a PR
> > > * Do not require that PRs are up-to-date
> > > * Require passing status check "build / CI checks completed" for PRs
> > > * Allow any committer to bypass the required check for a PR (for
> > > emergencies)
> > >
> > > I think this strikes a good balance between safety and convenience, and
> > is
> > > a definite improvement over what we have now (which is no protections).
> > >
> > > -David
> > >
> > >
> > > On Thu, Mar 6, 2025 at 9:06 AM David Arthur <mu...@gmail.com> wrote:
> > >
> > >> Hm, this wasn't my intention with this change. I was expected two
> things
> > >> to change:
> > >>
> > >> 1) PRs could only be merged if a required check passed
> > >> 2) Forced pushes were disabled
> > >>
> > >> I was not expecting regular push to be disabled since there is a
> > separate
> > >> config for that (Screenshot from a different repo)
> > >>
> > >> [image: image.png]
> > >>
> > >>
> > >> I'll get clarification from Infra about how this all works within the
> > >> context of asf.yaml.
> > >>
> > >> -David
> > >>
> > >> On Thu, Mar 6, 2025 at 3:48 AM Ismael Juma <me...@ismaeljuma.com> wrote:
> > >>
> > >>> Yes, that's the implication of this change.
> > >>>
> > >>> Ismael
> > >>>
> > >>> On Thu, Mar 6, 2025 at 12:38 AM Chia-Ping Tsai <ch...@apache.org>
> > >>> wrote:
> > >>>
> > >>> > hi David
> > >>> >
> > >>> > It appears we can't push reverted commits to trunk from our local
> > >>> > repository. See the following error message.
> > >>> >
> > >>> > remote: Resolving deltas: 100% (15/15), completed with 11 local
> > >>> objects.
> > >>> > remote: error: GH006: Protected branch update failed for
> > >>> refs/heads/trunk.
> > >>> > remote:
> > >>> > remote: - Changes must be made through a pull request.
> > >>> > remote:
> > >>> > remote: - Required status check "build / CI checks completed" is
> > >>> expected.
> > >>> > To github.com:apache/kafka.git
> > >>> > ...
> > >>> >
> > >>> > Does this mean we must revert code via pull requests in the future?
> > >>> >
> > >>> > Best,
> > >>> > Chia-Ping
> > >>> >
> > >>> > On 2025/03/06 04:30:22 David Arthur wrote:
> > >>> > > Ok looks like Infra’s manual GitHub config change got undone. I
> > see
> > >>> that
> > >>> > > this PR https://github.com/apache/kafka/pull/19120 is approved,
> > but
> > >>> > can't
> > >>> > > be merged :(
> > >>> > >
> > >>> > > I filed https://issues.apache.org/jira/browse/INFRA-26601,
> > hopefully
> > >>> > > someone gets to it soon.
> > >>> > >
> > >>> > >
> > >>> > >
> > >>> > >
> > >>> > > On Wed, Mar 5, 2025 at 23:19 David Arthur <mu...@gmail.com>
> > wrote:
> > >>> > >
> > >>> > > > The up-to-date requirement should not be there (for the reasons
> > you
> > >>> > > > mentioned). There was a bug with the Infra asf.yaml parser, so
> > >>> Infra
> > >>> > > > manually removed the up-to-date requirement.
> > >>> > > >
> > >>> > > > If the checks all pass and the PR is approved, it should be
> > >>> mergeable
> > >>> > > > up-to-date or not. Let me know if this isn’t the case.
> > >>> > > >
> > >>> > > > Prior to the PR being approved, the UI looks like it will force
> > >>> you to
> > >>> > > > merge in trunk, but it shouldn’t.
> > >>> > > >
> > >>> > > > David A
> > >>> > > >
> > >>> > > >
> > >>> > > > On Wed, Mar 5, 2025 at 21:58 Chia-Ping Tsai <
> chia7712@gmail.com>
> > >>> > wrote:
> > >>> > > >
> > >>> > > >> hi David
> > >>> > > >>
> > >>> > > >> Thank you for this protection, and I fully agree that we need
> to
> > >>> avoid
> > >>> > > >> exceptional merges as much as possible.
> > >>> > > >>
> > >>> > > >> For another, It seems we also require PRs to be up-to-date,
> > which
> > >>> is
> > >>> > good.
> > >>> > > >> However, the side effect is cache misses. I recall you've
> done a
> > >>> lot
> > >>> > of
> > >>> > > >> work on improving the cache, so I'm wondering if this
> protection
> > >>> > conflicts
> > >>> > > >> with cache usage.
> > >>> > > >>
> > >>> > > >> Best,
> > >>> > > >> Chia-Ping
> > >>> > > >>
> > >>> > > >> David Arthur <mu...@gmail.com> 於 2025年3月6日 週四 上午4:07寫道：
> > >>> > > >>
> > >>> > > >> > We had a hiccup today where a PR was merged due to a false
> > >>> positive
> > >>> > "All
> > >>> > > >> > checks have passed" message in the UI. This message was
> > >>> displayed
> > >>> > > >> because
> > >>> > > >> > the labelling workflows had run and were successful. So,
> > really
> > >>> the
> > >>> > > >> message
> > >>> > > >> > was correct -- all checks that had been run were successful.
> > The
> > >>> > problem
> > >>> > > >> > was, our CI was not among the checks that had run.
> > >>> > > >> >
> > >>> > > >> > This incident pointed out a deficiency in our PR workflow.
> > >>> > Essentially,
> > >>> > > >> we
> > >>> > > >> > have to remember to set the "ci-approved" label and we need
> to
> > >>> > ensure
> > >>> > > >> that
> > >>> > > >> > the CI checks are among the "passed" status checks before
> > >>> merging.
> > >>> > > >> >
> > >>> > > >> > To remedy this, I've added a branch protection for trunk
> which
> > >>> > defines a
> > >>> > > >> > required status check "build / CI checks completed". This
> > check
> > >>> is
> > >>> > set
> > >>> > > >> by a
> > >>> > > >> > job that runs at the end of our CI workflow. This means we
> > >>> cannot
> > >>> > merge
> > >>> > > >> a
> > >>> > > >> > PR unless the CI has run.
> > >>> > > >> >
> > >>> > > >> > Likely this means *all extant PRs need to merge in trunk* to
> > run
> > >>> > this
> > >>> > > >> new
> > >>> > > >> > "CI checks completed" job. Sorry for the noise, but I
> figured
> > >>> it was
> > >>> > > >> best
> > >>> > > >> > to rip the bandaid off now...
> > >>> > > >> >
> > >>> > > >> > Thanks!
> > >>> > > >> > David A
> > >>> > > >> >
> > >>> > > >> > P.S., I also added our release branches as protected
> branches,
> > >>> but
> > >>> > did
> > >>> > > >> not
> > >>> > > >> > add any branch protections rules. This was done to prevent
> > >>> forced
> > >>> > > >> pushing
> > >>> > > >> > to these branches which we honestly should have done long
> ago.
> > >>> > > >> >
> > >>> > > >> >
> > >>> > > >> >
> > >>> > > >> >
> > >>> > > >> >
> > >>> > > >> > --
> > >>> > > >> > David Arthur
> > >>> > > >> >
> > >>> > > >>
> > >>> > > >
> > >>> > >
> > >>> >
> > >>>
> > >>
> > >>
> > >> --
> > >> David Arthur
> > >>
> > >
> > >
> > > --
> > > David Arthur
> > >
> >
>
>
> --
> David Arthur
>
Re: Protected Branches and Required Checks
Posted by David Arthur <mu...@gmail.com>.
Sorry, let me clarify that point.
There are two proposed rulesets (full details in the INFRA ticket):
1) No force push on trunk or release branches
2) Commits require a PR with a passing status check
Ruleset 1 cannot be bypassed. Ruleset 2 can be bypassed by committers. This
means we should still have direct push access, but cannot force push.
The UI will give us an explicit visual warning if we try to merge a PR
without passing checks.
Does this make sense?
-David
On Thu, Mar 6, 2025 at 11:47 AM Ismael Juma <me...@ismaeljuma.com> wrote:
> Hi David,
>
> I don't follow the following:
>
> * Require all changes to trunk to flow through a PR
>
>
> That's exactly the case Chia-Ping was asking about that you said is _not_
> expected.
>
> Ismael
>
>
> On Thu, Mar 6, 2025 at 8:01 AM David Arthur <mu...@gmail.com> wrote:
>
> > Indeed, what I was trying to achieve is not possible with the legacy
> > branch protections that are configured via asf.yaml. I have reverted that
> > change.
> >
> > I filed https://issues.apache.org/jira/browse/INFRA-26603 which should
> > let us accomplish the following:
> >
> > * Protect trunk and release branches from force push and deletion
> > * Require all changes to trunk to flow through a PR
> > * Do not require that PRs are up-to-date
> > * Require passing status check "build / CI checks completed" for PRs
> > * Allow any committer to bypass the required check for a PR (for
> > emergencies)
> >
> > I think this strikes a good balance between safety and convenience, and
> is
> > a definite improvement over what we have now (which is no protections).
> >
> > -David
> >
> >
> > On Thu, Mar 6, 2025 at 9:06 AM David Arthur <mu...@gmail.com> wrote:
> >
> >> Hm, this wasn't my intention with this change. I was expected two things
> >> to change:
> >>
> >> 1) PRs could only be merged if a required check passed
> >> 2) Forced pushes were disabled
> >>
> >> I was not expecting regular push to be disabled since there is a
> separate
> >> config for that (Screenshot from a different repo)
> >>
> >> [image: image.png]
> >>
> >>
> >> I'll get clarification from Infra about how this all works within the
> >> context of asf.yaml.
> >>
> >> -David
> >>
> >> On Thu, Mar 6, 2025 at 3:48 AM Ismael Juma <me...@ismaeljuma.com> wrote:
> >>
> >>> Yes, that's the implication of this change.
> >>>
> >>> Ismael
> >>>
> >>> On Thu, Mar 6, 2025 at 12:38 AM Chia-Ping Tsai <ch...@apache.org>
> >>> wrote:
> >>>
> >>> > hi David
> >>> >
> >>> > It appears we can't push reverted commits to trunk from our local
> >>> > repository. See the following error message.
> >>> >
> >>> > remote: Resolving deltas: 100% (15/15), completed with 11 local
> >>> objects.
> >>> > remote: error: GH006: Protected branch update failed for
> >>> refs/heads/trunk.
> >>> > remote:
> >>> > remote: - Changes must be made through a pull request.
> >>> > remote:
> >>> > remote: - Required status check "build / CI checks completed" is
> >>> expected.
> >>> > To github.com:apache/kafka.git
> >>> > ...
> >>> >
> >>> > Does this mean we must revert code via pull requests in the future?
> >>> >
> >>> > Best,
> >>> > Chia-Ping
> >>> >
> >>> > On 2025/03/06 04:30:22 David Arthur wrote:
> >>> > > Ok looks like Infra’s manual GitHub config change got undone. I
> see
> >>> that
> >>> > > this PR https://github.com/apache/kafka/pull/19120 is approved,
> but
> >>> > can't
> >>> > > be merged :(
> >>> > >
> >>> > > I filed https://issues.apache.org/jira/browse/INFRA-26601,
> hopefully
> >>> > > someone gets to it soon.
> >>> > >
> >>> > >
> >>> > >
> >>> > >
> >>> > > On Wed, Mar 5, 2025 at 23:19 David Arthur <mu...@gmail.com>
> wrote:
> >>> > >
> >>> > > > The up-to-date requirement should not be there (for the reasons
> you
> >>> > > > mentioned). There was a bug with the Infra asf.yaml parser, so
> >>> Infra
> >>> > > > manually removed the up-to-date requirement.
> >>> > > >
> >>> > > > If the checks all pass and the PR is approved, it should be
> >>> mergeable
> >>> > > > up-to-date or not. Let me know if this isn’t the case.
> >>> > > >
> >>> > > > Prior to the PR being approved, the UI looks like it will force
> >>> you to
> >>> > > > merge in trunk, but it shouldn’t.
> >>> > > >
> >>> > > > David A
> >>> > > >
> >>> > > >
> >>> > > > On Wed, Mar 5, 2025 at 21:58 Chia-Ping Tsai <ch...@gmail.com>
> >>> > wrote:
> >>> > > >
> >>> > > >> hi David
> >>> > > >>
> >>> > > >> Thank you for this protection, and I fully agree that we need to
> >>> avoid
> >>> > > >> exceptional merges as much as possible.
> >>> > > >>
> >>> > > >> For another, It seems we also require PRs to be up-to-date,
> which
> >>> is
> >>> > good.
> >>> > > >> However, the side effect is cache misses. I recall you've done a
> >>> lot
> >>> > of
> >>> > > >> work on improving the cache, so I'm wondering if this protection
> >>> > conflicts
> >>> > > >> with cache usage.
> >>> > > >>
> >>> > > >> Best,
> >>> > > >> Chia-Ping
> >>> > > >>
> >>> > > >> David Arthur <mu...@gmail.com> 於 2025年3月6日 週四 上午4:07寫道：
> >>> > > >>
> >>> > > >> > We had a hiccup today where a PR was merged due to a false
> >>> positive
> >>> > "All
> >>> > > >> > checks have passed" message in the UI. This message was
> >>> displayed
> >>> > > >> because
> >>> > > >> > the labelling workflows had run and were successful. So,
> really
> >>> the
> >>> > > >> message
> >>> > > >> > was correct -- all checks that had been run were successful.
> The
> >>> > problem
> >>> > > >> > was, our CI was not among the checks that had run.
> >>> > > >> >
> >>> > > >> > This incident pointed out a deficiency in our PR workflow.
> >>> > Essentially,
> >>> > > >> we
> >>> > > >> > have to remember to set the "ci-approved" label and we need to
> >>> > ensure
> >>> > > >> that
> >>> > > >> > the CI checks are among the "passed" status checks before
> >>> merging.
> >>> > > >> >
> >>> > > >> > To remedy this, I've added a branch protection for trunk which
> >>> > defines a
> >>> > > >> > required status check "build / CI checks completed". This
> check
> >>> is
> >>> > set
> >>> > > >> by a
> >>> > > >> > job that runs at the end of our CI workflow. This means we
> >>> cannot
> >>> > merge
> >>> > > >> a
> >>> > > >> > PR unless the CI has run.
> >>> > > >> >
> >>> > > >> > Likely this means *all extant PRs need to merge in trunk* to
> run
> >>> > this
> >>> > > >> new
> >>> > > >> > "CI checks completed" job. Sorry for the noise, but I figured
> >>> it was
> >>> > > >> best
> >>> > > >> > to rip the bandaid off now...
> >>> > > >> >
> >>> > > >> > Thanks!
> >>> > > >> > David A
> >>> > > >> >
> >>> > > >> > P.S., I also added our release branches as protected branches,
> >>> but
> >>> > did
> >>> > > >> not
> >>> > > >> > add any branch protections rules. This was done to prevent
> >>> forced
> >>> > > >> pushing
> >>> > > >> > to these branches which we honestly should have done long ago.
> >>> > > >> >
> >>> > > >> >
> >>> > > >> >
> >>> > > >> >
> >>> > > >> >
> >>> > > >> > --
> >>> > > >> > David Arthur
> >>> > > >> >
> >>> > > >>
> >>> > > >
> >>> > >
> >>> >
> >>>
> >>
> >>
> >> --
> >> David Arthur
> >>
> >
> >
> > --
> > David Arthur
> >
>
--
David Arthur
Re: Protected Branches and Required Checks
Posted by Ismael Juma <me...@ismaeljuma.com>.
Hi David,
I don't follow the following:
* Require all changes to trunk to flow through a PR
That's exactly the case Chia-Ping was asking about that you said is _not_
expected.
Ismael
On Thu, Mar 6, 2025 at 8:01 AM David Arthur <mu...@gmail.com> wrote:
> Indeed, what I was trying to achieve is not possible with the legacy
> branch protections that are configured via asf.yaml. I have reverted that
> change.
>
> I filed https://issues.apache.org/jira/browse/INFRA-26603 which should
> let us accomplish the following:
>
> * Protect trunk and release branches from force push and deletion
> * Require all changes to trunk to flow through a PR
> * Do not require that PRs are up-to-date
> * Require passing status check "build / CI checks completed" for PRs
> * Allow any committer to bypass the required check for a PR (for
> emergencies)
>
> I think this strikes a good balance between safety and convenience, and is
> a definite improvement over what we have now (which is no protections).
>
> -David
>
>
> On Thu, Mar 6, 2025 at 9:06 AM David Arthur <mu...@gmail.com> wrote:
>
>> Hm, this wasn't my intention with this change. I was expected two things
>> to change:
>>
>> 1) PRs could only be merged if a required check passed
>> 2) Forced pushes were disabled
>>
>> I was not expecting regular push to be disabled since there is a separate
>> config for that (Screenshot from a different repo)
>>
>> [image: image.png]
>>
>>
>> I'll get clarification from Infra about how this all works within the
>> context of asf.yaml.
>>
>> -David
>>
>> On Thu, Mar 6, 2025 at 3:48 AM Ismael Juma <me...@ismaeljuma.com> wrote:
>>
>>> Yes, that's the implication of this change.
>>>
>>> Ismael
>>>
>>> On Thu, Mar 6, 2025 at 12:38 AM Chia-Ping Tsai <ch...@apache.org>
>>> wrote:
>>>
>>> > hi David
>>> >
>>> > It appears we can't push reverted commits to trunk from our local
>>> > repository. See the following error message.
>>> >
>>> > remote: Resolving deltas: 100% (15/15), completed with 11 local
>>> objects.
>>> > remote: error: GH006: Protected branch update failed for
>>> refs/heads/trunk.
>>> > remote:
>>> > remote: - Changes must be made through a pull request.
>>> > remote:
>>> > remote: - Required status check "build / CI checks completed" is
>>> expected.
>>> > To github.com:apache/kafka.git
>>> > ...
>>> >
>>> > Does this mean we must revert code via pull requests in the future?
>>> >
>>> > Best,
>>> > Chia-Ping
>>> >
>>> > On 2025/03/06 04:30:22 David Arthur wrote:
>>> > > Ok looks like Infra’s manual GitHub config change got undone. I see
>>> that
>>> > > this PR https://github.com/apache/kafka/pull/19120 is approved, but
>>> > can't
>>> > > be merged :(
>>> > >
>>> > > I filed https://issues.apache.org/jira/browse/INFRA-26601, hopefully
>>> > > someone gets to it soon.
>>> > >
>>> > >
>>> > >
>>> > >
>>> > > On Wed, Mar 5, 2025 at 23:19 David Arthur <mu...@gmail.com> wrote:
>>> > >
>>> > > > The up-to-date requirement should not be there (for the reasons you
>>> > > > mentioned). There was a bug with the Infra asf.yaml parser, so
>>> Infra
>>> > > > manually removed the up-to-date requirement.
>>> > > >
>>> > > > If the checks all pass and the PR is approved, it should be
>>> mergeable
>>> > > > up-to-date or not. Let me know if this isn’t the case.
>>> > > >
>>> > > > Prior to the PR being approved, the UI looks like it will force
>>> you to
>>> > > > merge in trunk, but it shouldn’t.
>>> > > >
>>> > > > David A
>>> > > >
>>> > > >
>>> > > > On Wed, Mar 5, 2025 at 21:58 Chia-Ping Tsai <ch...@gmail.com>
>>> > wrote:
>>> > > >
>>> > > >> hi David
>>> > > >>
>>> > > >> Thank you for this protection, and I fully agree that we need to
>>> avoid
>>> > > >> exceptional merges as much as possible.
>>> > > >>
>>> > > >> For another, It seems we also require PRs to be up-to-date, which
>>> is
>>> > good.
>>> > > >> However, the side effect is cache misses. I recall you've done a
>>> lot
>>> > of
>>> > > >> work on improving the cache, so I'm wondering if this protection
>>> > conflicts
>>> > > >> with cache usage.
>>> > > >>
>>> > > >> Best,
>>> > > >> Chia-Ping
>>> > > >>
>>> > > >> David Arthur <mu...@gmail.com> 於 2025年3月6日 週四 上午4:07寫道：
>>> > > >>
>>> > > >> > We had a hiccup today where a PR was merged due to a false
>>> positive
>>> > "All
>>> > > >> > checks have passed" message in the UI. This message was
>>> displayed
>>> > > >> because
>>> > > >> > the labelling workflows had run and were successful. So, really
>>> the
>>> > > >> message
>>> > > >> > was correct -- all checks that had been run were successful. The
>>> > problem
>>> > > >> > was, our CI was not among the checks that had run.
>>> > > >> >
>>> > > >> > This incident pointed out a deficiency in our PR workflow.
>>> > Essentially,
>>> > > >> we
>>> > > >> > have to remember to set the "ci-approved" label and we need to
>>> > ensure
>>> > > >> that
>>> > > >> > the CI checks are among the "passed" status checks before
>>> merging.
>>> > > >> >
>>> > > >> > To remedy this, I've added a branch protection for trunk which
>>> > defines a
>>> > > >> > required status check "build / CI checks completed". This check
>>> is
>>> > set
>>> > > >> by a
>>> > > >> > job that runs at the end of our CI workflow. This means we
>>> cannot
>>> > merge
>>> > > >> a
>>> > > >> > PR unless the CI has run.
>>> > > >> >
>>> > > >> > Likely this means *all extant PRs need to merge in trunk* to run
>>> > this
>>> > > >> new
>>> > > >> > "CI checks completed" job. Sorry for the noise, but I figured
>>> it was
>>> > > >> best
>>> > > >> > to rip the bandaid off now...
>>> > > >> >
>>> > > >> > Thanks!
>>> > > >> > David A
>>> > > >> >
>>> > > >> > P.S., I also added our release branches as protected branches,
>>> but
>>> > did
>>> > > >> not
>>> > > >> > add any branch protections rules. This was done to prevent
>>> forced
>>> > > >> pushing
>>> > > >> > to these branches which we honestly should have done long ago.
>>> > > >> >
>>> > > >> >
>>> > > >> >
>>> > > >> >
>>> > > >> >
>>> > > >> > --
>>> > > >> > David Arthur
>>> > > >> >
>>> > > >>
>>> > > >
>>> > >
>>> >
>>>
>>
>>
>> --
>> David Arthur
>>
>
>
> --
> David Arthur
>
Re: Protected Branches and Required Checks
Posted by David Arthur <mu...@gmail.com>.
Indeed, what I was trying to achieve is not possible with the legacy branch
protections that are configured via asf.yaml. I have reverted that change.
I filed https://issues.apache.org/jira/browse/INFRA-26603 which should let
us accomplish the following:
* Protect trunk and release branches from force push and deletion
* Require all changes to trunk to flow through a PR
* Do not require that PRs are up-to-date
* Require passing status check "build / CI checks completed" for PRs
* Allow any committer to bypass the required check for a PR (for
emergencies)
I think this strikes a good balance between safety and convenience, and is
a definite improvement over what we have now (which is no protections).
-David
On Thu, Mar 6, 2025 at 9:06 AM David Arthur <mu...@gmail.com> wrote:
> Hm, this wasn't my intention with this change. I was expected two things
> to change:
>
> 1) PRs could only be merged if a required check passed
> 2) Forced pushes were disabled
>
> I was not expecting regular push to be disabled since there is a separate
> config for that (Screenshot from a different repo)
>
> [image: image.png]
>
>
> I'll get clarification from Infra about how this all works within the
> context of asf.yaml.
>
> -David
>
> On Thu, Mar 6, 2025 at 3:48 AM Ismael Juma <me...@ismaeljuma.com> wrote:
>
>> Yes, that's the implication of this change.
>>
>> Ismael
>>
>> On Thu, Mar 6, 2025 at 12:38 AM Chia-Ping Tsai <ch...@apache.org>
>> wrote:
>>
>> > hi David
>> >
>> > It appears we can't push reverted commits to trunk from our local
>> > repository. See the following error message.
>> >
>> > remote: Resolving deltas: 100% (15/15), completed with 11 local objects.
>> > remote: error: GH006: Protected branch update failed for
>> refs/heads/trunk.
>> > remote:
>> > remote: - Changes must be made through a pull request.
>> > remote:
>> > remote: - Required status check "build / CI checks completed" is
>> expected.
>> > To github.com:apache/kafka.git
>> > ...
>> >
>> > Does this mean we must revert code via pull requests in the future?
>> >
>> > Best,
>> > Chia-Ping
>> >
>> > On 2025/03/06 04:30:22 David Arthur wrote:
>> > > Ok looks like Infra’s manual GitHub config change got undone. I see
>> that
>> > > this PR https://github.com/apache/kafka/pull/19120 is approved, but
>> > can't
>> > > be merged :(
>> > >
>> > > I filed https://issues.apache.org/jira/browse/INFRA-26601, hopefully
>> > > someone gets to it soon.
>> > >
>> > >
>> > >
>> > >
>> > > On Wed, Mar 5, 2025 at 23:19 David Arthur <mu...@gmail.com> wrote:
>> > >
>> > > > The up-to-date requirement should not be there (for the reasons you
>> > > > mentioned). There was a bug with the Infra asf.yaml parser, so Infra
>> > > > manually removed the up-to-date requirement.
>> > > >
>> > > > If the checks all pass and the PR is approved, it should be
>> mergeable
>> > > > up-to-date or not. Let me know if this isn’t the case.
>> > > >
>> > > > Prior to the PR being approved, the UI looks like it will force you
>> to
>> > > > merge in trunk, but it shouldn’t.
>> > > >
>> > > > David A
>> > > >
>> > > >
>> > > > On Wed, Mar 5, 2025 at 21:58 Chia-Ping Tsai <ch...@gmail.com>
>> > wrote:
>> > > >
>> > > >> hi David
>> > > >>
>> > > >> Thank you for this protection, and I fully agree that we need to
>> avoid
>> > > >> exceptional merges as much as possible.
>> > > >>
>> > > >> For another, It seems we also require PRs to be up-to-date, which
>> is
>> > good.
>> > > >> However, the side effect is cache misses. I recall you've done a
>> lot
>> > of
>> > > >> work on improving the cache, so I'm wondering if this protection
>> > conflicts
>> > > >> with cache usage.
>> > > >>
>> > > >> Best,
>> > > >> Chia-Ping
>> > > >>
>> > > >> David Arthur <mu...@gmail.com> 於 2025年3月6日 週四 上午4:07寫道：
>> > > >>
>> > > >> > We had a hiccup today where a PR was merged due to a false
>> positive
>> > "All
>> > > >> > checks have passed" message in the UI. This message was displayed
>> > > >> because
>> > > >> > the labelling workflows had run and were successful. So, really
>> the
>> > > >> message
>> > > >> > was correct -- all checks that had been run were successful. The
>> > problem
>> > > >> > was, our CI was not among the checks that had run.
>> > > >> >
>> > > >> > This incident pointed out a deficiency in our PR workflow.
>> > Essentially,
>> > > >> we
>> > > >> > have to remember to set the "ci-approved" label and we need to
>> > ensure
>> > > >> that
>> > > >> > the CI checks are among the "passed" status checks before
>> merging.
>> > > >> >
>> > > >> > To remedy this, I've added a branch protection for trunk which
>> > defines a
>> > > >> > required status check "build / CI checks completed". This check
>> is
>> > set
>> > > >> by a
>> > > >> > job that runs at the end of our CI workflow. This means we cannot
>> > merge
>> > > >> a
>> > > >> > PR unless the CI has run.
>> > > >> >
>> > > >> > Likely this means *all extant PRs need to merge in trunk* to run
>> > this
>> > > >> new
>> > > >> > "CI checks completed" job. Sorry for the noise, but I figured it
>> was
>> > > >> best
>> > > >> > to rip the bandaid off now...
>> > > >> >
>> > > >> > Thanks!
>> > > >> > David A
>> > > >> >
>> > > >> > P.S., I also added our release branches as protected branches,
>> but
>> > did
>> > > >> not
>> > > >> > add any branch protections rules. This was done to prevent forced
>> > > >> pushing
>> > > >> > to these branches which we honestly should have done long ago.
>> > > >> >
>> > > >> >
>> > > >> >
>> > > >> >
>> > > >> >
>> > > >> > --
>> > > >> > David Arthur
>> > > >> >
>> > > >>
>> > > >
>> > >
>> >
>>
>
>
> --
> David Arthur
>
--
David Arthur
Re: Protected Branches and Required Checks
Posted by David Arthur <mu...@gmail.com>.
Hm, this wasn't my intention with this change. I was expected two things to
change:
1) PRs could only be merged if a required check passed
2) Forced pushes were disabled
I was not expecting regular push to be disabled since there is a separate
config for that (Screenshot from a different repo)
[image: image.png]
I'll get clarification from Infra about how this all works within the
context of asf.yaml.
-David
On Thu, Mar 6, 2025 at 3:48 AM Ismael Juma <me...@ismaeljuma.com> wrote:
> Yes, that's the implication of this change.
>
> Ismael
>
> On Thu, Mar 6, 2025 at 12:38 AM Chia-Ping Tsai <ch...@apache.org>
> wrote:
>
> > hi David
> >
> > It appears we can't push reverted commits to trunk from our local
> > repository. See the following error message.
> >
> > remote: Resolving deltas: 100% (15/15), completed with 11 local objects.
> > remote: error: GH006: Protected branch update failed for
> refs/heads/trunk.
> > remote:
> > remote: - Changes must be made through a pull request.
> > remote:
> > remote: - Required status check "build / CI checks completed" is
> expected.
> > To github.com:apache/kafka.git
> > ...
> >
> > Does this mean we must revert code via pull requests in the future?
> >
> > Best,
> > Chia-Ping
> >
> > On 2025/03/06 04:30:22 David Arthur wrote:
> > > Ok looks like Infra’s manual GitHub config change got undone. I see
> that
> > > this PR https://github.com/apache/kafka/pull/19120 is approved, but
> > can't
> > > be merged :(
> > >
> > > I filed https://issues.apache.org/jira/browse/INFRA-26601, hopefully
> > > someone gets to it soon.
> > >
> > >
> > >
> > >
> > > On Wed, Mar 5, 2025 at 23:19 David Arthur <mu...@gmail.com> wrote:
> > >
> > > > The up-to-date requirement should not be there (for the reasons you
> > > > mentioned). There was a bug with the Infra asf.yaml parser, so Infra
> > > > manually removed the up-to-date requirement.
> > > >
> > > > If the checks all pass and the PR is approved, it should be mergeable
> > > > up-to-date or not. Let me know if this isn’t the case.
> > > >
> > > > Prior to the PR being approved, the UI looks like it will force you
> to
> > > > merge in trunk, but it shouldn’t.
> > > >
> > > > David A
> > > >
> > > >
> > > > On Wed, Mar 5, 2025 at 21:58 Chia-Ping Tsai <ch...@gmail.com>
> > wrote:
> > > >
> > > >> hi David
> > > >>
> > > >> Thank you for this protection, and I fully agree that we need to
> avoid
> > > >> exceptional merges as much as possible.
> > > >>
> > > >> For another, It seems we also require PRs to be up-to-date, which is
> > good.
> > > >> However, the side effect is cache misses. I recall you've done a lot
> > of
> > > >> work on improving the cache, so I'm wondering if this protection
> > conflicts
> > > >> with cache usage.
> > > >>
> > > >> Best,
> > > >> Chia-Ping
> > > >>
> > > >> David Arthur <mu...@gmail.com> 於 2025年3月6日 週四 上午4:07寫道：
> > > >>
> > > >> > We had a hiccup today where a PR was merged due to a false
> positive
> > "All
> > > >> > checks have passed" message in the UI. This message was displayed
> > > >> because
> > > >> > the labelling workflows had run and were successful. So, really
> the
> > > >> message
> > > >> > was correct -- all checks that had been run were successful. The
> > problem
> > > >> > was, our CI was not among the checks that had run.
> > > >> >
> > > >> > This incident pointed out a deficiency in our PR workflow.
> > Essentially,
> > > >> we
> > > >> > have to remember to set the "ci-approved" label and we need to
> > ensure
> > > >> that
> > > >> > the CI checks are among the "passed" status checks before merging.
> > > >> >
> > > >> > To remedy this, I've added a branch protection for trunk which
> > defines a
> > > >> > required status check "build / CI checks completed". This check is
> > set
> > > >> by a
> > > >> > job that runs at the end of our CI workflow. This means we cannot
> > merge
> > > >> a
> > > >> > PR unless the CI has run.
> > > >> >
> > > >> > Likely this means *all extant PRs need to merge in trunk* to run
> > this
> > > >> new
> > > >> > "CI checks completed" job. Sorry for the noise, but I figured it
> was
> > > >> best
> > > >> > to rip the bandaid off now...
> > > >> >
> > > >> > Thanks!
> > > >> > David A
> > > >> >
> > > >> > P.S., I also added our release branches as protected branches, but
> > did
> > > >> not
> > > >> > add any branch protections rules. This was done to prevent forced
> > > >> pushing
> > > >> > to these branches which we honestly should have done long ago.
> > > >> >
> > > >> >
> > > >> >
> > > >> >
> > > >> >
> > > >> > --
> > > >> > David Arthur
> > > >> >
> > > >>
> > > >
> > >
> >
>
--
David Arthur
Re: Protected Branches and Required Checks
Posted by Ismael Juma <me...@ismaeljuma.com>.
Yes, that's the implication of this change.
Ismael
On Thu, Mar 6, 2025 at 12:38 AM Chia-Ping Tsai <ch...@apache.org> wrote:
> hi David
>
> It appears we can't push reverted commits to trunk from our local
> repository. See the following error message.
>
> remote: Resolving deltas: 100% (15/15), completed with 11 local objects.
> remote: error: GH006: Protected branch update failed for refs/heads/trunk.
> remote:
> remote: - Changes must be made through a pull request.
> remote:
> remote: - Required status check "build / CI checks completed" is expected.
> To github.com:apache/kafka.git
> ...
>
> Does this mean we must revert code via pull requests in the future?
>
> Best,
> Chia-Ping
>
> On 2025/03/06 04:30:22 David Arthur wrote:
> > Ok looks like Infra’s manual GitHub config change got undone. I see that
> > this PR https://github.com/apache/kafka/pull/19120 is approved, but
> can't
> > be merged :(
> >
> > I filed https://issues.apache.org/jira/browse/INFRA-26601, hopefully
> > someone gets to it soon.
> >
> >
> >
> >
> > On Wed, Mar 5, 2025 at 23:19 David Arthur <mu...@gmail.com> wrote:
> >
> > > The up-to-date requirement should not be there (for the reasons you
> > > mentioned). There was a bug with the Infra asf.yaml parser, so Infra
> > > manually removed the up-to-date requirement.
> > >
> > > If the checks all pass and the PR is approved, it should be mergeable
> > > up-to-date or not. Let me know if this isn’t the case.
> > >
> > > Prior to the PR being approved, the UI looks like it will force you to
> > > merge in trunk, but it shouldn’t.
> > >
> > > David A
> > >
> > >
> > > On Wed, Mar 5, 2025 at 21:58 Chia-Ping Tsai <ch...@gmail.com>
> wrote:
> > >
> > >> hi David
> > >>
> > >> Thank you for this protection, and I fully agree that we need to avoid
> > >> exceptional merges as much as possible.
> > >>
> > >> For another, It seems we also require PRs to be up-to-date, which is
> good.
> > >> However, the side effect is cache misses. I recall you've done a lot
> of
> > >> work on improving the cache, so I'm wondering if this protection
> conflicts
> > >> with cache usage.
> > >>
> > >> Best,
> > >> Chia-Ping
> > >>
> > >> David Arthur <mu...@gmail.com> 於 2025年3月6日 週四 上午4:07寫道：
> > >>
> > >> > We had a hiccup today where a PR was merged due to a false positive
> "All
> > >> > checks have passed" message in the UI. This message was displayed
> > >> because
> > >> > the labelling workflows had run and were successful. So, really the
> > >> message
> > >> > was correct -- all checks that had been run were successful. The
> problem
> > >> > was, our CI was not among the checks that had run.
> > >> >
> > >> > This incident pointed out a deficiency in our PR workflow.
> Essentially,
> > >> we
> > >> > have to remember to set the "ci-approved" label and we need to
> ensure
> > >> that
> > >> > the CI checks are among the "passed" status checks before merging.
> > >> >
> > >> > To remedy this, I've added a branch protection for trunk which
> defines a
> > >> > required status check "build / CI checks completed". This check is
> set
> > >> by a
> > >> > job that runs at the end of our CI workflow. This means we cannot
> merge
> > >> a
> > >> > PR unless the CI has run.
> > >> >
> > >> > Likely this means *all extant PRs need to merge in trunk* to run
> this
> > >> new
> > >> > "CI checks completed" job. Sorry for the noise, but I figured it was
> > >> best
> > >> > to rip the bandaid off now...
> > >> >
> > >> > Thanks!
> > >> > David A
> > >> >
> > >> > P.S., I also added our release branches as protected branches, but
> did
> > >> not
> > >> > add any branch protections rules. This was done to prevent forced
> > >> pushing
> > >> > to these branches which we honestly should have done long ago.
> > >> >
> > >> >
> > >> >
> > >> >
> > >> >
> > >> > --
> > >> > David Arthur
> > >> >
> > >>
> > >
> >
>
Re: Protected Branches and Required Checks
Posted by Chia-Ping Tsai <ch...@apache.org>.
hi David
It appears we can't push reverted commits to trunk from our local repository. See the following error message.
remote: Resolving deltas: 100% (15/15), completed with 11 local objects.
remote: error: GH006: Protected branch update failed for refs/heads/trunk.
remote:
remote: - Changes must be made through a pull request.
remote:
remote: - Required status check "build / CI checks completed" is expected.
To github.com:apache/kafka.git
...
Does this mean we must revert code via pull requests in the future?
Best,
Chia-Ping
On 2025/03/06 04:30:22 David Arthur wrote:
> Ok looks like Infra’s manual GitHub config change got undone. I see that
> this PR https://github.com/apache/kafka/pull/19120 is approved, but can't
> be merged :(
>
> I filed https://issues.apache.org/jira/browse/INFRA-26601, hopefully
> someone gets to it soon.
>
>
>
>
> On Wed, Mar 5, 2025 at 23:19 David Arthur <mu...@gmail.com> wrote:
>
> > The up-to-date requirement should not be there (for the reasons you
> > mentioned). There was a bug with the Infra asf.yaml parser, so Infra
> > manually removed the up-to-date requirement.
> >
> > If the checks all pass and the PR is approved, it should be mergeable
> > up-to-date or not. Let me know if this isn’t the case.
> >
> > Prior to the PR being approved, the UI looks like it will force you to
> > merge in trunk, but it shouldn’t.
> >
> > David A
> >
> >
> > On Wed, Mar 5, 2025 at 21:58 Chia-Ping Tsai <ch...@gmail.com> wrote:
> >
> >> hi David
> >>
> >> Thank you for this protection, and I fully agree that we need to avoid
> >> exceptional merges as much as possible.
> >>
> >> For another, It seems we also require PRs to be up-to-date, which is good.
> >> However, the side effect is cache misses. I recall you've done a lot of
> >> work on improving the cache, so I'm wondering if this protection conflicts
> >> with cache usage.
> >>
> >> Best,
> >> Chia-Ping
> >>
> >> David Arthur <mu...@gmail.com> 於 2025年3月6日 週四 上午4:07寫道：
> >>
> >> > We had a hiccup today where a PR was merged due to a false positive "All
> >> > checks have passed" message in the UI. This message was displayed
> >> because
> >> > the labelling workflows had run and were successful. So, really the
> >> message
> >> > was correct -- all checks that had been run were successful. The problem
> >> > was, our CI was not among the checks that had run.
> >> >
> >> > This incident pointed out a deficiency in our PR workflow. Essentially,
> >> we
> >> > have to remember to set the "ci-approved" label and we need to ensure
> >> that
> >> > the CI checks are among the "passed" status checks before merging.
> >> >
> >> > To remedy this, I've added a branch protection for trunk which defines a
> >> > required status check "build / CI checks completed". This check is set
> >> by a
> >> > job that runs at the end of our CI workflow. This means we cannot merge
> >> a
> >> > PR unless the CI has run.
> >> >
> >> > Likely this means *all extant PRs need to merge in trunk* to run this
> >> new
> >> > "CI checks completed" job. Sorry for the noise, but I figured it was
> >> best
> >> > to rip the bandaid off now...
> >> >
> >> > Thanks!
> >> > David A
> >> >
> >> > P.S., I also added our release branches as protected branches, but did
> >> not
> >> > add any branch protections rules. This was done to prevent forced
> >> pushing
> >> > to these branches which we honestly should have done long ago.
> >> >
> >> >
> >> >
> >> >
> >> >
> >> > --
> >> > David Arthur
> >> >
> >>
> >
>
Re: Protected Branches and Required Checks
Posted by David Arthur <mu...@gmail.com>.
Ok looks like Infra’s manual GitHub config change got undone. I see that
this PR https://github.com/apache/kafka/pull/19120 is approved, but can't
be merged :(
I filed https://issues.apache.org/jira/browse/INFRA-26601, hopefully
someone gets to it soon.
On Wed, Mar 5, 2025 at 23:19 David Arthur <mu...@gmail.com> wrote:
> The up-to-date requirement should not be there (for the reasons you
> mentioned). There was a bug with the Infra asf.yaml parser, so Infra
> manually removed the up-to-date requirement.
>
> If the checks all pass and the PR is approved, it should be mergeable
> up-to-date or not. Let me know if this isn’t the case.
>
> Prior to the PR being approved, the UI looks like it will force you to
> merge in trunk, but it shouldn’t.
>
> David A
>
>
> On Wed, Mar 5, 2025 at 21:58 Chia-Ping Tsai <ch...@gmail.com> wrote:
>
>> hi David
>>
>> Thank you for this protection, and I fully agree that we need to avoid
>> exceptional merges as much as possible.
>>
>> For another, It seems we also require PRs to be up-to-date, which is good.
>> However, the side effect is cache misses. I recall you've done a lot of
>> work on improving the cache, so I'm wondering if this protection conflicts
>> with cache usage.
>>
>> Best,
>> Chia-Ping
>>
>> David Arthur <mu...@gmail.com> 於 2025年3月6日 週四 上午4:07寫道：
>>
>> > We had a hiccup today where a PR was merged due to a false positive "All
>> > checks have passed" message in the UI. This message was displayed
>> because
>> > the labelling workflows had run and were successful. So, really the
>> message
>> > was correct -- all checks that had been run were successful. The problem
>> > was, our CI was not among the checks that had run.
>> >
>> > This incident pointed out a deficiency in our PR workflow. Essentially,
>> we
>> > have to remember to set the "ci-approved" label and we need to ensure
>> that
>> > the CI checks are among the "passed" status checks before merging.
>> >
>> > To remedy this, I've added a branch protection for trunk which defines a
>> > required status check "build / CI checks completed". This check is set
>> by a
>> > job that runs at the end of our CI workflow. This means we cannot merge
>> a
>> > PR unless the CI has run.
>> >
>> > Likely this means *all extant PRs need to merge in trunk* to run this
>> new
>> > "CI checks completed" job. Sorry for the noise, but I figured it was
>> best
>> > to rip the bandaid off now...
>> >
>> > Thanks!
>> > David A
>> >
>> > P.S., I also added our release branches as protected branches, but did
>> not
>> > add any branch protections rules. This was done to prevent forced
>> pushing
>> > to these branches which we honestly should have done long ago.
>> >
>> >
>> >
>> >
>> >
>> > --
>> > David Arthur
>> >
>>
>
Re: Protected Branches and Required Checks
Posted by David Arthur <mu...@gmail.com>.
The up-to-date requirement should not be there (for the reasons you
mentioned). There was a bug with the Infra asf.yaml parser, so Infra
manually removed the up-to-date requirement.
If the checks all pass and the PR is approved, it should be mergeable
up-to-date or not. Let me know if this isn’t the case.
Prior to the PR being approved, the UI looks like it will force you to
merge in trunk, but it shouldn’t.
David A
On Wed, Mar 5, 2025 at 21:58 Chia-Ping Tsai <ch...@gmail.com> wrote:
> hi David
>
> Thank you for this protection, and I fully agree that we need to avoid
> exceptional merges as much as possible.
>
> For another, It seems we also require PRs to be up-to-date, which is good.
> However, the side effect is cache misses. I recall you've done a lot of
> work on improving the cache, so I'm wondering if this protection conflicts
> with cache usage.
>
> Best,
> Chia-Ping
>
> David Arthur <mu...@gmail.com> 於 2025年3月6日 週四 上午4:07寫道：
>
> > We had a hiccup today where a PR was merged due to a false positive "All
> > checks have passed" message in the UI. This message was displayed because
> > the labelling workflows had run and were successful. So, really the
> message
> > was correct -- all checks that had been run were successful. The problem
> > was, our CI was not among the checks that had run.
> >
> > This incident pointed out a deficiency in our PR workflow. Essentially,
> we
> > have to remember to set the "ci-approved" label and we need to ensure
> that
> > the CI checks are among the "passed" status checks before merging.
> >
> > To remedy this, I've added a branch protection for trunk which defines a
> > required status check "build / CI checks completed". This check is set
> by a
> > job that runs at the end of our CI workflow. This means we cannot merge a
> > PR unless the CI has run.
> >
> > Likely this means *all extant PRs need to merge in trunk* to run this new
> > "CI checks completed" job. Sorry for the noise, but I figured it was best
> > to rip the bandaid off now...
> >
> > Thanks!
> > David A
> >
> > P.S., I also added our release branches as protected branches, but did
> not
> > add any branch protections rules. This was done to prevent forced pushing
> > to these branches which we honestly should have done long ago.
> >
> >
> >
> >
> >
> > --
> > David Arthur
> >
>
Re: Protected Branches and Required Checks
Posted by Chia-Ping Tsai <ch...@gmail.com>.
hi David
Thank you for this protection, and I fully agree that we need to avoid
exceptional merges as much as possible.
For another, It seems we also require PRs to be up-to-date, which is good.
However, the side effect is cache misses. I recall you've done a lot of
work on improving the cache, so I'm wondering if this protection conflicts
with cache usage.
Best,
Chia-Ping
David Arthur <mu...@gmail.com> 於 2025年3月6日 週四 上午4:07寫道：
> We had a hiccup today where a PR was merged due to a false positive "All
> checks have passed" message in the UI. This message was displayed because
> the labelling workflows had run and were successful. So, really the message
> was correct -- all checks that had been run were successful. The problem
> was, our CI was not among the checks that had run.
>
> This incident pointed out a deficiency in our PR workflow. Essentially, we
> have to remember to set the "ci-approved" label and we need to ensure that
> the CI checks are among the "passed" status checks before merging.
>
> To remedy this, I've added a branch protection for trunk which defines a
> required status check "build / CI checks completed". This check is set by a
> job that runs at the end of our CI workflow. This means we cannot merge a
> PR unless the CI has run.
>
> Likely this means *all extant PRs need to merge in trunk* to run this new
> "CI checks completed" job. Sorry for the noise, but I figured it was best
> to rip the bandaid off now...
>
> Thanks!
> David A
>
> P.S., I also added our release branches as protected branches, but did not
> add any branch protections rules. This was done to prevent forced pushing
> to these branches which we honestly should have done long ago.
>
>
>
>
>
> --
> David Arthur
>