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 <m...@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 <mum...@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 <m...@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 <mum...@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 <mum...@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 <m...@ismaeljuma.com>
> wrote:
> > > >>
> > > >>> Yes, that's the implication of this change.
> > > >>>
> > > >>> Ismael
> > > >>>
> > > >>> On Thu, Mar 6, 2025 at 12:38 AM Chia-Ping Tsai <
> chia7...@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 <mum...@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 <
> > chia7...@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 <mum...@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
