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
