Re: [DISCUSSION] Enabling `pre-commit.ci` application for Airflow
I'm in favor of this. I love making docs changes directly in GitHub, but I often make a tiny mistake like a trailing space and the tests fail. I think things like this discourage new contributors, as contributing to docs is the easiest way to start getting involved. On Thu, Jan 4, 2024 at 12:16 PM Jarek Potiuk wrote: > Yep. Also surprised by the 50/50 - so far the "easy" path is blocked > by INFRA, so I am not sure if we will quickly do it, but I will likely > see what we can do soon. > > And yes. This is the same for me - I **LOVE** black and always have > pre-commit installed because I do not have to spend any mind-cycles on > things that are extremely important for the project and readability > (i.e. consistency) but extremely unnecessary to worry about it when I > think about solving real problems. > > On Thu, Jan 4, 2024 at 8:05 PM Oliveira, Niko > wrote: > > > > 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 > > 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 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, Sc
Re: [DISCUSSION] Enabling `pre-commit.ci` application for Airflow
Yep. Also surprised by the 50/50 - so far the "easy" path is blocked by INFRA, so I am not sure if we will quickly do it, but I will likely see what we can do soon. And yes. This is the same for me - I **LOVE** black and always have pre-commit installed because I do not have to spend any mind-cycles on things that are extremely important for the project and readability (i.e. consistency) but extremely unnecessary to worry about it when I think about solving real problems. On Thu, Jan 4, 2024 at 8:05 PM Oliveira, Niko wrote: > > 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 > 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 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 > >> 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) > >>> 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> > >>>> __
Re: [DISCUSSION] Enabling `pre-commit.ci` application for Airflow
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 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 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 >> 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) >>> 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 >>>> Sent: Saturday, December 30, 2023 7:50:10 AM >>>> To: 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, 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
Re: [DISCUSSION] Enabling `pre-commit.ci` application for Airflow
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 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 >> 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) >>> 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 >>>> Sent: Saturday, December 30, 2023 7:50:10 AM >>>> To: 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, 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 >>> wrote: >>>>> >>>>>> Hello everyone, >>>>>> >>>>>> TL;DRl; I'd like to propose that we enable the pre-commi
Re: [DISCUSSION] Enabling `pre-commit.ci` application for Airflow
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 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 >> 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) >>> 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 >>>> Sent: Saturday, December 30, 2023 7:50:10 AM >>>> To: 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, 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 >>> wrote: >>>>> >>>>>> Hello everyone, >>>>>> >>>>>> TL;DRl; I'd like to propose that we enable the pre-commi
Re: [DISCUSSION] Enabling `pre-commit.ci` application for Airflow
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 > 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) > > 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 > > > Sent: Saturday, December 30, 2023 7:50:10 AM > > > To: 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, 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 > > 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 > > >
Re: [DISCUSSION] Enabling `pre-commit.ci` application for Airflow
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 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) > 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 > > Sent: Saturday, December 30, 2023 7:50:10 AM > > To: 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, 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 > 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
Re: [DISCUSSION] Enabling `pre-commit.ci` application for Airflow
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) 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 > Sent: Saturday, December 30, 2023 7:50:10 AM > To: 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, 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 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=05%7C02%7CJens.Scheffler%40de.bosch.com%7C7292ed56c2374a93eb1508dc09039ec0%7C0ae51e1907c84e4bbb6d648ee58410f4%7C0%7C0%7C638395158349294376%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C=tgcyFz64S3IBLcAAYUYjswy7cqG%2FZ0KNbgRtSJyxOxQ%3D=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 automat
Re: [DISCUSSION] Enabling `pre-commit.ci` application for Airflow
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 Sent: Saturday, December 30, 2023 7:50:10 AM To: 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, 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 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=05%7C02%7CJens.Scheffler%40de.bosch.com%7C7292ed56c2374a93eb1508dc09039ec0%7C0ae51e1907c84e4bbb6d648ee58410f4%7C0%7C0%7C638395158349294376%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C=tgcyFz64S3IBLcAAYUYjswy7cqG%2FZ0KNbgRtSJyxOxQ%3D=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=05%7C02%7CJe
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, 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 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://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://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. > > >
Re: [DISCUSSION] Enabling `pre-commit.ci` application for Airflow
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 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://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://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. >
[DISCUSSION] Enabling `pre-commit.ci` application for Airflow
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://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://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.
