I agree with the other folks. I'm personally not a fan of pre commit hooks, but if people like it, they can enable it.
Cheers, Fokko Op di 23 jul. 2019 om 20:13 schreef Kaxil Naik <kaxiln...@gmail.com> > It is fully-optional so I don't think we need an AIP there. > > > > On Tue, Jul 23, 2019 at 11:36 PM Beau Barker <beauinmelbou...@gmail.com> > wrote: > > > Just add a .pre-commit-config.yaml to the project, no need for an AIP. > > > > > On 24 Jul 2019, at 2:42 am, Jarek Potiuk <jarek.pot...@polidea.com> > > wrote: > > > > > > Any more comments on it? > > > Should I make an AIP for that :)? Or should I just ask a vote/propose > a > > PR > > > ? Anyone has a strong opinion? > > > I think it changes the dev workflow quite a bit on one hand, but it is > > > fully optional on the other hand. > > > > > > On Thu, Jul 4, 2019 at 9:23 AM Kamil Breguła < > kamil.breg...@polidea.com> > > > wrote: > > > > > >> +1 At first I was skeptical about this tool. I prefer to run it by > > >> hand, but I am more and more convinced that using it as a pre-commit > > >> makes sense. > > >> > > >> On Wed, Jul 3, 2019 at 10:58 PM Felix Uellendall > <felue...@pm.me.invalid > > > > > >> wrote: > > >>> > > >>> +1 (non-binding), I love that. I use a git pre-commit bash script > but I > > >> think the framework looks even better. :) > > >>> > > >>> -feluelle > > >>> > > >>> -------- Original Message -------- > > >>>> On Jul 2, 2019, 19:48, Maxime Beauchemin wrote: > > >>>> > > >>>> +1 > > >>>> > > >>>> On Mon, Jul 1, [2019](tel:2019) at 11:20 PM Kaxil Naik < > > >> kaxiln...@gmail.com> wrote: > > >>>> > > >>>>> +1 We have been using this on some of our Astronomer repositories > as > > >> well > > >>>>> and have been happy with it. > > >>>>> > > >>>>> Regards, > > >>>>> Kaxil > > >>>>> > > >>>>> On Tue, Jul 2, [2019](tel:2019), 11:46 Jarek Potiuk < > > >> jarek.pot...@polidea.com> wrote: > > >>>>> > > >>>>>> TL;DR: I would like to make a proposal to add (easily managed) > > >>>>> pre-commit > > >>>>>> hooks with various lint checkers part of recommended (and > > >> encouraged) > > >>>>>> development workflow for Airflow. This will help with speeding-up > > >> local > > >>>>>> development cycle and decrease pressure on Travis CI. > > >>>>>> > > >>>>>> Over the last few months in our other oozie-to-airflow > > >>>>>> <https://github.com/GoogleCloudPlatform/oozie-to-airflow> project > > >> we've > > >>>>>> been running pre-commit hooks - we've integrated the fantastic > > >>>>>> https://pre-commit.com/ framework and we never looked back. The > > >> upcoming > > >>>>>> changes to docker images (AIP-10) makes it easy to standardise > > >>>>> pylint/mypy > > >>>>>> and other checks to be the same for every developer (using docker > > >> images) > > >>>>>> so it also makes possible to have consistent pre-commit > environment > > >> for > > >>>>>> static checks for everyone. > > >>>>>> > > >>>>>> Pre-commit framework has a number of cool features you would > expect > > >> from > > >>>>>> such tool: > > >>>>>> > > >>>>>> - it is super easy to install and manage the checks ('pre-commit > > >>>>> install') > > >>>>>> - all the checks are managed in single .pre-commit-config.yaml > file > > >> (in > > >>>>>> repo) > > >>>>>> - it allows to run various checks only on the changes you are > > >> committing > > >>>>>> - it has nice and simple UI to run checks individually or skip > some > > >>>>> checks, > > >>>>>> or run on all files > > >>>>>> - it automatically uses all processors on your machine (it will > > >>>>> distribute > > >>>>>> changed files evenly) > > >>>>>> - it has great UI for reporting what's going on > > >>>>>> - it has a number of pre-defined checks that are super-useful > > >>>>>> - check if you have not left debug instructions (Ash :) - no more > > >>>>>> problems with that!) > > >>>>>> - check if you resolved merge conflicts > > >>>>>> - check if you have not submitted a private key > > >>>>>> - check correctness of various files (shell, yaml, xml, whether > > >>>>> shebangs > > >>>>>> are added correctly etc.) > > >>>>>> - .... > > >>>>>> - it can even correct some of the problems found (it leaves > > >> corrected > > >>>>> files > > >>>>>> for you to commit): > > >>>>>> - automatically adding missing licences > > >>>>>> - automatically add python-encoding pragma > > >>>>>> - add Table of Contents for markdown files > > >>>>>> - and many more > > >>>>>> - it can be integrated in CI to run the very same checks but on > all > > >> files > > >>>>>> during CI. > > >>>>>> - you can use --no-verify switch to skip pre-commit > > >>>>>> > > >>>>>> There are multiple advantages of using pre-commit hooks. It not > only > > >>>>> speeds > > >>>>>> up local development (no more waiting for Travis) but if most > > >> people will > > >>>>>> start using it - it will also decrease the pressure on Travis - > less > > >>>>> people > > >>>>>> will be submitting PRs just to check if they pass all the checks. > I > > >> think > > >>>>>> that might be single biggest thing that we can do to have far less > > >> CPU > > >>>>> used > > >>>>>> by Airflow builds. > > >>>>>> > > >>>>>> There is one disadvantage I noticed - when you have unstaged > > >> changes, > > >>>>>> pre-commit stashes them before running checks and sometimes (if > you > > >>>>> Ctrl^C > > >>>>>> very quickly) it will not stash them back - but it is recoverable > > >> it just > > >>>>>> needs one liner to recover. In normal circumstances it is really > > >> safe to > > >>>>>> run. > > >>>>>> > > >>>>>> I already made a working POC of pre-commit (not intended for merge > > >> - it > > >>>>> has > > >>>>>> many more unrelated changes that I am going to submit as separate > > >> PRs). > > >>>>> It > > >>>>>> takes less than [20-30](tel:2030) seconds to run all the checks in > > >> a small commit > > >>>>>> (including pylint/mypy and other checks) so IMHO it is perfectly > OK > > >> to > > >>>>> run > > >>>>>> on every commit. You can see the Job in CI here: > > >>>>>> https://travis-ci.org/PolideaInternal/airflow/jobs/552862055 and > a > > >> short > > >>>>>> documentation I wrote on using pre-commits here: > > >>>>>> > > >>>>>> > > >>>>> > > >> > > > https://github.com/PolideaInternal/airflow/blob/pre-commit-hooks/CONTRIBUTING.md#pre-commit-hooks > > >>>>>> > > >>>>>> Here are the checks I managed to get running for Airflow (pylint > is > > >>>>> skipped > > >>>>>> for now until we finish it but it can be tested in separate job - > I > > >> also > > >>>>>> did it in the same build): > > >>>>>> > > >>>>>> Docker build - might take longer on setup.py change (see > > >>>>>> ./.build/cache dir for logs).......................Passed > > >>>>>> Add licence for all XML, md > > >>>>>> > > >>>>>> > > >>>>> > > >> > > > files...........................................................................Passed > > >>>>>> Add licence for all JS > > >>>>>> > > >>>>>> > > >>>>> > > >> > > > files................................................................................Passed > > >>>>>> Add licence for all SQL > > >>>>>> > > >>>>>> > > >>>>> > > >> > > > files...............................................................................Passed > > >>>>>> Add licence for all JINJA template > > >>>>>> > > >>>>>> > > >>>>> > > >> > > > files....................................................................Passed > > >>>>>> Add licence for all other > > >>>>>> > > >>>>>> > > >>>>> > > >> > > > files.............................................................................Passed > > >>>>>> No-tabs > > >>>>>> > > >>>>> > > >> > > > checker.............................................................................................Passed > > >>>>>> Check yaml files with > > >>>>>> > > >>>>>> > > >>>>> > > >> > > > yamllint..............................................................................Passed > > >>>>>> Check that executables have > > >>>>>> > > >>>>>> > > >>>>> > > >> > > > shebangs........................................................................Passed > > >>>>>> Check for merge > > >>>>>> > > >>>>>> > > >>>>> > > >> > > > conflicts...................................................................................Passed > > >>>>>> Check > > >>>>>> > > >>>>> > > >> > > > Xml...................................................................................................Passed > > >>>>>> Debug Statements > > >>>>>> > > >>>>>> > > >>>>> > > >> > > > (Python)...................................................................................Passed > > >>>>>> Detect Private > > >>>>>> > > >>>>> > > >> > > > Key..........................................................................................Passed > > >>>>>> Fix python encoding > > >>>>>> > > >>>>>> > > >>>>> > > >> > > > pragma..................................................................................Passed > > >>>>>> Fix End of > > >>>>>> > > >>>>> > > >> > > > Files............................................................................................Passed > > >>>>>> Mixed line > > >>>>>> > > >>>>> > > >> > > > ending...........................................................................................Passed > > >>>>>> Check hooks apply to the > > >>>>>> > > >>>>>> > > >>>>> > > >> > > > repository.........................................................................Passed > > >>>>>> Add TOC to markdown > > >>>>>> > > >>>>>> > > >>>>> > > >> > > > documents...............................................................................Passed > > >>>>>> Check Shell scripts syntax > > >>>>>> > > >>>>>> > > >>>>> > > >> > > > corectness.......................................................................Passed > > >>>>>> Lint > > >>>>>> > > >>>>> > > >> > > > dockerfile.............................................................................................Passed > > >>>>>> Run > > >>>>>> > > >>>>> > > >> > > > mypy....................................................................................................Passed > > >>>>>> Run pylint for main > > >>>>>> > > >>>>>> > > >>>>> > > >> > > > sources................................................................................Skipped > > >>>>>> Run pylint for > > >>>>>> > > >>>>> > > >> > > > tests.......................................................................................Skipped > > >>>>>> Run > > >>>>>> > > >>>>> > > >> > > > flake8..................................................................................................Passed > > >>>>>> > > >>>>>> > > >>>>>> J. > > >>>>>> > > >>>>>> J. > > >>>>>> > > >>>>>> -- > > >>>>>> > > >>>>>> Jarek Potiuk > > >>>>>> Polidea <https://www.polidea.com/> | Principal Software Engineer > > >>>>>> > > >>>>>> M: [+48 660 796 129](tel:+48660796129) > > >> <[+48660796129](tel:+48660796129)> > > >>>>>> [image: Polidea] <https://www.polidea.com/> > > >>>>>> > > >>>>> > > >> > > > > > > > > > -- > > > > > > Jarek Potiuk > > > Polidea <https://www.polidea.com/> | Principal Software Engineer > > > > > > M: +48 660 796 129 <+48660796129> > > > [image: Polidea] <https://www.polidea.com/> > > > > > -- > *Kaxil Naik* > *Big Data Consultant | DevOps Data Engineer* > *Certified *Google Cloud Data Engineer | *Certified* Apache Spark & Neo4j > Developer > *LinkedIn*: https://www.linkedin.com/in/kaxil >
