On Sat, Apr 26, 2025 at 3:02 PM Amogh Desai <amoghdesai....@gmail.com> wrote:
> I am not sure if I like this feature a lot with my recent experience with > this PR: https://github.com/apache/airflow/pull/49692. > > On the PR status, it mentions "finalize tests" is waiting to report status > but when i describe the tests, I see that task has completed. > It is quite confusing to me with this experience at least. But, maybe I > need some getting used to. > That was a temporary issue - it is green now. I needed to figure out how to name checks properly, and it turned out that "Tests / Finalize tests / Summarize Warnings" is named (for whatever reason) as "Finalize tests / Summarize Warnings" when run as part of the composite "umbrella" workflow "Tests". Simply - the first level path is stripped in this case. And yes it took me some time because indeed - it inconsistently shows in the UI and it depends on timing: * When you add it first as required, it is displayed as "Finalize tests / Summarize Warnings" but when the job starts running this required check magically turns into "Tests / Finalize tests / Summarize Warnings" * Also this required check disappears when you remove it (and PR can be merged then) but when you add it to existing PR, it will not get green until you rerun the job (or whole PR) - so if your "Summarize Warnings" was added before I properly added the check - you will see two different checks - one completed (With Tests) and one waiting (without). . ¯\_(ツ)_/¯ So I struggled a bit with guessing how it should be done. But I figured it - out - your PR is green now, and if you see it as a required check - re-running the finalize warning or just rebasing your PR should do the job. It was only really problematic for PRs that were completing in a last hour or two where I tried to figure out what check I should put in.... And For the fun of it, I added the log of me fighting with GH naming at the end of that email: > > Speaking from a general perspective and in time sensitive situations like > cutting an RC, I am sure > we would like to have some PRs in as and when they pass CI / sometimes > don't pass (known CI issues going on). > Yeah. That's the biggest worry I have. Those are the mitigations we can apply: a) temporarily disable branch protection when we are in "crunch" situation b) push such PRs directly to gitbox - bypassing GH branch protection > This will make it slightly inconvenient and hard to keep track of whether a > PR has been merged or not, at the very least. > Yes, when we push it directly we have to make sure to do it carefully and add PR# manually > > I'm expressing this concern because I do not see any option to "override" > the enable or disable automerge feature and merge > it myself. > > Yes. There is none. > Would love it if someone could help me with that information. > > P.S: It could be that I am not able to force merge because someone else > (Jarek) enabled the force merge for my PR? > Temp timing issue > > Thanks & Regards, > Amogh Desai > ------------------------------------------------------------------ Lof of fighting with GH naming: commit 4a074c1b17726df4939fb8b426f6cb90b53f0881 Author: Jarek Potiuk <ja...@potiuk.com> Date: Sat Apr 26 15:12:36 2025 +0200 Another attempt to set the right context commit af977853743a97d5785a9b7f9ce63e7fd91cd133 Author: Jarek Potiuk <ja...@potiuk.com> Date: Sat Apr 26 14:49:51 2025 +0200 Another attempt to set the right context commit 97c5d9cf33e4c2f0654c744872ca8de452daa5ca Author: Jarek Potiuk <ja...@potiuk.com> Date: Sat Apr 26 14:40:45 2025 +0200 Another attempt to set the right context commit 14d60c7ce99f30d3477a01d56dbce15460b11f28 Author: Jarek Potiuk <ja...@potiuk.com> Date: Sat Apr 26 14:33:05 2025 +0200 Maybe this one will work? commit 156d0e8080c20b5cb61315ac6118cbce5abf75a9 Author: Jarek Potiuk <ja...@potiuk.com> Date: Sat Apr 26 14:21:51 2025 +0200 Another attempt to set the right context commit 1adf5afcc9cca4a6492359b2883b5348f60e7f9f Author: Jarek Potiuk <ja...@potiuk.com> Date: Sat Apr 26 12:56:08 2025 +0200 Proper (?) context for branch protection experiment (#49815) > On Sat, Apr 26, 2025 at 5:41 PM Jarek Potiuk <ja...@potiuk.com> wrote: > > > Hello everyone, > > > > With some initial teething problems we've enabled an "experimental" > feature > > of "auto-merging" PRs in our repo. It should potentially help with > "focus" > > of maintainers, because they will not have to come-back to the PRs to > merge > > it once they enabled auto-merge for them. > > > > It works in the way, that when "required status checks", "reviews" and > > "resolved conversations" (the required conditions for "main" protected > > branch) are not met, committer can use "Enable auto-merge" on the PR and > > when all conditions are met, the PR will get merged automatically. > > > > But after enabling it, it turns out that this has one **serious** > drawback. > > We currently cannot override the protection from GitHub UI, and it's not > > possible to merge PR that did not pass one of the checks (So "Finalize > > tests / Summarize warnings" has to complete successfully in order to be > > able to merge PR, > > > > This is quite a bit of a blocker. But not entirely, because I've learned > (I > > did not know it before) that we (committers) already have a way to bypass > > ANY protection - by directly pushing code to main branch via gitbox URL > > exposed by Apache Infrastructure: > > > > https://gitbox.apache.org/repos/asf > > > > Specifically - you can set this as remote > > https://gitbox.apache.org/repos/asf/airflow.git - and push changes > > directly > > to "main" branch. It will bypass any protection. You do not even need to > > get a review from another maintainer (yes - I just tested it and it > works). > > You just need to authenticate with your apache id / password. > > > > That is not great from a security and provenance point of view, but well, > > ASF allows it for now (which is something we will have to fix eventually > I > > think. It requires using git CLI/local client to push such branches and > > there are some small things we have to remember (like manually adding PR > # > > to the branch we are pushing or not having PR# in the merged commit at > > all). > > > > Certainly not as convenient as the merge button in PR - but workable if > we > > want to merge something quickly, regardless of the status (and apparently > > regardless of review / approval which is a bummer). > > > > I left it enabled for a moment - the weekend maximum and maybe beginning > > of Monday and would love to hear what you think. > > > > J. > > >
