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
