Externalize linting from molecule v3

[ssbarnea]

Proposal: lint-outsourcing

Author: Sorin Sbarnea <@ssbarnea> IRC: zbr

Date: 2019-09-15

• Status: Accepted after consultation on reddit.
• Proposal type: core design
• Estimated time to implement: 2 weeks

As would like to get some community feedback regarding a major change I would like to make for v3 version of Ansible Molecule. While doing other refactoring the need to rework linting become more pressing. The whole idea is to make molecule lint easier to use and more flexible at the same time.

Another reason for this task is that linting should be performed at repository level and not at scenario level. One repository can have lots of roles and scenarios and linting at scenario level does not make much sense, creating a lot of duplicated configuration.

Molecule v3 :: Externalize linting

Linting is important but the reality is that each project may have different needs and preferences. Projects are likely to want to use more than linter, maybe to disable some checks.

We were close to completely removing linting support from molecule as it was adding a lot of complexity and coupling between different components. IRC channel has at least one question related to linting each week, the common ones where to configure it, how to disable it, why is running at the wrong step.

Having only one lint command which runs one configurable shell tasks is the way to go. This would allow users to use any linter they want, without having to modify molecule code to add support for a new linter.

We did our best to avoid execution failures when you start using v3 so this is what it will happen:

Migrating linter config from v2 to v3

If you removed or disabled linting in molecule.yml, there is nothing for you to change.

An error would be raised if obsolete lint config items are found. In order to make it easier to migrate you can see this complex setup before and after migration.

    # old v2 format:
    lint:
      name: yamllint
      enabled: true
    provisioner:
      lint:
        name: ansible-lint
      options: ...
      env: ...
    verifier:
      lint:
        name: flake8

If you want to make molecule avoid any linting remove all linting related options from the config. If you still want to perform linting, look at example below:

    # new v3 format, calling multiple linters:
    lint: |   
        yamllint .
        ansible-lint
        flake8

Behaviour to implemented

• If no lint section exists assumes no linting
• If old linting options are found, error will be displayed pointing to this ticket, which contain example on how to use new syntax
• Update documentation
• Update templates

Feedback needed

Please use up/down voting to show your support level or add comments if you have ideas about alternative approaches that should be considered.

[decentral1se]

I'm for making linting a pluggable thing.

Pre-commit has shown itself to be worthy but I'd rather just have it such that Molecule can understand that multiple linters should be run and users just use standard linter specific configuration rather than an extra tool / layer of indirection with pre-commit.

Can we clarify what exactly the linting is doing at the provisioner level and the verifier level on this ticket? I can't remember off hand and it would be good to have that written out by someone who has a clear idea. Can all linting configuration be put at the provisioner level? For example:

https://molecule.readthedocs.io/en/stable/configuration.html#lint (provisioner level linter) https://molecule.readthedocs.io/en/stable/configuration.html#verifier (each verifier has a linter)

[ssbarnea]

As linting seems to confuse others too and also prevented me from finishing other refactoring, I propose the following steps:

• Remove linter support from provisioner and verifier, only the root command will remain. While doing this molecule should not fail to run if user still has these options inside his molecule file as we do not want to force them to update their molecule.yml files.
• Make default linter pre-commit
• Update init role to also create pre-commit config with all 3 currently used linters as hooks: yamllint, ansible-lint and flake8. If a .pre-commit-config.yaml is already present in the repository it should not be touched.

This approach will assure that newly created roles will be tested very well.

I propose this because orchestrating linters is a never ending job and this feature is outside the scope of molecule. Molecule itself should be able to provide only: a template with recommended linters for new roles and a command to call the linter. If that linter is a meta-linter (like pre-commit) even better.

Mainly molecule will just run what it find under linte.name in its config, nothing else. If it find flake8 it will run flake8, if it finds echo foo it will run echo foo. The nice part about this is that this will allow users to use their own tools without having to modify molecule or to write plugins for it. This will make porting from v2 to v3 very easy, without any changes in most cases.

Example:

lint:
    name: flake8
# name: yamllint .
# name: pre-commit run -a
# name: ansible-lint

[decentral1se]

Make default linter pre-commit

