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
> >
> >
>

Reply via email to