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

Allow warnings to be skipped #2925

Merged
merged 1 commit into from
Jan 20, 2023
Merged

Allow warnings to be skipped #2925

merged 1 commit into from
Jan 20, 2023

Conversation

ssbarnea
Copy link
Member

Fixes: #2868
Closes: #2917

@ssbarnea ssbarnea requested a review from a team as a code owner January 20, 2023 16:17
@ssbarnea ssbarnea added the bug label Jan 20, 2023
@ssbarnea ssbarnea merged commit 04204dc into main Jan 20, 2023
@ssbarnea ssbarnea deleted the fix/skip branch January 20, 2023 16:56
@nkakouros
Copy link
Contributor

nkakouros commented Jan 21, 2023

@ssbarnea I don't think this is the right fix. What this PR does is not output the error about the empty-playbook if the rule is excluded. Still, warning[emtpy-playbook] remains fatal and no further checks are done, which should not be the case given that an empty playbook really is not fatal and it is also excluded. For example, given the files below:

# vars/main.yml
q: 1
# playbooks/main.yml
---

if I run ansible-lint -x 'warning[empty-playbook]' vars playbooks, I get:


Passed with production profile: 0 failure(s), 0 warning(s) on 6 files.
You are using a pre-release version of ansible-lint.

ansible-lint never reaches the point where it checks that vars file (to complain about a missing doc start).

And there is also the question if a warning should be considered as fatal error by default (I argue it should not).

@ssbarnea
Copy link
Member Author

You are confusing the special rune named "warning" with the warning level. They are two different things, and the warning meta-rule can act as a warning or error (levels).

@nkakouros
Copy link
Contributor

nkakouros commented Jan 21, 2023

There are 3 issues here and I think you would agree that there is sth at least inconsistent if not wrong. It would be great to hear your view on these 3 points.

  1. It is not me that confuses the two. The docs say:

warning is a special type of internal rule that is used to report generic runtime warnings found during execution. As stated by its name, they are not counted as errors, so they do not influence the final outcome.

So, warning rules are about warnings according to the docs and they should not be stopping further execution of other rules.

  1. Regardless of naming, do you think it is fine for this rule to be fatal and stop execution of further rules?

warning[empty-playbook] breaks further execution because it relies on ansible-playbook's --syntax-check behavior where an empty playbook is fatal. But even ansible-playbook can run an empty playbook without issue (ansible-playbook path/to/empty/playbook works fine and further playbooks in the command line are run without issue).

There is nothing really other than the above that prevents ansible-lint from continuing to check files, there is no for example bad yaml where ansible-lint will not know how to parse it. This is apparent in my PR where I modified the code to not follow ansible-playbook --check by making that warning non-breaking, but still reporting it.

  1. Especially with this PR here, if the user skips warning[empty-playbook], they may see:
Passed with production profile: 0 failure(s), 0 warning(s) on 6 files.

which is misleading. Ansible-lint has not actually checked 6 files and found them ok, it just stopped after the empty playbook and did not applied most of the rest of the rules.

@ssbarnea
Copy link
Member Author

Please create a new issue for this and we can work next week to address it. It might need multiple changes but that is not a problem, we should fix it and probably do it will also involve avoiding using "warning" as a rule name.

@nkakouros
Copy link
Contributor

I opened #2927.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Warning is fatal and stops further rule checks
3 participants