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 <[email protected]> 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 <[email protected]> > 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 <[email protected]> >> 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 < >> [email protected]> 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 < >> [email protected]> 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/>
