Make sure all tasks get evaluated by matchtask including block/always/rescue and nested tasks

.github/workflows/tox.yml

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

src/ansiblelint/rules/unnamed_task.py

@@ -23,6 +23,7 @@
 from typing import TYPE_CHECKING, Any, Dict, Union
 
 from ansiblelint.rules import AnsibleLintRule
+from ansiblelint.utils import is_nested_task
 
 if TYPE_CHECKING:
     from typing import Optional
@@ -45,4 +46,5 @@ class TaskHasNameRule(AnsibleLintRule):
     def matchtask(
         self, task: Dict[str, Any], file: "Optional[Lintable]" = None
     ) -> Union[bool, str]:
-        return not task.get("name")
+        # Ignore nonamed block/always/rescue
+        return not (task.get("name") or is_nested_task(task))

src/ansiblelint/rules/var_naming.py

@@ -17,7 +17,7 @@
     from ansiblelint.errors import MatchError
 
 
-# Should raise var-naming at line [4, 8].
+# Should raise var-naming at line [4, 8, 18, 20].
 FAIL_PLAY = """---
 - hosts: localhost
   vars:
@@ -35,6 +35,16 @@
     - name: bar
       ansible.builtin.set_fact:
         CamelCaseButErrorIgnored: true  # noqa: var-naming
+    - name: test in a block
+      block:
+        - name:
+          ansible.builtin.set_fact:
+            CamelCaseIsBad: "{{ BAD }}" # invalid
+          vars:
+            ALL_CAPS_ARE_BAD_TOO: "{{ MoreBad }}" # invalid
+      vars:
+        BAD: false # invalid
+        MoreBad: ... # invalid
 """
 
 
@@ -189,12 +199,12 @@ def matchyaml(self, file: Lintable) -> List["MatchError"]:
     def test_invalid_var_name_playbook(rule_runner: RunFromText) -> None:
         """Test rule matches."""
         results = rule_runner.run_playbook(FAIL_PLAY)
-        assert len(results) == 2
+        assert len(results) == 4
         for result in results:
             assert result.rule.id == VariableNamingRule.id
 
         # list unexpected error lines or non-matching error lines
