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 >
