+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/> > >> > > >>
