A comment here - and we would not have learned all that without us trying it first ! So - in retrospect - I am glad I did it, and we should do it more often IMHO to try and see and then go back with learnings (but yes, it should have been discussed at devlist if even briefly). That discussion would be very academic if we have not tried it ;)
J. On Wed, Apr 30, 2025 at 6:18 PM Kaxil Naik <kaxiln...@gmail.com> wrote: > Exactly! > > > That is an interesting point - yes, there is no way to say "only bypass > > some branch protection features" - it's either all or none. > > > Correct, good reminder, but the question is what do we want? I don't think > we want that. So just because one flaw exists (which would hopefully be > fixed), we shouldn't create another. The reason we are even have that > discussion is because we don't want to allow that. So why do it by ourself > with GitHub -- it will be even "more open" and not a way to do via CLI > > > > But just to remind - as I found out (by doing the experiment in fact) - > we > > already can bypass ANY protection (including reviews) by just pushing to > > GitBox. There is a separate discussion which I started on it with > private@ > > and security@ - so INFRA-26779 does not change much in that respect - > any > > committer can already bypass any protection. If we want to prevent that > - > > we should better chime in that other discussion and stress how important > it > > is to do something about it. > > > On Wed, 30 Apr 2025 at 21:35, Buğra Öztürk <ozturkbugr...@gmail.com> > wrote: > > > Hey all, > > > > As a concept, it can save us and make the main stable for the things > > slipping from eyes. I agree and experincing myself as well hard time > asking > > rebase on unrelated faluires. I haven't used the `escape hatch` myself. I > > always tried to make it green. It is indeed a bit time consuming at > start, > > this could led to more stable CI while always thinking that side (only > > related faluires). On the other hand, at the first steps it could be > > painful if we think similar to team is started using static code analysis > > without automated fixes then it force people to learn and follow some > > certain rules like keeping cyclomatic complexity at certain level. Things > > getting slow at first but by the time team adopt. For sure, there are > > certain concept differences though have lots of in common. > > I am okay with the auto merge. I beleive we can bring easier escape > > options. I am also okay to disable make the flow easy and then enable > again > > if the decision move towards having it. > > > > On Wed, 30 Apr 2025, 17:47 Kaxil Naik, <kaxiln...@gmail.com> wrote: > > > > > Regarding what we need, though, I don't think we want to disable branch > > > protection or allow overriding it. If that happens, PRs can be merged > to > > > main without any approval. What we really need if we enable auto-merge > is > > > some way to bypass and manually merge the PR without waiting for all > > checks > > > to be green *once there is an approved review *from a committer -- > which > > > afaik isn't available in GitHub yet. So I don't think > > > https://issues.apache.org/jira/browse/INFRA-26779 fulfills that > > > requirement. > > > > > > On Wed, 30 Apr 2025 at 20:34, Jarek Potiuk <ja...@potiuk.com> wrote: > > > > > > > And yes - apologies once again i have not checked and discussed it > > > before. > > > > We SHOULD discuss it in devlist - and I **should** hold myself > > > accountable > > > > to that similarly as I hold others, and I hope - similarly - when we > > see > > > > other important things not being discussed here - people will not > > > complain > > > > and just bring them here and straighten it up rather than being > > > defensive > > > > about it - similarly to what I just did. > > > > > > > > On Wed, Apr 30, 2025 at 5:01 PM Jarek Potiuk <ja...@potiuk.com> > wrote: > > > > > > > > > > I also tend to be -1 on this. > > > > > > > > > > So If there are also few others who have opinions here - (other > than > > > yes, > > > > > I really thought we discussed it in devlist before - but it seems > it > > > was > > > > > only on slack where we discussed it at length) - from what I see - > > > > clearly > > > > > that the "escape hatch" does not cut it and in order to make it > > really > > > > > works - we would need to be able to "override" the protection - and > > if > > > we > > > > > have it, we should ree-valuate. > > > > > > > > > > I'd love to continue the discussion here regardless - if others > have > > > > > another experience or other doubts than just "not able to merge > stuff > > > > > forcefully" - as mentioned few times, I was also worried about it > and > > > now > > > > > seeing people complaining about it, it's clear it is what we need, > > and > > > I > > > > > would take it as a positive feedback "would be nice if we had this > > > > option". > > > > > But if there are other reservations and problems - I think it's > good > > to > > > > say > > > > > them here - hopefully when we have "override" enabled it will help > > with > > > > the > > > > > future discussion on whether we should re-enable it. > > > > > > > > > > I opened a JIRA issue > > > https://issues.apache.org/jira/browse/INFRA-26779 > > > > > to see if there is anything against such a .asf.yaml feature and we > > > might > > > > > contribute it soon. > > > > > > > > > > I've learned what I wanted - and I am happy with disabling it. I > just > > > > > approved https://github.com/apache/airflow/pull/50009 to do so > > > > > (ironically all that would have to be done for it would be to > undraft > > > to > > > > > get it merged if it had auto-merge enabled :)) > > > > > > > > > > J, > > > > > > > > > > > > > > > On Wed, Apr 30, 2025 at 4:37 PM Jarek Potiuk <ja...@potiuk.com> > > 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. > > > > >> > > > > >> My mistake. It should be - similar as all the other things we > > > discussed > > > > >> then. I actually thought we discussed it. I will do better next > time > > > > and i > > > > >> expects others to follow as well - I have not set a good example > > > > >> > > > > >> Thanks for reminding it Ash. Good that we are covering each > other's > > > back > > > > >> and reminding each other about it continuously. Let's continue > doing > > > it. > > > > >> > > > > >> J, > > > > >> > > > > >> > > > > >> 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 > > > > >>> > > > > >>> > > > > > > > > > >
