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

Make use of yamllint for generic YAML issues #953

Closed
ssbarnea opened this issue Aug 13, 2020 · 1 comment · Fixed by #955
Closed

Make use of yamllint for generic YAML issues #953

ssbarnea opened this issue Aug 13, 2020 · 1 comment · Fixed by #955
Assignees
Labels
feature major Used for release notes, requires major versioning bump

Comments

@ssbarnea
Copy link
Member

Summary

Few weeks back I opened the subject on https://www.reddit.com/r/ansible/comments/hvo67m/should_ansiblelint_also_call_yamllint/ as I think our linter should not perform tasks that are already covered by YAML generic tools.

Issue Type
  • Feature Idea
Additional Information

Instead of re-implementing rules that already exist in yamllint, like below, we should be able to just call yamllint and combine its reports with our own.

While we should have our own default YAML style settings, we should also respect any in-repository .yamllint configuration.

In order to avoid adding yamllint as a permanent dependency, this feature should be implemented as optional, if yamllint module is installed, it will be used. Even in this case we should allow user to to use skips.

For example I am aware of a very useful feature in yamllint the ability to detect duplicate map keys.

@ansiblejunky
Copy link
Contributor

We should consider the overall design strategy for ansible-lint. ansible-lint needs to detect various file types that are supported by Ansible (python, yaml, jinja) and even further detect types of yaml files (playbooks, tasks, variable files, meta, handlers). Once we detect all of that, ansible-lint should allow using embedded rules (rules created and included with ansible-lint specifically to ensure ansible best practices are met).

Typically, when I need to run yamllint rules, I just add that to the CI pipeline or pre-commit configuration file and its done. I don't need ansible-lint configured to use yamllint. So from this standpoint, I'm not sure ansible-lint should use yamllint. Rather let folks use yamllint if they want to, but ansible-lint should test ansible best practices.

Having these rules inside ansible-lint has the benefit of being in our control and perhaps even being "better" suited for ansible in some way. Having them external (managed in yamllint) and using default settings still requires testing and synchronizing between ansible-lint and yamllint. In fact, as we branch out to use external linters it will also require alignment with other external tools like flask8 and jinjalint etc.

You could argue that Ansible Engine itself was "batteries included" and it caused difficulties with orchestrating releases. I think adding dependencies on yamllint, jinjalint and flask8 is more or less adding the batteries.

Perhaps using a framework that allows for the batteries to be included (if user wants to use flask8 rules, they can add that as an option). For example, ansible-lint can check a python file has some basic ansible concepts in there and if the user wants to additionally check for base rules from flask8, then perhaps its a config option in the ansible-lint config file that enables that? I just worry about having to parse those rules and display them all in the same manner and allowing false-positives in the external tool (flask8, etc) and handling that or whatnot... it might get crazy.

Just my 2 cents on the matter.. just writing my thoughts as they come.

ssbarnea added a commit that referenced this issue Dec 23, 2020
Also detect yamllint rule violations when this is found as
installed. This will allow us to remove a set of rules that
overlapped with general YAML syntax ones.

Fixes: #953
ssbarnea added a commit that referenced this issue Dec 23, 2020
Also detect yamllint rule violations when this is found as
installed. This will allow us to remove a set of rules that
overlapped with general YAML syntax ones.

Fixes: #953
ssbarnea added a commit that referenced this issue Dec 23, 2020
Also detect yamllint rule violations when this is found as
installed. This will allow us to remove a set of rules that
overlapped with general YAML syntax ones.

Fixes: #953
ssbarnea added a commit that referenced this issue Dec 25, 2020
Also detect yamllint rule violations when this is found as
installed. This will allow us to remove a set of rules that
overlapped with general YAML syntax ones.

Fixes: #953
@ssbarnea ssbarnea self-assigned this Dec 26, 2020
@ssbarnea ssbarnea added feature major Used for release notes, requires major versioning bump and removed new Triage required labels Dec 26, 2020
ssbarnea added a commit that referenced this issue Dec 27, 2020
Also detect yamllint rule violations when this is found as
installed. This will allow us to remove a set of rules that
overlapped with general YAML syntax ones.

Fixes: #953
ssbarnea added a commit that referenced this issue Dec 27, 2020
Also detect yamllint rule violations when this is found as
installed. This will allow us to remove a set of rules that
overlapped with general YAML syntax ones.

Fixes: #953
ssbarnea added a commit that referenced this issue Dec 27, 2020
Also detect yamllint rule violations when this is found as
installed. This will allow us to remove a set of rules that
overlapped with general YAML syntax ones.

Fixes: #953
ssbarnea added a commit that referenced this issue Dec 30, 2020
Also detect yamllint rule violations when this is found as
installed. This will allow us to remove a set of rules that
overlapped with general YAML syntax ones.

Fixes: #953
ssbarnea added a commit that referenced this issue Dec 30, 2020
Also detect yamllint rule violations when this is found as
installed. This will allow us to remove a set of rules that
overlapped with general YAML syntax ones.

Fixes: #953
ssbarnea added a commit that referenced this issue Dec 30, 2020
Also detect yamllint rule violations when this is found as
installed. This will allow us to remove a set of rules that
overlapped with general YAML syntax ones.

Fixes: #953
ssbarnea added a commit that referenced this issue Dec 30, 2020
Also detect yamllint rule violations when this is found as
installed. This will allow us to remove a set of rules that
overlapped with general YAML syntax ones.

Fixes: #953
ssbarnea added a commit that referenced this issue Dec 31, 2020
Also detect yamllint rule violations when this is found as
installed. This will allow us to remove a set of rules that
overlapped with general YAML syntax ones.

Fixes: #953
ssbarnea added a commit that referenced this issue Dec 31, 2020
Also detect yamllint rule violations when this is found as
installed. This will allow us to remove a set of rules that
overlapped with general YAML syntax ones.

Fixes: #953
archlinux-github pushed a commit to archlinux/infrastructure that referenced this issue Feb 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature major Used for release notes, requires major versioning bump
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants