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 >
