Improve no-changed-when rule (#3050)

Co-authored-by: bluikko <[email protected]>

Author: ssbarnea

.github/workflows/tox.yml

@@ -70,7 +70,7 @@ jobs:
       WSLENV: FORCE_COLOR:PYTEST_REQPASS:TOXENV:GITHUB_STEP_SUMMARY
       # Number of expected test passes, safety measure for accidental skip of
       # tests. Update value if you add/remove tests.
-      PYTEST_REQPASS: 796
+      PYTEST_REQPASS: 791
 
     steps:
       - name: Activate WSL1

examples/playbooks/rule-no-changed-when-fail.yml

@@ -0,0 +1,15 @@
+---
+- name: Fixture for no-changed-when (fail with 3 occurrences)
+  hosts: all
+  tasks:
+    - name: Register command output, but cat still does not change anything
+      ansible.builtin.command: cat {{ my_file | quote }}
+      register: my_output
+    - name: Block level 1
+      block:
+        - name: Block level 2
+          block:
+            - name: Basic command task, should fail
+              ansible.builtin.command: cat my_file
+    - name: Basic shell task, should fail
+      shell: cat my_file # noqa: fqcn command-instead-of-shell

examples/playbooks/rule-no-changed-when-pass.yml

@@ -0,0 +1,23 @@
+---
+- name: Fixture for no-changed-when (pass)
+  hosts: all
+  tasks:
+    - name: Handle command output with return code # noqa: command-instead-of-shell
+      ansible.builtin.command: cat {{ my_file | quote }}
+      register: my_output
+      changed_when: my_output.rc != 0
+
+    - name: Handle shell output with return code # noqa: command-instead-of-shell
+      ansible.builtin.shell: cat {{ my_file | quote }}
+      register: my_output
+      changed_when: my_output.rc != 0
+
+    - name: Handle shell output with false changed_when # noqa: command-instead-of-shell
+      ansible.builtin.shell: cat {{ my_file | quote }}
+      register: my_output
+      changed_when: false
+
+    - name: Command with argument
+      command: createfile.sh # noqa: fqcn
+      args:
+        creates: /tmp/????unknown_files????

examples/playbooks/rule-no-free-form-pass.yml

@@ -16,3 +16,4 @@
       # https://github.com/ansible/ansible-lint/issues/2573
       ansible.builtin.command: localectl set-locale LANG=en_GB.UTF-8
       when: not ansible_check_mode
+      changed_when: false

src/ansiblelint/rules/no_changed_when.md

@@ -1,12 +1,20 @@
 # no-changed-when
 
-This rule checks that tasks return changes to results or conditions.
-Unless tasks only read information, you should ensure that they return changes in the following ways:
+This rule checks that tasks return changes to results or conditions. Unless
+tasks only read information, you should ensure that they return changes in the
+following ways:
 
 - Register results or conditions and use the `changed_when` clause.
 - Use the `creates` or `removes` argument.
 
-You should use the `when` clause to run tasks only if a check returns a particular result.
+You should always use the `changed_when` clause on tasks that do not naturally
+detect if a change has occurred or not. Some of the most common examples are
+[shell] and [command] modules, which run arbitrary commands.
+
+One very common workaround is to use a boolean value like `changed_when: false`
+if the task never changes anything or `changed_when: true` if it always
+changes something, but you can also use any expressions, including ones that
+use the registered result of a task, like in our example below.
 
 ## Problematic Code
 
@@ -31,3 +39,8 @@ You should use the `when` clause to run tasks only if a check returns a particul
       register: my_output # <- Registers the command output.
       changed_when: my_output.rc != 0 # <- Uses the return code to define when the task has changed.
 ```
+
+[shell]:
+  https://docs.ansible.com/ansible/latest/collections/ansible/builtin/shell_module.html
+[command]:
+  https://docs.ansible.com/ansible/latest/collections/ansible/builtin/command_module.html

src/ansiblelint/rules/no_changed_when.py

@@ -24,6 +24,7 @@
 import sys
 from typing import TYPE_CHECKING, Any
 
+from ansiblelint.errors import MatchError
 from ansiblelint.rules import AnsibleLintRule
 
 if TYPE_CHECKING:
@@ -34,168 +35,60 @@ class CommandHasChangesCheckRule(AnsibleLintRule):
     """Commands should not change things if nothing needs doing."""
 
     id = "no-changed-when"
-    description = """
-Tasks should tell Ansible when to return ``changed``, unless the task only reads
-information. To do this, set ``changed_when``, use the ``creates`` or
-``removes`` argument, or use ``when`` to run the task only if another check has
-a particular result.
-
-For example, this task registers the ``shell`` output and uses the return code
-to define when the task has changed.
-
-```yaml
-    - name: Handle shell output with return code
-      ansible.builtin.shell: cat {{ my_file|quote }}
-      register: my_output
-      changed_when: my_output.rc != 0
-```
-
-The following example will trigger the rule since the task does not
-handle the output of the ``command``.
-
-```yaml
-    - name: Does not handle any output or return codes
-      ansible.builtin.command: cat {{ my_file|quote }}
-```
-    """
     severity = "HIGH"
     tags = ["command-shell", "idempotency"]
     version_added = "historic"
 
-    _commands = ["command", "shell", "raw"]
+    _commands = [
+        "ansible.builtin.command",
+        "ansible.builtin.shell",
+        "ansible.builtin.raw",
+        "ansible.legacy.command",
+        "ansible.legacy.shell",
+        "ansible.legacy.raw",
+        "command",
+        "shell",
+        "raw",
+    ]
 
     def matchtask(
         self, task: dict[str, Any], file: Lintable | None = None
-    ) -> bool | str:
+    ) -> list[MatchError]:
+        result = []
         # tasks in a block are "meta" type
         if task["__ansible_action_type__"] in ["task", "meta"]:
-            if task["action"]["__ansible_module__"] in self._commands:
-                return (
-                    "changed_when" not in task
-                    and "when" not in task
-                    and "creates" not in task["action"]
-                    and "removes" not in task["action"]
-                )
-        return False
+            if task["action"]["__ansible_module__"] in self._commands and (
+                "changed_when" not in task
+                and "creates" not in task["action"]
+                and "removes" not in task["action"]
+            ):
+                result.append(self.create_matcherror(filename=file))
+        return result
 
 
 if "pytest" in sys.modules:
     import pytest
 
-    NO_CHANGE_COMMAND_RC = """
-- hosts: all
-  tasks:
-    - name: Handle command output with return code
-      ansible.builtin.command: cat {{ my_file|quote }}
-      register: my_output
-      changed_when: my_output.rc != 0
-"""
-
-    NO_CHANGE_SHELL_RC = """
-- hosts: all
-  tasks:
-    - name: Handle shell output with return code
-      ansible.builtin.shell: cat {{ my_file|quote }}
-      register: my_output
-      changed_when: my_output.rc != 0
-"""
-
-    NO_CHANGE_SHELL_FALSE = """
-- hosts: all
-  tasks:
-    - name: Handle shell output with false changed_when
-      ansible.builtin.shell: cat {{ my_file|quote }}
-      register: my_output
-      changed_when: false
-"""
-
-    NO_CHANGE_ARGS = """
-- hosts: all
-  tasks:
-    - name: Command with argument
-      command: createfile.sh
-      args:
-        creates: /tmp/????unknown_files????
-"""
-
-    NO_CHANGE_REGISTER_FAIL = """
-- hosts: all
-  tasks:
-    - name: Register command output, but cat still does not change anything
-      ansible.builtin.command: cat {{ my_file|quote }}
-      register: my_output
-"""
-
-    # also test to ensure it catches tasks in nested blocks.
-    NO_CHANGE_COMMAND_FAIL = """
-- hosts: all
-  tasks:
-    - block:
-        - block:
-            - name: Basic command task, should fail
-              ansible.builtin.command: cat my_file
-"""
-
-    NO_CHANGE_SHELL_FAIL = """
-- hosts: all
-  tasks:
-    - name: Basic shell task, should fail
-      shell: cat my_file
-"""
-
-    @pytest.mark.parametrize(
-        "rule_runner", (CommandHasChangesCheckRule,), indirect=["rule_runner"]
-    )
-    def test_no_change_command_rc(rule_runner: Any) -> None:
-        """This should pass since ``*_when`` is used."""
-        results = rule_runner.run_playbook(NO_CHANGE_COMMAND_RC)
-        assert len(results) == 0
-
-    @pytest.mark.parametrize(
-        "rule_runner", (CommandHasChangesCheckRule,), indirect=["rule_runner"]
-    )
-    def test_no_change_shell_rc(rule_runner: Any) -> None:
-        """This should pass since ``*_when`` is used."""
-        results = rule_runner.run_playbook(NO_CHANGE_SHELL_RC)
-        assert len(results) == 0
-
-    @pytest.mark.parametrize(
-        "rule_runner", (CommandHasChangesCheckRule,), indirect=["rule_runner"]
-    )
-    def test_no_change_shell_false(rule_runner: Any) -> None:
-        """This should pass since ``*_when`` is used."""
-        results = rule_runner.run_playbook(NO_CHANGE_SHELL_FALSE)
-        assert len(results) == 0
-
-    @pytest.mark.parametrize(
-        "rule_runner", (CommandHasChangesCheckRule,), indirect=["rule_runner"]
-    )
-    def test_no_change_args(rule_runner: Any) -> None:
-        """This test should not pass since the command doesn't do anything."""
-        results = rule_runner.run_playbook(NO_CHANGE_ARGS)
-        assert len(results) == 0
-
-    @pytest.mark.parametrize(
-        "rule_runner", (CommandHasChangesCheckRule,), indirect=["rule_runner"]
-    )
-    def test_no_change_register_fail(rule_runner: Any) -> None:
-        """This test should not pass since cat still doesn't do anything."""
-        results = rule_runner.run_playbook(NO_CHANGE_REGISTER_FAIL)
-        assert len(results) == 1
-
-    @pytest.mark.parametrize(
-        "rule_runner", (CommandHasChangesCheckRule,), indirect=["rule_runner"]
-    )
-    def test_no_change_command_fail(rule_runner: Any) -> None:
-        """This test should fail because command isn't handled."""
-        # this also ensures that this catches tasks in nested blocks
-        results = rule_runner.run_playbook(NO_CHANGE_COMMAND_FAIL)
-        assert len(results) == 1
+    from ansiblelint.rules import RulesCollection  # pylint: disable=ungrouped-imports
+    from ansiblelint.runner import Runner  # pylint: disable=ungrouped-imports
 
     @pytest.mark.parametrize(
-        "rule_runner", (CommandHasChangesCheckRule,), indirect=["rule_runner"]
+        ("file", "expected"),
+        (
+            pytest.param(
+                "examples/playbooks/rule-no-changed-when-pass.yml", 0, id="pass"
+            ),
+            pytest.param(
+                "examples/playbooks/rule-no-changed-when-fail.yml", 3, id="fail"
+            ),
+        ),
     )
-    def test_no_change_shell_fail(rule_runner: Any) -> None:
-        """This test should fail because shell isn't handled.."""
-        results = rule_runner.run_playbook(NO_CHANGE_SHELL_FAIL)
-        assert len(results) == 1
+    def test_rule_no_changed_when(
+        default_rules_collection: RulesCollection, file: str, expected: int
+    ) -> None:
+        """Validate no-changed-when rule."""
+        results = Runner(file, rules=default_rules_collection).run()
+
+        for result in results:
+            assert result.rule.id == CommandHasChangesCheckRule.id, result
+        assert len(results) == expected