Thanks for that Jarek! I find the lack of ability to merge PRs fast very limiting but it might be just something to get used to.
On Tue, Apr 29, 2025 at 6:18 AM Amogh Desai <amoghdesai....@gmail.com> wrote: > I gave it another try and I am convinced that it is helpful now. > > > (if we enable it for v3-0-test) - potentially we could disable the > "draft" option for backport PRs - even if workflows are not going to be > started those backport PRs will have this "required status check" that will > prevent accidental merges and we will have to rebase/update/close/reopen to > retrigger the workflow. > > I like this :) > > > new contributors that have PRS where workflow are waiting for approval > > And this too, I have seen some new contributor PRs go stale, this will > solve that issue. > > Thanks & Regards, > Amogh Desai > > > On Sun, Apr 27, 2025 at 5:35 PM Jarek Potiuk <ja...@potiuk.com> wrote: > > > BTW. The nice side-effect of it is that with this "required" check, we > > should not have any problems any more with merging PRs that had: > > > > * bug in workflow resulting in no workflow run at ll > > * new contributors that have PRS where workflow are waiting for approval > > * (if we enable it for v3-0-test) - potentially we could disable the > > "draft" option for backport PRs - even if workflows are not going to be > > started those backport PRs will have this "required status check" that > will > > prevent accidental merges and we will have to rebase/update/close/reopen > to > > retrigger the workflow. > > > > In all those cases the required status check will prevent the PR from > being > > merged accidentally. > > > > J. > > > > > > On Sun, Apr 27, 2025 at 4:41 AM Amogh Desai <amoghdesai....@gmail.com> > > wrote: > > > > > > 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. > > > > > > Oh ok! > > > > > > > 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 > > > > > > Haha, I see. Unfortunately that was the case with one of my PRs :) > > > Gave me the initial bad preview. > > > > > > > > > My initial experience was shadowing the benefits that this would offer > to > > > the mental > > > load of maintainers. It would be a great tool overall as Jens mentioned > > > too. > > > > > > However, I am going to try it out again and express any concerns once i > > > encounter > > > them. IAC, it's worth experimenting with, given that our recent > > experiments > > > like > > > backport tool, dosubot have all stayed :D > > > > > > > > > Thanks & Regards, > > > Amogh Desai > > > > > > > > > On Sat, Apr 26, 2025 at 9:35 PM Jens Scheffler > > <j_scheff...@gmx.de.invalid > > > > > > > wrote: > > > > > > > Hi all, > > > > > > > > thanks for tuning on auto-merge. > > > > > > > > Agree that there might be a residual risk that pipeline goes green > "too > > > > early" and approval is there... but I think we need to use carefully. > > > > > > > > I just did my first auto merge and carefully watched... all looks > good. > > > > If it helps in only 50% of cases to reduce mental load I am totally > > > > fine. Might not be perfect in all situations. But in cases where (1) > I > > > > trust the author that while I am not watching nothing surprising is > > > > committed and turning green and (2) pipeline is stable. Otherwise > there > > > > is always the "resert PR" option. > > > > > > > > Jens > > > > > > > > P.S.: Ups, found one dependency issue which I attempt to fix in > #49827 > > > > > > > > On 26.04.25 15:45, Jarek Potiuk wrote: > > > > > 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. > > > > >>> > > > > > > > > --------------------------------------------------------------------- > > > > To unsubscribe, e-mail: dev-unsubscr...@airflow.apache.org > > > > For additional commands, e-mail: dev-h...@airflow.apache.org > > > > > > > > > > > > > >
