[AIRFLOW-5161] Static checks are run automatically in pre-commit hooks

[potiuk]

Make sure you have checked all steps below.

Jira

• My PR addresses the following Airflow Jira issues and references them in the PR title. For example, "[AIRFLOW-XXX] My Airflow PR"
  • https://issues.apache.org/jira/browse/AIRFLOW-5161

Description

• Here are some details about my PR, including screenshots of any UI changes:

Tests

• My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

• My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
  1. Subject is separated from body by a blank line
  2. Subject is limited to 50 characters (not including Jira issue reference)
  3. Subject does not end with a period
  4. Subject uses the imperative mood ("add", not "adding")
  5. Body wraps at 72 characters
  6. Body explains "what" and "why", not "how"

Documentation

• In case of new functionality, my PR adds documentation that describes how to use it.
  • All the public functions and the classes in the PR contain docstrings that explain what it does
  • If you implement backwards incompatible changes, please leave a note in the Updating.md so we can assign it to a appropriate release

Code Quality

• Passes flake8

[potiuk]

Hey. While flying to NYC I managed to rebase, cleanup and trim-down pre-commit hook implementation for all our static checks. I would love if you can take a look, review and try it out. It is built on top of the CI image (and the scripts that build it locally).

It is super easy to install and you can use it to run your checks locally. The best thing is that it runs automatically on multiple processors and only for the files you change in your commit - so it is really fast to run pylint + mypy + flake + docker lint (and you do not have to remember which files you should run it on. And it is super easy to trigger it manually.

I think if we promote it within community that might significantly improve cycle time for development (especially taking into account TravisCI slowness). The checks are using exactly the same docker image and the same configurations that Travis CI uses - which gives more confidence (not 100% but close) before you push to Travis.

I am happy to iterate on it a bit while I am starting holidays as this could be really useful to get better development experience.

Once we merge this one I have a number of other pre-commit hooks that we can add over time (validating xml files, yaml, shellcheck. tab removal etc.) - but those can be added gradually - for now I focused on having our current checks in pre-commit.

[potiuk]

@nuclearpinguin ^^ also you can help to see if that works as expected :)

[potiuk]

All documentation and instructions are in https://github.com/PolideaInternal/airflow/blob/pre-commit-hooks/CONTRIBUTING.md#pre-commit-hooks

[dimberman]

@potiuk dang that was a seriously productive flight. Looking over now :)

[dimberman]

Hi @potiuk I did a first run of the documentation/a request to rewrite some env variables. Could you handle those and then I'll go over the rest?

[potiuk]

Thanks @dimberman! Very good comments. I improved the docs quite a bit now.

Note also that I am not too worried about having perfect documentation just yet :). I know I am not the best both from grammar and writing instructions to developers (I make too many assumptions about the things I know), but this is going to change hopefully.

We have been granted two Technical Writers by the Google Summer of Docs initiative and one of the writers (Elena) decided to work on my proposal (I am a mentor in the program) to improve the whole on-boarding documentation for new users.

That including Breeze (sooon!) and development environment in general. So over the next few months, I will be working with her to improve that part significantly - both from the English/grammar point of view and the way how the whole information is structured. I hope to learn a lot from her !! (she is a senior technical writer from Intel).

[dimberman]

@potiuk No worries at all! As I've said before, if I had to write documents in Polish the results would not be pretty. That's awesome to hear we'll have an actual technical writer on board. Could you please introduce me to her? I'd love to work with her to make better onboarding docs for the k8sexecutor.

[potiuk]

Hey @dimberman (and others ;) - @BasPH ). I made a slight improvement to pre-commit framework. Now it is not only used by pre-commit hook but also for travis CI tests (with --all-files switch) for all static checks in one job (except pylint). This saves quite a lot of overhead for booting VMs on travis - we have less jobs to handle and it's rather fast. Also as we add more static checks they will be run automatically as we add them in the same job. And people will get used to the pre-commit interface showing passsed/failed. See https://travis-ci.org/apache/airflow/jobs/570789001 for the job that runs the static checks.

I have more separate PRs to add more static checks which I will add separately.

[potiuk]

And now it's also locally a bit simpler when your check requires rebuilding of the image (for example when you changed setup.py). It will fail and print the message informing that you should add REBUILD=true in front of your command to make it works or that you should run the git message with --no-verify switch:

ERROR! Quitting. The images need rebuilding.

  You should re-run your command with REBUILD=true environment variable set

  For example 'REBUILD=true git commit'
  or 'REBUILD=true git push'

  In case you do not want to rebuild, You can always commit the code
  with --no-verify switch. This skips pre-commit checks.
  Still, the tests will be run on CI will run the checks for you.

[Fokko]

Instead of Static checks except pylint and docs, can we mention what's actually being run, mypy for example.

[Fokko]

Works like a charm:

MacBook-Pro-van-Fokko:incubator-airflow fokkodriesprong$ ./scripts/ci/ci_run_all_static_tests.sh
Check hooks apply to the repository......................................Passed
Lint dockerfile..........................................................Passed
Run mypy.................................................................Passed
Run pylint for main sources.............................................Skipped
Run pylint for tests....................................................Skipped
Run flake8...............................................................Passed
MacBook-Pro-van-Fokko:incubator-airflow fokkodriesprong$ ./scripts/ci/ci_pylint_tests.sh

Running pylint for 'tests' folder


------------------------------------
Your code has been rated at 10.00/10


Pylint check succeeded

MacBook-Pro-van-Fokko:incubator-airflow fokkodriesprong$ ./scripts/ci/ci_pylint_tests.sh

Running pylint for 'tests' folder


------------------------------------
Your code has been rated at 10.00/10


Pylint check succeeded

[potiuk]

Thanks @Fokko -> There are more checks to come, the whole idea is that we can add many more static checks (I have the follow-up PRs adding them) so it will be difficult to maintain such list. It's better to have one job covering all the checks (then the overhead for starting the job will be much smaller). We will run all the checks here except docs build (this is not really applicable for incremental pre-commit checks) and pylint (not applicable until we finish pylint introduction and get rid of pylint_todo.txt). So I prefer to leave that name.

@dimberman - are you happy with my fixes to docs ? I would love to merge that one so that I can better handle the follow-up changes with more static checks and get them added one-by-one

[dimberman]

@potiuk LGTM :)