make rule deprecated-local-action preserve module parameters (#4733)

Co-authored-by: Bradley A. Thornton <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Alison Hart <[email protected]>

Author: 4 people

examples/playbooks/tasks/local_action.transformed.yml

@@ -1,4 +1,36 @@
 ---
-- name: Sample
-  ansible.builtin.command: echo 123
+- name: Sample 1
+  command: echo 123
   delegate_to: localhost
+- name: Sample 2
+  command:
+    cmd: echo 456
+  delegate_to: localhost
+- name: Sample 3
+  ansible.builtin.debug:
+  delegate_to: localhost
+- name: Sample 4 - Dict-form local_action
+  user:
+    name: alice
+    state: present
+  delegate_to: localhost
+  vars:
+    foo: bar
+- name: Sample 5 - String-form local_action (all key=value)
+  file: path=/tmp/foo state=touch mode=0644
+  delegate_to: localhost
+  tags: always
+- name: Sample 6 - String-form local_action (positional arguments)
+  shell: echo "hello world"
+  delegate_to: localhost
+- name: Sample 7 - String-form local_action (single positional argument)
+  command: hostname
+  delegate_to: localhost
+- name: Sample 8 - Dict-form local_action with no params (only module)
+  ping:
+  delegate_to: localhost
+- name: Sample 9 - Args copied over
+  copy: src=/etc/hosts dest=/tmp/hosts
+  delegate_to: localhost
+  args:
+    remote_src: true

examples/playbooks/tasks/local_action.yml

@@ -1,3 +1,30 @@
 ---
-- name: Sample
+- name: Sample 1
   local_action: command echo 123
+- name: Sample 2
+  local_action:
+    module: command
+    cmd: echo 456
+- name: Sample 3
+  local_action: ansible.builtin.debug
+- name: Sample 4 - Dict-form local_action
+  local_action:
+    module: user
+    name: alice
+    state: present
+  vars:
+    foo: bar
+- name: Sample 5 - String-form local_action (all key=value)
+  local_action: file path=/tmp/foo state=touch mode=0644
+  tags: always
+- name: Sample 6 - String-form local_action (positional arguments)
+  local_action: shell echo "hello world"
+- name: Sample 7 - String-form local_action (single positional argument)
+  local_action: command hostname
+- name: Sample 8 - Dict-form local_action with no params (only module)
+  local_action:
+    module: ping
+- name: Sample 9 - Args copied over
+  local_action: copy src=/etc/hosts dest=/tmp/hosts
+  args:
+    remote_src: yes

src/ansiblelint/rules/deprecated_local_action.md

@@ -16,7 +16,7 @@ This rule recommends using `delegate_to: localhost` instead of the
 
 ```yaml
 - name: Task example
-    ansible.builtin.debug:
+  ansible.builtin.debug:
   delegate_to: localhost # <-- recommended way to run on localhost
 ```
 

src/ansiblelint/rules/deprecated_local_action.py

@@ -4,29 +4,37 @@
 # Copyright (c) 2018, Ansible Project
 from __future__ import annotations
 
-import copy
 import logging
-import os
 import sys
 from pathlib import Path
-from typing import TYPE_CHECKING
+from typing import TYPE_CHECKING, Any
 
+from ruamel.yaml.comments import CommentedMap
+
+from ansiblelint.errors import MatchError
+from ansiblelint.file_utils import Lintable
 from ansiblelint.rules import AnsibleLintRule, TransformMixin
 from ansiblelint.runner import get_matches
 from ansiblelint.transformer import Transformer
 
 if TYPE_CHECKING:
-    from ruamel.yaml.comments import CommentedMap, CommentedSeq
+    from ruamel.yaml.comments import CommentedSeq
 
     from ansiblelint.config import Options
-    from ansiblelint.errors import MatchError
-    from ansiblelint.file_utils import Lintable
     from ansiblelint.utils import Task
 
 
 _logger = logging.getLogger(__name__)
 
 
+class LocalActionTransformError(Exception):
+    """Exception raised when a local_action is not processed correctly."""
+
+    def __init__(self, message: str) -> None:
+        """Initialize the LocalActionTransformError with a message."""
+        _logger.error(message)
+
+
 class TaskNoLocalActionRule(AnsibleLintRule, TransformMixin):
     """Do not use 'local_action', use 'delegate_to: localhost'."""
 
@@ -52,37 +60,108 @@ def transform(
         lintable: Lintable,
         data: CommentedMap | CommentedSeq | str,
     ) -> None:
-        if match.tag == self.id:
-            # we do not want perform a partial modification accidentally
-            original_target_task = self.seek(match.yaml_path, data)
-            target_task = copy.deepcopy(original_target_task)
-            for _ in range(len(target_task)):
-                k, v = target_task.popitem(False)
-                if k == "local_action":
-                    if isinstance(v, dict):
-                        module_name = v["module"]
-                        target_task[module_name] = None
-                        target_task["delegate_to"] = "localhost"
-                    elif isinstance(v, str):
-                        module_name, module_value = v.split(" ", 1)
-                        target_task[module_name] = module_value
-                        target_task["delegate_to"] = "localhost"
-                    else:  # pragma: no cover
-                        _logger.debug(
-                            "Ignored unexpected data inside %s transform.",
-                            self.id,
-                        )
-                        return
+        """Transform the task to use delegate_to: localhost.
+
+        Args:
+            match: The match object.
+            lintable: The lintable object.
+            data: The data to transform.
+        """
+        try:
+            self.perform_transform(match, lintable, data)
+        except LocalActionTransformError as e:
+            match.fixed = False
+            match.message = str(e)
+            return
+
+    def perform_transform(
+        self,
+        match: MatchError,
+        lintable: Lintable,
+        data: CommentedMap | CommentedSeq | str,
+    ) -> None:
+        """Transform the task to use delegate_to: localhost.
+
+        Args:
+            match: The match object.
+            lintable: The lintable object.
+            data: The data to transform.
+
+        Raises:
+            LocalActionTransformError: If the local_action is not dict | str.
+        """
+        original_task = self.seek(match.yaml_path, data)
+        task_location = f"{lintable.name}:{match.lineno}"
+
+        target_task = {}
+
+        for k, v in original_task.items():
+            if k == "local_action":
+                if isinstance(v, dict):
+                    target_task.update(self.process_dict(v, task_location))
+                elif isinstance(v, str):
+                    target_task.update(self.process_string(v, task_location))
                 else:
-                    target_task[k] = v
+                    err = f"Unsupported local_action type '{type(v).__name__}' in task at {task_location}"
+                    raise LocalActionTransformError(err)
+                target_task["delegate_to"] = "localhost"
+            else:
+                target_task[k] = v
+
         match.fixed = True
-        original_target_task.clear()
-        original_target_task.update(target_task)
+        original_task.clear()
+        original_task.update(target_task)
+
+    def process_dict(
+        self, local_action: dict[str, Any], task_location: str
+    ) -> dict[str, Any]:
+        """Process a dict-form local_action.
+
+        Args:
+            local_action: The local_action dictionary.
+            task_location: The location of the task.
+
+        Returns:
+            A dictionary with the module and parameters.
+
+        Raises:
+            LocalActionTransformError: If the local_action dictionary is missing a 'module' key.
+        """
+        if "module" not in local_action:
+            err = f"No 'module' key in local_action in task at {task_location}"
+            raise LocalActionTransformError(err)
+        return {
+            local_action["module"]: {
+                k: v for k, v in local_action.items() if k != "module"
+            }
+            or None
+        }
+
+    def process_string(
+        self, local_action: str, task_location: str
+    ) -> dict[str, str | None]:
+        """Process a string-form local_action.
+
+        Args:
+            local_action: The local_action string.
+            task_location: The location of the task.
+
+        Returns:
+            A dictionary with the module and parameters.
+
+        Raises:
+            LocalActionTransformError: If the local_action string is empty or whitespace.
+        """
+        if not local_action or not local_action.strip():
+            err = f"Empty local_action string in task at {task_location}"
+            raise LocalActionTransformError(err)
+        parts = local_action.split(" ", 1)
+        return {parts[0]: parts[1] if len(parts) > 1 else None}
 
 
 # testing code to be loaded only with pytest or when executed the rule file
 if "pytest" in sys.modules:
-    from unittest import mock
+    import pytest
 
     from ansiblelint.rules import RulesCollection
     from ansiblelint.runner import Runner
@@ -94,27 +173,80 @@ def test_local_action(default_rules_collection: RulesCollection) -> None:
             rules=default_rules_collection,
         ).run()
 
