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