-        expected_error_lines = [4, 8]
+        expected_error_lines = [4, 8, 18, 20]
         lines = [i.linenumber for i in results]
         error_lines_difference = list(
             set(expected_error_lines).symmetric_difference(set(lines))

src/ansiblelint/utils.py

@@ -521,12 +521,39 @@ def _sanitize_task(task: Dict[str, Any]) -> Dict[str, Any]:
     return result
 
 
+def _extract_ansible_parsed_keys_from_task(
+    result: Dict[str, Any],
+    task: Dict[str, Any],
+    keys: Tuple[str, ...],
+) -> Dict[str, Any]:
+    """Return a dict with existing key in task."""
+    for (k, v) in list(task.items()):
+        if k in keys:
+            # we don't want to re-assign these values, which were
+            # determined by the ModuleArgsParser() above
+            continue
+        result[k] = v
+    return result
+
+
 def normalize_task_v2(task: Dict[str, Any]) -> Dict[str, Any]:
     """Ensure tasks have a normalized action key and strings are converted to python objects."""
-    result = {}
+    result: Dict[str, Any] = {}
+    ansible_parsed_keys = ("action", "local_action", "args", "delegate_to")
+
+    if is_nested_task(task):
+        _extract_ansible_parsed_keys_from_task(result, task, ansible_parsed_keys)
+        # Add dummy action for block/always/rescue statements
+        result["action"] = dict(
+            __ansible_module__="block/always/rescue",
+            __ansible_module_original__="block/always/rescue",
+        )
+
+        return result
 
     sanitized_task = _sanitize_task(task)
     mod_arg_parser = ModuleArgsParser(sanitized_task)
+
     try:
         action, arguments, result["delegate_to"] = mod_arg_parser.parse(
             skip_action_validation=options.skip_action_validation
@@ -545,12 +572,9 @@ def normalize_task_v2(task: Dict[str, Any]) -> Dict[str, Any]:
         action = "shell"
         del arguments["_uses_shell"]
 
-    for (k, v) in list(task.items()):
-        if k in ("action", "local_action", "args", "delegate_to") or k == action:
-            # we don't want to re-assign these values, which were
-            # determined by the ModuleArgsParser() above
-            continue
-        result[k] = v
+    _extract_ansible_parsed_keys_from_task(
+        result, task, ansible_parsed_keys + (action,)
+    )
 
     if not isinstance(action, str):
         raise RuntimeError(f"Task actions can only be strings, got {action}")
@@ -660,8 +684,6 @@ def get_action_tasks(data: AnsibleBaseYAMLObject, file: Lintable) -> List[Any]:
 
     # Add sub-elements of block/rescue/always to tasks list
     tasks.extend(extract_from_list(tasks, NESTED_TASK_KEYS, recursive=True))
-    # Remove block/rescue/always elements from tasks list
-    tasks[:] = [task for task in tasks if all(k not in task for k in NESTED_TASK_KEYS)]
 
     # Include the FQCN task names as this happens before normalize
     return [
@@ -875,3 +897,12 @@ def nested_items(
             yield "list-item", item, parent
             for k, v, returned_parent in nested_items(item):
                 yield k, v, returned_parent
+
+
+def is_nested_task(task: Dict[str, Any]) -> bool:
+    """Check if task includes block/always/rescue."""
+    for key in NESTED_TASK_KEYS:
+        if task.get(key):
+            return True
+
+    return False

test/eco/colsystem.result

@@ -3,11 +3,12 @@ CMD: ansible-lint -f pep8 -x fqcn-builtins
 RC: 2
 
 STDERR:
-WARNING  Listing 4 violation(s) that are fatal
+WARNING  Listing 5 violation(s) that are fatal
 You can skip specific rules or tags by adding them to your configuration file:
 # .config/ansible-lint.yml
 warn_list:  # or 'skip_list' to silence them completely
   - load-failure  # Failed to load or parse file.
+  - no-handler  # Tasks that run when changed should likely be handlers.
   - yaml  # Violations reported by yamllint.
 
 
@@ -17,3 +18,4 @@ STDOUT:
 playbooks/molecule/sudo/molecule.yml:17: yaml: line too long (576 > 160 characters) (line-length)
 roles/firewalld/defaults/main.yml:11: yaml: missing starting space in comment (comments)
 roles/firewalld/defaults/main.yml:28: yaml: missing starting space in comment (comments)
+roles/packages_workstation/tasks/Linux/06_blu_ray.yml:18: no-handler: Tasks that run when changed should likely be handlers.

test/test_utils.py

@@ -34,6 +34,7 @@
 from _pytest.capture import CaptureFixture
 from _pytest.logging import LogCaptureFixture
 from _pytest.monkeypatch import MonkeyPatch
+from ansible.utils.sentinel import Sentinel
 
 from ansiblelint import cli, constants, utils
 from ansiblelint.__main__ import initialize_logger
@@ -130,6 +131,59 @@ def test_normalize_complex_command() -> None:
     )
 
 
+@pytest.mark.parametrize(
+    ("task", "expected_form"),
+    (
+        pytest.param(
+            dict(
+                name="ensure apache is at the latest version",
+                yum={"name": "httpd", "state": "latest"},
+            ),
+            dict(
+                delegate_to=Sentinel,
+                name="ensure apache is at the latest version",
+                action={
+                    "__ansible_module__": "yum",
+                    "__ansible_module_original__": "yum",
+                    "__ansible_arguments__": [],
+                    "name": "httpd",
+                    "state": "latest",
+                },
+            ),
+        ),
+        pytest.param(
+            dict(
+                name="Attempt and graceful roll back",
+                block=[
+                    {
+                        "name": "Install httpd and memcached",
+                        "ansible.builtin.yum": ["httpd", "memcached"],
+                        "state": "present",
+                    }
+                ],
+            ),
+            dict(
+                name="Attempt and graceful roll back",
+                block=[
+                    {
+                        "name": "Install httpd and memcached",
+                        "ansible.builtin.yum": ["httpd", "memcached"],
+                        "state": "present",
+                    }
+                ],
+                action={
+                    "__ansible_module__": "block/always/rescue",
+                    "__ansible_module_original__": "block/always/rescue",
+                },
+            ),
+        ),
+    ),
+)
+def test_normalize_task_v2(task: Dict[str, Any], expected_form: Dict[str, Any]) -> None:
+    """Check that it normalizes task and returns the expected form."""
+    assert utils.normalize_task_v2(task) == expected_form
+
+
 def test_extract_from_list() -> None:
     """Check that tasks get extracted from blocks if present."""
     block = {
@@ -353,3 +407,41 @@ def test_nested_items() -> None:
         match=r"Call to deprecated function ansiblelint\.utils\.nested_items.*"
     ):
         assert list(utils.nested_items(data)) == items
+
+
+@pytest.mark.parametrize(
+    ("task", "expected"),
+    (
+        pytest.param(
+            dict(
+                name="ensure apache is at the latest version",
+                yum={"name": "httpd", "state": "latest"},
+            ),
+            False,
+        ),
+        pytest.param(
+            dict(
+                name="Attempt and graceful roll back",
+                block=[
+                    {"name": "Force a failure", "ansible.builtin.command": "/bin/false"}
+                ],
+                rescue=[
+                    {
+                        "name": "Force a failure in middle of recovery!",
+                        "ansible.builtin.command": "/bin/false",
+                    }
+                ],
+                always=[
+                    {
+                        "name": "Always do this",
+                        "ansible.builtin.debug": {"msg": "This always executes"},
+                    }
+                ],
+            ),
+            True,
+        ),
+    ),
+)
+def test_is_nested_task(task: Dict[str, Any], expected: bool) -> None:
+    """Test is_nested_task() returns expected bool."""
+    assert utils.is_nested_task(task) == expected