improve and simplify linting process on tripleo repositories

Bug #1786286 reported by Sorin Sbarnea on 2018-08-09
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Sorin Sbarnea

Bug Description

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.

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.

List of CR related to this bug

Sorin Sbarnea (ssbarnea) on 2018-08-09
Changed in tripleo:
assignee: nobody → Sorin Sbarnea (ssbarnea)
Changed in tripleo:
milestone: none → stein-1
importance: Undecided → Medium
status: New → Triaged
Changed in tripleo:
status: Triaged → In Progress
Sorin Sbarnea (ssbarnea) on 2018-08-10
description: updated
Changed in tripleo:
assignee: Sorin Sbarnea (ssbarnea) → wes hayutin (weshayutin)
Changed in tripleo:
assignee: wes hayutin (weshayutin) → Sorin Sbarnea (ssbarnea)

Submitter: Zuul
Branch: master

commit 1c6cfa1e4669d107d76e58b9cfdca3437add8dc6
Author: Sorin Sbarnea <email address hidden>
Date: Thu Jul 19 15:40:32 2018 +0100

    Run bashate via pre-commit

    Migrates bashate linting task to pre-commit in order to allow fast
    and transparent execution before uploading changes to gerrit.

    Other linters can be run as well, including flake8 and ansible-linter.
    This also lowers the footprint on developer machines because each
    linter/version is stored only once on disk, regardless how many
    repositories are using it.

    Only action needed for enabling pre-commit as a developer is to
    run `pip install pre-commit`, everything else is done transparently

    Partial-Bug: #1786286
    Change-Id: I6ad834bbfbaf7645c542e4f922e89bc4449247c3

Changed in tripleo:
milestone: stein-1 → stein-2

Submitter: Zuul
Branch: master

commit 681ff01f8b810dc3e9f63de8e5ee9b2101849309
Author: Sorin Sbarnea <email address hidden>
Date: Thu Jul 12 12:38:09 2018 +0100

    enable ansible-lint as pre-commit hook

    Enable auto linting on commit, for developers
    that have pre-commit installed (pip install pre-commit)

    Execution is extreamly fast (1-2s) due to caching and
    because on commit only modified files are checked.

    ci-scripts/ is now obsolete and removed,
    as the same functionality is done via pre-commit.

    Partial-Bug: #1786286
    Change-Id: I6854145c71fd817d940c4b49b60b0853b2280cf0

Changed in tripleo:
milestone: stein-2 → stein-3
Changed in tripleo:
milestone: stein-3 → train-1
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers