Add new rule that detects use of blind ignore_errors: true

[konstruktoid]

This PR adds a new rule name ignore-errors that will trigger if ignore_errors: true is used.

The reasoning behind this is that instead of ignoring all errors, the use of failed_when: and specified acceptable error codes to reduce the risk of ignoring important failures is recommended.

Signed-off-by: Thomas Sjögren konstruktoid@users.noreply.github.com

[ssbarnea]

I am ok with it but I am sure this will be controversial, so I so want extra eyes on it before we add it.

Is good that is added as experimental, which means it will not fail the linting, only acting as a warning, at least for the moment.

[tadeboro]

I did not look at the code too closely, but adding this rule is OK with me.

We do use ignore_error: true a lot in our integration tests because we often expect tasks to fail there. And while it would be possible to use failed_when in those cases, it would considerably decrease readability (example of such playbook: https://github.com/sensu/sensu-go-ansible/blob/c000b1f016955047976028805d8ef021773c244e/tests/integration/molecule/module_filter/converge.yml#L8-L19). If we would use ansible-lint on those playbooks, we would need to add a lot of ignores to our playbooks. But luckily we do not ;)

[dmsimard]

Like @tadeboro mentioned, there are use cases where it is relevant and desired to actually display a failure that was ignored.

I think the feature would be useful but I'm not sure I would impose it as a default.

[MarkusTeufelberger]

The "correct" version if this is failed_when: false which reads a bit more counter-intuitive to me than ignore_errors: true and I don't see the immediate benefit tbh.

To me ignore_errors: true is at most a code smell, not something that I would outright try to change or forbid ecosystem wide through a lint rule (unlike for example the "key=value" syntax for tasks).

[dmsimard]

The "correct" version if this is failed_when: false which reads a bit more counter-intuitive to me than ignore_errors: true and I don't see the immediate benefit tbh.

To me, failed_when: false basically means: "regardless of the outcome of this task, it can never fail and will always return OK"
Whereas ignore_errors: true means: "this task can fail and mark it as such but I don't want this failure to fail the entire playbook, I might even handle this failure in a subsequent task"

[ssbarnea]

I think that everyone involved in this discussion known the pros and cons of ignore errors. The question is if we can find a way to lower the number of false-positives from this rule?

• How about ignoring the rule if register exists on task? That is usually a sign that the recorded value is used later.
• How about ignoring the rule if (test|molecule) is part of the file path? That usually indicates that this is testing code, where we can assume the user probably knew.

No need to be afraid about this rule breaking existing code because is added as experimental (warning) only.

As @tadeboro said, we can assume some false-positives with any rule but it would be essential for the number of false positives to be very low as nobody likes polluting the code with ignores.

[MarkusTeufelberger]

Please don't make rules dependent on external factors such as other files/folders existing.

Having a "it makes sense in these use cases" exception might be a good hint that this is not a good universal rule to begin with.

Why should ignore_errors: true be removed from everywhere except test code in the first place? There are also other reasons why one might want/need to ignore an error happening while still having it show up as "failed" in the logs (something failed_when doesn't do).

There might be an argument for different rule sets for roles and test playbooks or some "strict mode" that ensures some certain parameters like "no failing tasks get skipped", but that would be a bigger discussion.

[ssbarnea]

Something that some of you may not know is that ignore_errors does ignore errors even if the called ansible module is crashing! I seen multiple cases where modules were called with invalid parameters for months and nobody observed due to use of ignore_errors. Ignore errors is more like a catch Exception: pass in python ;)

[konstruktoid]

Something that some of you may not know is that ignore_errors does ignore errors even if the called ansible module is crashing! I seen multiple cases where modules were called with invalid parameters for months and nobody observed due to use of ignore_errors. Ignore errors is more like a catch Exception: pass in python ;)

This basically sums up the reasoning for this rule, and it's labeled as experimental which shouldn't cause any issues in existing pipelines.

Ignoring the rule if register exists on task seems like a good next step in my opinion as well.

[alessfg]

The Ansible docs recommend using ignore_errors when using check mode to avoid potential check mode related errors https://docs.ansible.com/ansible/latest/user_guide/playbooks_checkmode.html -- Before this rule gets unlabeled as experimental, it'd be great to add an exception for that use case.

[ssbarnea]

Make a pr to do that, it makes sense and it should be really easy to do so.