Please, I am very -1 on this and I've said it numerous times before. I do not want to impose this tool on users. They are looking for their linters and those specific linter configs (which are typically well documented upstream), not a further abstraction and another thing to learn, when they are just trying to make linting work. We don't need to template out configurations for the linters. Leave it to the user. Just document clearly how to do it. We just run the linters and that's all. Pre-commit should just be another linter that pre-commit users can use if they want.

Otherwise, I am for adding a single lint command and decoupling linting from 3 places.

[tadeboro]

I would vote for decoupling linting. At least for our use case (we use Molecule for running integration tests for an Ansible collection), linting is just an annoyance that had to be disabled (https://github.com/sensu/sensu-go-ansible/blob/master/tests/integration/base.yml#L53), because it has different expectations from the ansible-test sanity command.

I also agree with @decentral1se about not enforcing the pre-commit tool. As I already said before, we use completely separate set of tools for linting our sources. I would not mind if init commands generate a template that has some preselected set of tools enabled, but we should not force the users to use them.

[ssbarnea]

Glad to hear that we agree on the single lint command and decoupling. I am not strong about default change. What would you consider that default should be? Just the current yamlint?

This means that when upgrading to 3.0, other linters will stop working. Users will not be able to run more than one linter.

Lets assume implementation is a detail, how to we want a newly generated config to look like and how should users migrate linters from v2 to v3?

[decentral1se]

This means that when upgrading to 3.0, other linters will stop working. Users will not be able to run more than one linter.

I thought the proposal was to have the molecule lint command run all the linters just like the lint sequences triggers all the linters from the 3 places?

because it has different expectations from the ansible-test sanity command.

It will be nice to have this as an additional linter. I didn't know it was so different though :/

[tadeboro]

To me, it feels like linting is not something that Molecule should be concerned with. The things I expect from Molecule to do well are:

1. Abstract away the nitty-gritty details about creating test containers or virtual machines.
2. Run my playbooks against created hosts.
3. Allow me to inspect instances while developing modules or roles.
4. Clean after the test is done.

The only exception to this rule would be creating a new role or collection. And this is where we can offer several "flavors" of initial setup. For example, we can create a Makefile in the root of the role/collection with some predefined targets such as lint that would run ansible-test sanity or ansible-list or whatever, unit for running unit tests using ansible-test units, molecule for running Molecule scenarios.

This way, we can get rid of some cruft in the Molecule's codebase while still guiding users to adequately tested roles and collections.

[decentral1se]

Not everyone does Makefile and the whole point of Molecule is to wrap the tools of the trade in a friendly fashion. Linting is a big part of getting the job done and users rely on it (I've talked to a lot of users). I see no good reason to remove it. I don't know what you're referring to as cruft? We're working on making linting pluggable, so what you're doing with your Makefile will be possible with a plugin.

[decentral1se]

@ssbarnea, from your pad:

In the end we decided to have onlu one lint sequence which run one shell command.

I'm -1 on this IIUC. Can multiple linters still be run? It's unclear to me.

We should run all linters configured, not one. The behaviour should be the same as it is now but the configurations should be under a single place, not many. Users need multiple linters and expect to be able to do that.

[decentral1se]

Points from IRC:

• "everyone agrees that we no longer want to see lint section all over the places, one in root is enough, right?"
• "the desire to run more than one linter seems real"

I think that is our current consensus!

My idea of the requirements are:

• Should be configured in a single "lint" config in the molecule.yml.
• Should be possible to run one, multiple or all linters. Should be easy from the CLI.
• Should still offer out-of-the-box working defaults.
• Should avoid lots of duplication when using multiple scenarios (should generate configs at the root level not the scenario level as per the original post in this proposal).
• Should look easily forward compatible (let's try to think through that)
• Should have a clear migration strategy for v2 -> v3.

Two problems now are:

1. What the new configuration will look like
2. What will the CLI look like?
3. Where will the code that runs linters live?

1. new config

lint:
  - project:
    - name: yamllint
  - provisioner:
    - name: ansible-lint
  - verifier:
    - name: flake8

This proposal tries to maintain the context of the original configuration (which had separated the lint sections into "project wide (all .yaml/.yml files)", provisioner and verifier). All linter configs take the same original options: 'options: config-file: "..."' etc.

The advantage of keeping project/provisioner/verifier is that the linter class can still safely implement _get_files/_get_tests to ensure a good working default for the user. The verifier section linter will always look for files in the molecule/**/tests/* location so you don't need to configure that (as it is now).

I can now run multiple linters:

lint:
  - project:
    - name: yamllint
    - name: pre-commit

2. CLI

And my CLI will correspond to:

$ molecule lint --verifier  # run linters in verifier scope
$ molecule lint --project  # run linters in project scope

3. code

Move all linters that implement Base into a single directory. The code is actually a small maintenance issue and with new docs, it will be clear how to implement it.

Alternatively, we could create a molecule-linters repository and pypi package where we do the same but outside of the core repository. It's one pypi package and users can just add their linter file there. We would then remove the Base class out of Molecule core and core code would simply run the expected API on the linter object.

[ssbarnea]

• no cli changes, molecule lint should lint.
• avoid scope-creeping for this change alone: run all linters by default without any option to configure them, lets keep this for follow-ups (is not possible even now)
• we document that lint must runs as soon as posible but after dependency, we already know why as this was fixed recently.
• i see no need to keep any linting classes in molecule, I only want commands, these are more flexible and accesible than any plugins.
• molecule should not attempt to install linters itself but for backwards compatibility we should keep them as dependencies (flake8,ansible-lint,pre-commit,yamllint). Maybe we can make them extras in the future for for initial change we should minimize the required porting maintenance.
• DRY:

  # v2/v3 compatible!!!
  lint:
  name: yamllint  # display deprecation warning asking you to replace name with cmd
  enabled: false  # display deprecation, asks user to use ansible, like `when: false`

# v3 only, multiple linters
lint:
  cmd: yamllint && ansiblelint

In fact this setup may also allow us to add extra sequences in the future that are nothing else than ansible shell tasks.

Update: I got an idea, how about doing a github-advanced-search survey and get an idea about how many molecule.yml file are using linter options, something that we want to remove.

[decentral1se]

avoid scope-creeping

OK.

i see no need to keep any linting classes in molecule, I only want commands, these are more flexible and accesible than any plugins.

From https://github.com/ansible/molecule/issues/2293#issuecomment-554941778:

the linter class can still safely implement _get_files/_get_tests to ensure a good working default for the user. The verifier section linter will always look for files in the molecule/*/tests/ location so you don't need to configure that (as it is now).

Classes bring good defaults. That's another thing not to think about. How do you propose to drop the classes and still have pre-baked _get_files/_get_tests that the user doesn't need to bother to configure because these files are always in the same place by default convention.

cmd example

lint:
  cmd: yamllint && ansiblelint

But noone will be running that command because they'll have to hard-code the paths. So, it'll be instead:

lint:
  cmd: yamllint . && ansible-lint molecule/**/ tasks  # or whatever

Update: I got an idea, how about doing a github-advanced-search survey and get an idea about how many molecule.yml file are using linter options, something that we want to remove.

Fair enough although not all are on Github and I expect the real advanced users are private repositories. It will give some indication but let's not stake it all on that.

[ssbarnea]

Ansible-lint is already able to detect roles and playbooks so it no longer needs any arguments. I implemented this features last month and it will make its use much easier. For yamllint I proposed it at https://github.com/ansible/molecule/issues/2293 and hopefully it will be accepted.

Looking back I realise that we could make the transition easy by deprecating old arguments and telling people with to write instead. The key aspect here is to assure that when we find the old style user we fail with a very clear message: "replace ... with ...".

[ssbarnea]

Please note that I rewrote the original proposal and copied that content from https://www.reddit.com/r/ansible/comments/e1wq19/proposal_on_ansible_molecule_v3_major_linting/ post which apparently got 94% approval. This means that comments above may refer to the older version of the proposal. You may want to update or remove them.

[ssbarnea]

@geerlingguy please have a look at the description, the comments may be little out-of-sync as the description was updated more recently. I think that the example on how the new config would look should be self-explanatory.

[malodie]

Not having much luck with this new linting format and the docker container. It works fine natively, but the lint isn't handled. Please let me know if this is not the appropriate place for this, and I can comment elsewhere.

docker run --rm -it \ -v "$(pwd)":/tmp/$(basename "${PWD}"):ro \ -v /var/run/docker.sock:/var/run/docker.sock \ -w /tmp/$(basename "${PWD}") \ quay.io/ansible/molecule:latest \ molecule test ERROR: Failed to validate. {'lint': ['must be of dict type'], 'verifier': [{'name': ['unallowed value ansible']}]}