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