-        assert len(results) == 1
-        assert results[0].tag == "deprecated-local-action"
+        assert any(result.tag == "deprecated-local-action" for result in results)
+
+    @pytest.mark.parametrize(
+        ("data", "prefix"),
+        (
+            (
+                CommentedMap({"local_action": True}),
+                "Unsupported local_action type 'bool'",
+            ),
+            (
+                CommentedMap({"local_action": 123}),
+                "Unsupported local_action type 'int'",
+            ),
+            (
+                CommentedMap({"local_action": 12.34}),
+                "Unsupported local_action type 'float'",
+            ),
+            (
+                CommentedMap({"local_action": []}),
+                "Unsupported local_action type 'list'",
+            ),
+            (CommentedMap({"local_action": {}}), "No 'module' key in local_action"),
+            (CommentedMap({"local_action": ""}), "Empty local_action string"),
+            (CommentedMap({"local_action": "  "}), "Empty local_action string"),
+        ),
+        ids=[
+            "bool",
+            "int",
+            "float",
+            "list",
+            "empty_dict",
+            "empty_string",
+            "whitespace_string",
+        ],
+    )
+    def test_local_action_transform_fail(
+        caplog: pytest.LogCaptureFixture, data: CommentedMap, prefix: str
+    ) -> None:
+        """Test transform functionality for a failure.
+
+        Args:
+            caplog: The pytest fixture for capturing logs.
+            data: The data to test.
+            prefix: The expected error prefix.
+        """
+        file = "site.yml"
+        rule = TaskNoLocalActionRule()
+        lintable = Lintable(name=file)
+        match_error = MatchError(message="error", lintable=lintable, lineno=1)
+        with pytest.raises(LocalActionTransformError):
+            rule.perform_transform(match_error, lintable, data)
+        assert f"{prefix} in task at {file}:1" in caplog.text
 
-    @mock.patch.dict(os.environ, {"ANSIBLE_LINT_WRITE_TMP": "1"}, clear=True)
     def test_local_action_transform(
         config_options: Options,
-        default_rules_collection: RulesCollection,
+        monkeypatch: pytest.MonkeyPatch,
     ) -> None:
         """Test transform functionality for no-log-password rule."""
+        monkeypatch.setenv("ANSIBLE_LINT_WRITE_TMP", "1")
+
         playbook = Path("examples/playbooks/tasks/local_action.yml")
         config_options.write_list = ["all"]
 
         config_options.lintables = [str(playbook)]
+        only_local_action_rule: RulesCollection = RulesCollection()
+        only_local_action_rule.register(TaskNoLocalActionRule())
         runner_result = get_matches(
-            rules=default_rules_collection,
+            rules=only_local_action_rule,
             options=config_options,
         )
         transformer = Transformer(result=runner_result, options=config_options)
         transformer.run()
         matches = runner_result.matches
-        assert len(matches) == 3
+        assert any(error.tag == "deprecated-local-action" for error in matches)
 
         orig_content = playbook.read_text(encoding="utf-8")
         expected_content = playbook.with_suffix(
@@ -126,4 +258,5 @@ def test_local_action_transform(
 
         assert orig_content != transformed_content
         assert expected_content == transformed_content
+
         playbook.with_suffix(f".tmp{playbook.suffix}").unlink()

src/ansiblelint/schemas/__store__.json

@@ -56,7 +56,7 @@
     "url": "https://raw.githubusercontent.com/ansible/ansible-lint/main/src/ansiblelint/schemas/role-arg-spec.json"
   },
   "rulebook": {
-    "etag": "586088ca8247a8ba635f31d41d265f5309f3b690583eaa7dbd8a045f36eddfa9",
+    "etag": "9f785b5986cdbfe3b4ba911cbb65263df150bb595b22e4f191081b9360717d33",
     "url": "https://raw.githubusercontent.com/ansible/ansible-rulebook/main/ansible_rulebook/schema/ruleset_schema.json"
   },
   "tasks": {

src/ansiblelint/schemas/rulebook.json

@@ -481,6 +481,12 @@
                         },
                         "lock": {
                             "type": "string"
+                        },
+                        "labels": {
+                            "type": "array",
+                            "items": {
+                                "type": "string"
+                            }
                         }
                     },
                     "required": [
@@ -537,6 +543,12 @@
                         },
                         "lock": {
                             "type": "string"
+                        },
+                        "labels": {
+                            "type": "array",
+                            "items": {
+                                "type": "string"
+                            }
                         }
                     },
                     "required": [