Comment 0 for bug 1786286

Revision history for this message
Sorin Sbarnea (ssbarnea) wrote :

Simple and effective linting can not only avoid introducing bugs but also avoid wasting developer and CI time, cutting the time needed for review to get merged.

openstack-tox-linters
=====================
Historically openstack projects had a "pep8" check which did pep8 (flake8) testing, this being by far the most popular linter used on openstack projects.

Still, linting is not limited to python code and we would benefit from using other linters like:

* yamllint
* ansible-lint (ansible-review)
* bashate (or shellcheck)
* spellchecking,
* more generic checks like consistent whitespace usage
* commit checks like 50/72 rule

As a sidenode, pylint was deliberately left out of this list because it is know to be a very resource intensive linter, thus a controversial one.

Zuul has a general template named `openstack-tox-linters` which is supposed to slowly replace the `openstack-tox-pep8` one. The only difference is that this runs "tox -e linters" instead of "tox -e pep8".

This avoids the confusion that this check would only check for "pep8" compliance. This was needed because it is unreasonable to create new jobs for every possible new linter. The check name should be able what it does, not what tool(s) it was using to achieve it. Even flake8 was renamed to pycodestyle and nobody wants to start doing search and replace in tons of repos.

Having one generic check that runs all linters enabled by the project keeps things simple and allows incremental (linting) improvements without having to create or rename CI jobs. Whatever new features are implemented, they are added as additional commands on `linters` tox environment, which should be the first one listed in envlist section.

pre-commit linting
==================
pre-commit is a small but effective way to deploy and manage git hooks. Once installed using
`pip install pre-commit` it will hook into git and transparently manage hooks defined in .pre-commit-hooks.yaml file from the repository.

pre-commit has few essential features that make it very good linting solution:
- by default it runs only on touched files (huge speedup)
- user level caching (avoid growing .tox folders for each repository)
- normal execution duration is 1-2s even if you have like 4-10 linters, considerably faster than tox (and with lower footprint)
- integrates perfectly with tox
- reproducible by design (all linters are pinned, similar to hacking idea)
- single command to update linters
- extra checks (like whitespace), being able to even auto-fix some errors on commit.

Once pre-commit is enabled, a developer will get errors reported before he would even raise the change request, saving not only his own time, but also saving CI from running very expensive jobs
multiple times, just because of some trivial errors that could have being cough faster.

Indeed, developer would need to install pre-commit to benefit from it but this is hardly an issue because the time saved would make it worth. This system works ok as it degrades gracefully, if a developer does not have and run the pre-commit hooks, CI will do it for him inside the linters check.