Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

yamllint soft-dependency should be removed #1735

Closed
jamesquilty opened this issue Oct 7, 2021 · 6 comments · Fixed by #1881
Closed

yamllint soft-dependency should be removed #1735

jamesquilty opened this issue Oct 7, 2021 · 6 comments · Fixed by #1881
Labels
bug feedback-needed Divergent oppinions, additional feedback is desired in order to unblock it.
Milestone

Comments

@jamesquilty
Copy link

Summary

#953 introduced a "soft-dependency" on yamllint. While that was a well-intentioned decision, it also very unfortunately created a situation where the behaviour of the linter is unpredictable and unreliable: it depends on whether the yamllint library can be found (not present but found, see: #1722). This particularly affects teams because ansible-lint is run in a variety of environments and ansible-lint will lint YAML in only some of the environments depending on whether it can find the yamllint library. It's a pernicious situation because the absence of the library is indistinguishable from the absence of yaml lint rule violations.

This issue requests the removal of the "soft-dependency" to improve linter predictability and reliability.

Issue Type
  • Bug Report
Ansible and Ansible Lint details

Not relevant (but, see #1722).

OS / ENVIRONMENT

All.

STEPS TO REPRODUCE
  1. run ansible-lint and observe no rule violation messages
  2. Note that this situation is consistent with either:
    1. yaml files being linted and no rule violations being present, or
    2. yaml files not being linted.
Desired Behaviour

ansible-lint doesn't lint YAML; run yamllint to lint YAML.

Actual Behaviour

ansible-lint sometimes lints YAML, depending on the environment in which it is run.

In a recent case in my organisation more than 30 engineers, running macOS, Windows and Linux, attempted to use ansible-lint to enjoy the benefits of standardisation. Some of the development environments are maintained by a sysadmin, so they have limited control of environment configuration. ansible-lint behaved differently in the different environments and it was rather unfair to everyone when code was committed to the repos which contained yaml linting rule violations that the authors - through no fault of their own - were never shown.

I can understand the good intention behind attempting to retain the feature that ansible-lint checks yaml even after yamllint was removed, but the "soft-dependency" has introduced harmful unpredictability into the linter's behaviour for limited benefit. If you want ansible-lint to lint YAML then you have to install yamllint... but in that case you can independently run yamllint on your playbooks at marginal extra cost and with some benefit from additional output and more control over options and configuration.

As I wrote over in #1722, ansible-lint should either always lint YAML or never lint YAML; it should never sometimes lint YAML depending on the environment in which its run due to the problems this causes. If, as was indicated in #1722 (comment), ansible-lint can not always lint YAML then the desired behaviour should be that it never lints YAML.

@jamesquilty jamesquilty added bug new Triage required labels Oct 7, 2021
@konstruktoid
Copy link
Contributor

One option is too disable them by adding ‘yaml’ to the ‘skip_list’ (https://ansible-lint.readthedocs.io/en/latest/default_rules.html#yaml).

@jamesquilty
Copy link
Author

@konstruktoid Thanks for the quick response, and it's not the worst suggestion for working-around the problem if I was only ever developing alone...

...but I typically work with many others whose environments I have no control over and if, due to human fallibility, someone else's environment is different then YAML lint rule violations can be overlooked and committed to the repo... which then causes problems for everyone else. This isn't hypothetical, this actually happened a few weeks ago.

@ssbarnea
Copy link
Member

ssbarnea commented Oct 7, 2021

Shortly, I am in favour of making yamllint dependency permanent to avoid confusing behavior. Still, I am against removing it as its rule are very useful to the linter.

To give some insights, ansible-lint had several rules that were applying to any YAML file. We introduced yamllint integration while removing those internal rules as most people agreed that it makes no sense to have different rules for files with same extension inside the same repository.

The reason why we did not make yamllint a direct dependency was licensing, as ansible-lint codebase itself is MIT and yamllint being GPL. That would "downgrade" linter to GPL.

We already have a similar problem with ansible itself, as linter does imports from it and we do not list ansible as a dependency. Probably that is the time when we should just make linter GPL or dual GPL/MIT so we avoid a potential blocker if we manage to remove the GPL dependency in the future.

@jamesquilty
Copy link
Author

#1777 reports more confusing behaviour attributable to the inclusion of YAML linting via "soft" integration of yamllint into ansible-lint. I agree that linting of YAML files is related to ansible-lint's core purpose of linting Ansible playbooks. It seems to me that the attempted integration of yamllint is costing more in problems than any benefits it confers: integration complicates the ansible-lint code and creates confusing situations where configurations and environment have non-local effects.

I appreciate the value YAML linting adds to ansible-lint, and globally, so much so that I installed yamllint independently.

The benefits to removing YAML linting are improvements to the simplicity and locality of ansible-lint's core function and licensing, and users can be directed to yamllint as a highly recommended YAML linting tool which complements and enhances ansible-lint.

What would be the costs of making ansible-lint never lint YAML?

@ssbarnea
Copy link
Member

Shortly, yamllint integration will not go away. We will talk with the team and decide if we are to make yamllint a default dependency, resolving at least one side of the problem. Either way, you will have to add option to skip yaml checks if you do not care about these.

@ssbarnea ssbarnea added this to To do in devtools triage via automation Dec 30, 2021
@ssbarnea ssbarnea added feedback-needed Divergent oppinions, additional feedback is desired in order to unblock it. and removed new Triage required labels Jan 10, 2022
@ssbarnea
Copy link
Member

ssbarnea commented Jan 10, 2022

@ansible-community/devtools We need to discuss this in next team meeting, mostly related to GPL licensing. I am in favour of making yamllint a permanent dependency and marking distribution of ansible-lint GPL (while keeping its source-code MIT).

Feel free to up/downvote that proposal if you already have an opinion.

@ssbarnea ssbarnea added this to the 6.0.0 milestone Feb 4, 2022
ssbarnea added a commit to ssbarnea/ansible-lint that referenced this issue Feb 12, 2022
This makes the linter execution more predictable, avoiding divergence
in behavior based on yamllint presence or absence.

Fixes: ansible#1735
ssbarnea added a commit to ssbarnea/ansible-lint that referenced this issue Feb 13, 2022
This makes the linter execution more predictable, avoiding divergence
in behavior based on yamllint presence or absence.

Fixes: ansible#1735
devtools triage automation moved this from To do to Done Feb 13, 2022
@gundalow gundalow removed this from Done in devtools triage Mar 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug feedback-needed Divergent oppinions, additional feedback is desired in order to unblock it.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants