Maybe I need to get used to the `escape hatch` (which I don't really like tbh), but I've been having a really hard time not being able to merge things with unrelated CI failure: - Asking people to rebase again numerous time, until CI gets green, for instance https://github.com/apache/airflow/pull/50014. (flaky tests, or main broken) - CI Bug equals PR can't be merged until it's resolved and re-run again for instance https://github.com/apache/airflow/pull/49866 if you take a look at the number of rebase + close/open PR to rerun CI for a very simple change it's not great. There was a glitch in the CI for UI only PRs
I like the auto-merge feature but I think losing the ability to force merge when unrelated failure happens is too much of a downside IMO. I feel like there were many more cases where I wanted to manually merge things and couldn't compare to when I marked for auto-merge and it actually succeeded so I would say it slowed me down in the last couple of days. I also tend to be -1 on this. On Wed, Apr 30, 2025 at 3:38 PM Ash Berlin-Taylor <a...@apache.org> wrote: > > Why was a workflow change that impacts every. single. PR. not discussed in > this list before turning it on? I can’t think of a more impactful change to > committers. > > Just last month weren’t you saying we needed to have more discussion about > impactful change on this list? > > > On 30 Apr 2025, at 10:04, Jarek Potiuk <ja...@potiuk.com> wrote: > > Yeah, And I would encourage everyone to try it and provide feedback > while > > it is enabled. > > > > So far we identified a few things (and fix them) that made it borderline > > unusable (bug in workflow for UI-only changes, GitHub starting to 429-us > > with too many requests, and the mysterious "hanging" of the latest > > botocore/celery (?) "special tests" -> all that is already addressed in > > main, and those issues should not happen (hopefully), so I'd say we can > now > > "truly" see how it might work. > > > > And one comment from my side - indeed, I find it nice actually, but it's > > definitely not a "deal breaker"- so if others find it too disruptive, I > am > > definitely not going to die on this hill, I just thought it does improve > > the workflow in the way that it allows for mostly "fire and forget" when > > you approve the workflow, one thing that it definitely improves is that > you > > do not have to remember about coming back to merge a request when CI > > succeeds. > > > > J. > > > > On Wed, Apr 30, 2025 at 10:58 AM Kaxil Naik <kaxiln...@gmail.com> wrote: > > > >> Forgot to note an additional point in Summary: If we find anything > blocking > >> us in that period, we will merge > >> https://github.com/apache/airflow/pull/50009 to disable auto-merge. > >> > >> On Wed, 30 Apr 2025 at 14:26, Kaxil Naik <kaxiln...@gmail.com> wrote: > >> > >>> Jarek & I discussed it on Slack in #internal-airflow-ci-cd. Summary > >> below: > >>> I have a PR to disable it: > https://github.com/apache/airflow/pull/50009. > >>> However, given that many countries will be on holiday on May 1 due to > >>> Labour Day, and some teething issues were fixed yesterday, we will let > it > >>> run for a few more days so other committers and contributors can get a > >>> chance to try it out and share their experience after the > >> experiment/trial > >>> is concluded. > >>> On Wed, 30 Apr 2025 at 13:59, Kaxil Naik <kaxiln...@gmail.com> wrote: > >>>> Whoops yeah. > >>>>> Yep. Because it did not have all conversations resolved. We also have > >>>> "require resolved conversation" as one of the branch protection > >>>> conditions. > >>>> I resolved the conversation and it got merged automatically. > >>>> Let's adapt it when ready though, I don't see any urgency of getting > >>>> enabling auto-merge or getting it contributed immediately to asf INFRA > >> when > >>>> it isn't critical. It is about priortization > >>>> I'd say - if that is really bothering us - let's shape the feature > >> rather > >>>>> than outright reject it. > >>>> On Wed, 30 Apr 2025 at 12:37, Jarek Potiuk <ja...@potiuk.com> wrote: > >>>>> On Wed, Apr 30, 2025 at 7:55 AM Kaxil Naik <kaxiln...@gmail.com> > >> wrote: > >>>>>> To the point that the original PR is still not merged even after I > >> had > >>>>>> re-triggered the failed tests yesterday: > >>>>>> https://github.com/apache/airflow/pull/49727 > >>>>> Yep. Because it did not have all conversations resolved. We also have > >>>>> "require resolved conversation" as one of the branch protection > >>>>> conditions. > >>>>> I resolved the conversation and it got merged automatically. > >>>>>> On Wed, 30 Apr 2025 at 11:20, Kaxil Naik <kaxiln...@gmail.com> > >> wrote: > >>>>>>> The gitbox escape hatch isn't it though -- if we are to allow that > >>>>> why > >>>>>> not > >>>>>>> just allow people to merge it directly from github that to go via > >> an > >>>>>>> "escape hatch". > >>>>> Generally speaking GitHub has this option. Currently "admins" have a > >>>>> possibility of overriding branch protection (via UI). And it would be > >>>>> possible - if INFRA will allow it - to possibly add an .asf.yaml > >> feature > >>>>> to > >>>>> also allow branch protection override for all committers or a subset > of > >>>>> the > >>>>> committers (PMC Members ? ). This is more of a limitation of the > >> current > >>>>> implementation of permissions than a missing feature. If we all feel > >> that > >>>>> the gitbox escape hatch is not enough, we can likely even contribute > >>>>> such a > >>>>> feature to .asf.yaml - if INFRA will be ok with the option. It's very > >>>>> easy > >>>>> to contribute to - INFRA made it possible, we have a new framework: > >>>>> https://github.com/apache/infrastructure-asfyaml - we can even > >> implement > >>>>> "airflow-only" .asf.yaml feature, that will not be initially > available > >> to > >>>>> other ASF projects and later we can promote it to be available to > >>>>> everyone. > >>>>> I'd say - if that is really bothering us - let's shape the feature > >> rather > >>>>> than outright reject it. > >>>>>>> I am -1 on this auto-merge feature > >>>>> Understood :). But let's give it a bit more time as well and maybe > >>>>> improve > >>>>> it (see above) - unless we really feel we are blocked now - then it > >>>>> should > >>>>> be as easy as merging an .asf.yaml change to disable it. > >>>>>>> On Wed, 30 Apr 2025 at 11:18, Kaxil Naik <kaxiln...@gmail.com> > >>>>> wrote: > >>>>>>>> That’s not a single person, it impacts the committers and the PR > >>>>> author > >>>>>>>> involved too. I don’t see how team productivity soars here. > >>>>>>>> On Wed, 30 Apr 2025 at 02:39, Jarek Potiuk <ja...@potiuk.com> > >>>>> wrote: > >>>>>>>>> But yes, I also miss the previous "merge because I think it's > >> safe" > >>>>>>>>> workflow. > >>>>>>>>> I badly miss it. Personally, It hurts my productivity. > >>>>>>>>> But I think the "require status check" to be green is great for > >>>>> "team > >>>>>>>>> productivity". Usually when single person is impacted more than > >>>>> team in > >>>>>>>>> general, it's worse for the person impacted, but team > >> productivity > >>>>>> soars. > >>>>>>>>> J. > >>>>>>>>> On Tue, Apr 29, 2025 at 11:03 PM Jarek Potiuk <ja...@potiuk.com> > >>>>>> wrote: > >>>>>>>>>> Just to add comment: > >>>>>>>>>> a) there was some instability of "celery/boto" hanging tests > >>>>> today > >>>>>>>>> that is > >>>>>>>>>> rather difficult to address - but we worked around it by just > >>>>>> removing > >>>>>>>>>> "special-tests" from pre-requisite of merging > >>>>>>>>>> b) GitHub today (like literally today!) started to be picky on > >>>>> "too > >>>>>>>>> many > >>>>>>>>>> requests" - I addressed it today for both helm chart and our > >>>>> release > >>>>>>>>> tests > >>>>>>>>>> (we are using bearer-token to authenticate and increase the > >>>>> limit - > >>>>>>>>> and we > >>>>>>>>>> added cache on downloading json schema that was downloaded > >>>>> "per-test" > >>>>>>>>>> c) in cases like the one mentioned above with intermittent > >>>>> failures - > >>>>>>>>>> simple "rerun failed jobs" (assuming it will succeed after > >>>>> rerun) - > >>>>>> is > >>>>>>>>>> essentially equivalent of "merge" (unless it fails again which > >>>>> for me > >>>>>>>>> is a > >>>>>>>>>> signal of "DO NOT MERGE") > >>>>>>>>>> d) we always have the "gitbox" escape hatch - that allows any > >>>>>>>>> committer to > >>>>>>>>>> push the fix directly, bypassing the limits: > >>>>>>>>>> This is a simple thing for committers: > >>>>>>>>>> git add remote gitbox > >>>>>> https://gitbox.apache.org/repos/asf/airflow.git > >>>>>>>>>> git fetch gitbox > >>>>>>>>>> git commit --amend ("add #PR number") > >>>>>>>>>> git push gitbox BRANCH_NAME:main (you need to provide your > >>>>> apache id > >>>>>>>>> and > >>>>>>>>>> password) > >>>>>>>>>> This is a nice escape hatch that we can use as "exceptional > >>>>>> workflow" - > >>>>>>>>>> and it works - I did it quite a few times over the last few > >>>>> days. Not > >>>>>>>>> UI > >>>>>>>>>> controlled, but IMHO exceptional workflow should be - well - > >>>>>>>>> exceptional. > >>>>>>>>>> J. > >>>>>>>>>> On Tue, Apr 29, 2025 at 8:52 PM Kaxil Naik < > >> kaxiln...@gmail.com> > >>>>>>>>> wrote: > >>>>>>>>>>> Similar experience as Elad, I am in favor of disabling it tbh. > >>>>> For > >>>>>>>>>>> example, > >>>>>>>>>>> https://github.com/apache/airflow/pull/49727 has a failing > >>>>> test as > >>>>>>>>> below > >>>>>>>>>>> -- > >>>>>>>>>>> which is not an issue, and test passes locally so I would want > >>>>> to > >>>>>>>>> merge it > >>>>>>>>>>> but I can't. > >>>>>>>>>>> FAILED > >> > helm-tests/tests/helm_tests/airflow_aux/test_basic_helm_chart.py::TestBaseChartTest::test_priority_classes > >>>>>>>>>>> - requests.exceptions.HTTPError: 429 Client Error: Too Many > >>>>> Requests > >>>>>>>>> for > >>>>>>>>>>> url: > >> > https://raw.githubusercontent.com/yannh/kubernetes-json-schema/master/v1.29.1-standalone-strict/priorityclass-scheduling-v1.json > >>>>>>>>>>> On Tue, 29 Apr 2025 at 18:29, Jarek Potiuk <ja...@potiuk.com> > >>>>>> wrote: > >>>>>>>>>>>> On Tue, Apr 29, 2025 at 1:46 PM Elad Kalif < > >>>>> elad...@apache.org> > >>>>>>>>> wrote: > >>>>>>>>>>>>> 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. > >>>>>>>>>>>> Indeed. I also see it, but also I got a few manually pushed > >>>>> "must > >>>>>>>>> fix > >>>>>>>>>>>> quickly" to gitbox, and actually I find it really nice - > >>>>> because > >>>>>> it > >>>>>>>>> is > >>>>>>>>>>>> still possible, but it require some extra effort and > >>>>> deliberate > >>>>>> "ok > >>>>>>>>> that > >>>>>>>>>>>> one really should be pushed to unblock everyone" - as long > >> as > >>>>> we > >>>>>> all > >>>>>>>>>>>> (especially those people that are active in the > >>>>>>>>>>>> #internal-airfow-ci-cd channel) know how to do it and can > >> fix > >>>>>> things > >>>>>>>>>>>> quickly, this is actually a nice way to make it into > >>>>> "exceptional" > >>>>>>>>>>> workflow > >>>>>>>>>>>> - which will push us more in making sure airflow main is > >>>>> really > >>>>>>>>> "green" > >>>>>>>>>>> - > >>>>>>>>>>>> which ultimately is our goal to make it as green as possible > >>>>> all > >>>>>> the > >>>>>>>>>>> time. > >>>>>>>>>>>> What **might help with that** (and also keeping the "enable > >>>>> auto > >>>>>>>>> merge" > >>>>>>>>>>>> might motivate it more to) is to: > >>>>>>>>>>>> * speed up the builds - we MUST prioritise now ARC (K8S > >>>>>> self-hosted > >>>>>>>>>>>> runners) to make our builds simply faster - I started a > >>>>> discussion > >>>>>>>>> and a > >>>>>>>>>>>> small group of people who can work together to complete it > >>>>> after > >>>>>>>>>>> Hussein's > >>>>>>>>>>>> POC) > >>>>>>>>>>>> * speed up the image release - with ARM runners (which we > >>>>> might be > >>>>>>>>> able > >>>>>>>>>>> to > >>>>>>>>>>>> do independently as recently I think we have > >>>>> hypervisor-enabled > >>>>>> ARM > >>>>>>>>>>> images > >>>>>>>>>>>> available as public runners as github made it generally > >>>>>> available). > >>>>>>>>>>>> * speed up the doc builds for airflow-site - we (mainly > >>>>> Pavan) are > >>>>>>>>>>> close to > >>>>>>>>>>>> complete offloading of the historical release docs to S3 > >> and I > >>>>>> hope > >>>>>>>>> it > >>>>>>>>>>> will > >>>>>>>>>>>> cut down a lot on doc publishing workflows. > >>>>>>>>>>>> J, > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@airflow.apache.org > For additional commands, e-mail: dev-h...@airflow.apache.org > >
