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 ansible-lint and yamllint use more strict rules. #400

Merged
merged 17 commits into from Sep 30, 2021

Conversation

rjeffman
Copy link
Member

@rjeffman rjeffman commented Sep 21, 2020

This patch modifies configuration of both ansible-lint and yamllint
to check for more rules, resulting in a more strict verification.

The rules left disable are either necessary due to missing Ansible
modules, or might impact modules or roles execution or testing.

@rjeffman rjeffman marked this pull request as draft September 21, 2020 23:01
@rjeffman rjeffman changed the title Make ansible-lint and yamllint use more strict rules. [WIP} Make ansible-lint and yamllint use more strict rules. Sep 23, 2020
@rjeffman rjeffman changed the title [WIP} Make ansible-lint and yamllint use more strict rules. [WIP] Make ansible-lint and yamllint use more strict rules. Sep 23, 2020
@rjeffman rjeffman force-pushed the lint_fix_ansible_lint_issues branch 2 times, most recently from 2ccd4d6 to 18f134f Compare September 23, 2020 19:34
@rjeffman rjeffman force-pushed the lint_fix_ansible_lint_issues branch 3 times, most recently from 6416524 to c02511d Compare November 26, 2020 00:06
@rjeffman rjeffman removed the blocked label Nov 26, 2020
@rjeffman rjeffman force-pushed the lint_fix_ansible_lint_issues branch 3 times, most recently from 7194e7a to 4d968e9 Compare November 27, 2020 11:39
@rjeffman rjeffman force-pushed the lint_fix_ansible_lint_issues branch from 13f9d13 to 55abf8c Compare May 4, 2021 13:00
@rjeffman rjeffman removed the blocked label May 4, 2021
@rjeffman rjeffman requested a review from t-woerner May 4, 2021 13:00
@rjeffman rjeffman changed the title [WIP] Make ansible-lint and yamllint use more strict rules. Make ansible-lint and yamllint use more strict rules. May 4, 2021
@rjeffman rjeffman marked this pull request as ready for review May 4, 2021 13:01
@rjeffman rjeffman force-pushed the lint_fix_ansible_lint_issues branch 2 times, most recently from 4415c24 to 32235f1 Compare May 27, 2021 23:04
@rjeffman
Copy link
Member Author

@t-woerner, please, can you review this PR?

@rjeffman rjeffman force-pushed the lint_fix_ansible_lint_issues branch 4 times, most recently from 99f027e to ac4643b Compare June 1, 2021 11:19
@rjeffman rjeffman force-pushed the lint_fix_ansible_lint_issues branch 2 times, most recently from 96c693d to fbc8d26 Compare July 8, 2021 17:34
.ansible-lint Outdated Show resolved Hide resolved
.ansible-lint Outdated
- '305' # Use shell only when shell functionality is required
- '306' # risky-shell-pipe
- yaml # yamllint should be executed separately.
- experimental # Ignore rules that resulted in false positives
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is experimental exactly?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

experimental varies from release to release. It used to give lots of false positive, but is much more stable in the latest versions.

I enabled experimental and fixed the warnings for ansible-lint 5.0.7 using ansible 2.9.25.

ipaadmin_password: SomeADMINpassword
name: host01.exmaple.com
name: host01.example.com
managedby_host: server.exmaple.com
Copy link
Member

@t-woerner t-woerner Sep 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a lot of "exmaple" in the playbooks folder. Let's fix this with another PR.

utils/lint_check.sh Outdated Show resolved Hide resolved
This patch adds 'name' to all test playbook tasks that did not
have it, fixing ansible-lint's error 'unnamed-task'.
This patch adds 'name' to all example playbook tasks that did not
have it, fixing ansible-lint's error 'unnamed-task'.
This patch fixes yamllint's "line too long" (line-lenght) warnings
by ensuring all lines in YAML files have, at most, 160 characters.

If a line cannot be written as a multiline block, line-length rule
evaluation is disabled for the specific line, both on yamllint and
on ansible-lint.
Comments in YAML files should be aligned to content.
This patch modifies configuration of both ansible-lint and yamllint
to check for more rules, resulting in a more strict verification.

For ansible-lint verification of errors 301, 305 and 505 are skipped,
due to false positives. For the same reason, 'experimental' rules
are skipped.

ansible-lint error 306 is skipped since the fix is to set pipefail,
which is not available in all shells (for example dash, which runs
ansible-freeipa CI).

Yamllint disabled rules (comments, and indentation) would introduce a
huge amount of small changes, and are left for future changes, it
deemed necessary.
Some tests for ipahost and ipauser modules, related to certificates
had the verification part disabled. This patch enable these
verifications.
These playbooks manage the certificates of a user, but did not have
the proper action for it.
As of September, 2021, Ansible-lint cannot evaluate task files which
included through `include_tasks`, as it fails syntax-check.

This change exclude evaluation of these files (`env_*`) when evaluating
files before commit (pre-commit).
Copy link
Member

@t-woerner t-woerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@t-woerner t-woerner merged commit b434c5f into freeipa:master Sep 30, 2021
@rjeffman rjeffman deleted the lint_fix_ansible_lint_issues branch February 23, 2022 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants