Interesting how 50/50 this one has turned out to be! I'm personally in favour (+1). The less I have to worry about accidental typos, indentation, quoting, etc the better, I can focus on important changes. It will also unblock many PRs from contributors that are otherwise mergeable except for being stuck on very simple static check failures, which as a maintainer sounds very nice (it will solve having to post the regular comment of "please run and fix static checks").
And ultimately if the bot does something silly (just as a human can and often does) we can catch it in the PR review. Cheers, Niko ________________________________ From: Wei Lee <weilee...@gmail.com> Sent: Tuesday, January 2, 2024 5:58:18 PM To: dev@airflow.apache.org Subject: RE: [EXTERNAL] [COURRIEL EXTERNE] [DISCUSSION] Enabling `pre-commit.ci` application for Airflow CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe. AVERTISSEMENT: Ce courrier électronique provient d’un expéditeur externe. Ne cliquez sur aucun lien et n’ouvrez aucune pièce jointe si vous ne pouvez pas confirmer l’identité de l’expéditeur et si vous n’êtes pas certain que le contenu ne présente aucun risque. Same as Amogh. Even though I would like to fix that myself, it would make it much easier for those who aren’t familiar with these tools and still be able to contribute. But we might need to doc this behavior somewhere (GitHub PR issue might make more sense 🤔). Otherwise, the contributor might be surprised by the new commit. Best, Wei > On Jan 3, 2024, at 12:21 AM, Vincent Beck <vincb...@apache.org> wrote: > > I like the concept! +1 > > On 2023/12/30 11:16:35 Amogh Desai wrote: >> I am aligning here with Pierre, but I am not against the idea of enabling >> the pre commit ci application. >> >> I’d rather have myself fix the issue as it sometimes also lets me have >> second,third or multiple passes at my code which is up for review. This is >> a personal choice where I feel that we are trying to fix a problem that is >> not too problematic. >> >> Again, only a personal choice but not against it. If it makes lives of the >> stakeholders involved easier, I am all up for it! >> >> Thanks & Regards, >> Amogh Desai >> >> On Sat, 30 Dec 2023 at 2:35 PM, Pierre Jeambrun <pierrejb...@gmail.com> >> wrote: >> >>> I like the idea, but in practice auto fixable static checks are very >>> obvious to fix and doesn’t require much work. >>> >>> On the other hand most of static failure are ‘real issues’ and not auto >>> fixable, for instance mypy, spelling, sphinx, db session usage etc…. (And >>> ruff fix is a little aggressive IMO regarding linting). >>> >>> So I would say that in practice it solves a painless problem (formatting, >>> import sorting and other obvious things) and can’t do much about other >>> issues. >>> >>> This is why I am not sure it is worth the confusion for users. (But I am >>> not against it) >>> >>> On Sat 30 Dec 2023 at 09:19, Scheffler Jens (XC-DX/PJ-PACE-E03) >>> <jens.scheff...@de.bosch.com.invalid> wrote: >>> >>>> I‘d also like to have auto-fixing included in CI. It is a classic pitfall >>>> and all that can be automated does not need to be a manual burden. >>>> Though I am not sure whether the plugin is able to use all the custom >>>> stuff as we also depend during execution on the CI image and docker. >>>> Besides security things this would be something that needs testing if it >>>> works. >>>> >>>> TLDR: +1 opinion >>>> >>>> Sent from Outlook for iOS<https://aka.ms/o0ukef> >>>> ________________________________ >>>> From: Pankaj Koti <pankaj.k...@astronomer.io.INVALID> >>>> Sent: Saturday, December 30, 2023 7:50:10 AM >>>> To: dev@airflow.apache.org <dev@airflow.apache.org> >>>> Subject: Re: [DISCUSSION] Enabling `pre-commit.ci` application for >>> Airflow >>>> >>>> I very much like the concept. We have been using it actively for >>> Astronomer >>>> code repositories for 1+ year already and it has helped us greatly >>> (Thanks >>>> to Felix Uellendall for introducing this back then 🙂) >>>> >>>> On Sat, 30 Dec 2023, 12:10 Jarek Potiuk, <ja...@potiuk.com> wrote: >>>> >>>>> FYI - Just now INFRA rejected the request on the basis of "code write" >>>>> access permissions the app needs. >>>>> >>>>> I'd still love to get feedback though on the concept - I am not giving >>>> up >>>>> that easily. We might still get it approved easily. We likely have some >>>>> ways we can get "auto-fixing" working for us. >>>>> >>>>> 1) I believe Github Applications now can use a bit different mechanism >>> to >>>>> ask for permissions and it can have "required" and "optional" >>> permissions >>>>> and I believe "Pull request write" should be enough (and I might >>> attempt >>>> to >>>>> convince the maintainers of it to adapt it to our needs). >>>>> 2) Also, there is a "Pre-commit Lite Github Action" that we **might** >>> be >>>>> able to use to achieve a similar effect (with some added complexity to >>>> our >>>>> `Pull Request Target` workflow. >>>>> >>>>> So I would still love to hear from others :) >>>>> >>>>> J. >>>>> >>>>> On Fri, Dec 29, 2023 at 11:52 PM Jarek Potiuk <ja...@potiuk.com> >>> wrote: >>>>> >>>>>> Hello everyone, >>>>>> >>>>>> TL;DRl; I'd like to propose that we enable the pre-commit-ci GitHub >>>>>> application for Airflow repo. According to how I understand it works, >>>> it >>>>>> should greatly reduce friction (especially for new contributors) for >>>>>> passing the quality gates for our pre-commits. That is - if we get >>> the >>>>>> approval for pre-commit-ci application from the ASF infra team. >>>>>> >>>>>> Some more context: >>>>>> >>>>>> We use and love (well some of us do, some of us likely hate) the >>>>>> pre-commit as a quality gate for our checks. We have been using it >>> for >>>>>> years for local checks and CI integration and we have ~60 custom >>>>> precommits >>>>>> and in total we use about 100 pre-commit checks as our "quality" >>> gates >>>>>> >>>>>> However, using `standard` pre-commit (that is a de-facto standard in >>>>>> Python world) has a nice property of 'standing on the shoulders of >>>>> giants'. >>>>>> There is one thing that few of us are aware of, that there is a way >>> to >>>>>> reduce friction for pre-commits that are not only flagging errors but >>>> can >>>>>> also fix them. If we get the `pre-commit-ci` application ( >>>>>> >>>> >>> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpre-commit.ci%2F&data=05%7C02%7CJens.Scheffler%40de.bosch.com%7C7292ed56c2374a93eb1508dc09039ec0%7C0ae51e1907c84e4bbb6d648ee58410f4%7C0%7C0%7C638395158349294376%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=tgcyFz64S3IBLcAAYUYjswy7cqG%2FZ0KNbgRtSJyxOxQ%3D&reserved=0 >>>> )<https://pre-commit.ci/> approved for our repo from the ASF infra team >>> it >>>>>> - in theory - should be able to AUTO-FIX PRs that are not passing the >>>>>> pre-commits (and can be automatically corrected). >>>>>> >>>>>> Yep. You read it right. No more asking a new contributor "please fix >>>>>> static checks" - PRs that have auto-fixable pre-commit failures will >>> be >>>>>> fixed automatically. >>>>>> >>>>>> For example when you make a PR that does not pass "ruff" formatting, >>>> the >>>>>> application should automatically amend the PR and FIX IT. We have >>>> quite a >>>>>> number of such PRs from first-time contributors, but also a number of >>>>>> seasoned contributors (including myself) occasionally send a PR that >>>> does >>>>>> not pass an auto-fixable static check. This can happen with a few >>>>> scenarios >>>>>> (rebasing, or correcting a PR by applying a suggestion from review >>> and >>>> a >>>>>> few other scenarios). >>>>>> >>>>>> It can be a little strange to see your PR corrected by a bot though, >>>> so I >>>>>> am reaching out here to see if you think it is a good idea. I also >>>>> opened a >>>>>> JIRA request to approve the application (but I made a comment that it >>>>>> should be pending the discussion here): >>>>>> >>>> >>> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fissues.apache.org%2Fjira%2Fbrowse%2FINFRA-25322&data=05%7C02%7CJens.Scheffler%40de.bosch.com%7C7292ed56c2374a93eb1508dc09039ec0%7C0ae51e1907c84e4bbb6d648ee58410f4%7C0%7C0%7C638395158349450599%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=mRyEImOqrqGXmbMxxLvPG5H%2F5J5CsnNdZH%2FYjJeRJLg%3D&reserved=0 >>>> <https://issues.apache.org/jira/browse/INFRA-25322> - it will likely >>>>>> require to slightly change our workflows to make it works as well. >>>>>> >>>>>> Do you think it's a good idea to have it enabled? Maybe it will be >>> too >>>>>> much for our contributors and they will be surprised to see it >>>> happening? >>>>>> >>>>>> WDYT? >>>>>> >>>>>> J. >>>>>> >>>>> >>>> >>> >> > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@airflow.apache.org > For additional commands, e-mail: dev-h...@airflow.apache.org > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@airflow.apache.org For additional commands, e-mail: dev-h...@airflow.apache.org
