Add ability to auto-fix fcqn rule violations (#3316)

Signed-off-by: Markus Teufelberger <[email protected]>
Co-authored-by: Markus Teufelberger <[email protected]>

.config/dictionary.txt

@@ -174,6 +174,7 @@ jsonfile
 jsonschema
 junitxml
 keepends
+keypair
 keyserver
 konstruktoid
 kubernetes

.github/workflows/tox.yml

@@ -59,7 +59,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: 794
+      PYTEST_REQPASS: 795
     steps:
       - name: Activate WSL1
         if: "contains(matrix.shell, 'wsl')"

examples/playbooks/fqcn.transformed.yml

@@ -0,0 +1,16 @@
+---
+- name: FQCN transform test file
+  hosts: localhost
+  tasks:
+    - name: Rewrite shell to ansible.builtin.shell via the fqcn[action-core] transform # noqa: command-instead-of-shell
+      ansible.builtin.shell: echo This rule should get matched by the fqcn[action-core] rule
+      changed_when: false
+    - name: Rewrite openssh_keypair to community.crypto.openssh_keypair via the fqcn[action] transform
+      community.crypto.openssh_keypair:
+        path: /tmp/supersecret
+    - name: Rewrite ansible.builtin.synchronize to ansible.posix.synchronize via the fqcn[canonical] transform
+      ansible.posix.synchronize:
+        src: dummy
+        dest: dummy2
+        owner: false
+        group: false

examples/playbooks/fqcn.yml

@@ -0,0 +1,16 @@
+---
+- name: FQCN transform test file
+  hosts: localhost
+  tasks:
+    - name: Rewrite shell to ansible.builtin.shell via the fqcn[action-core] transform # noqa: command-instead-of-shell
+      shell: echo This rule should get matched by the fqcn[action-core] rule
+      changed_when: false
+    - name: Rewrite openssh_keypair to community.crypto.openssh_keypair via the fqcn[action] transform
+      openssh_keypair:
+        path: /tmp/supersecret
+    - name: Rewrite ansible.builtin.synchronize to ansible.posix.synchronize via the fqcn[canonical] transform
+      ansible.builtin.synchronize:
+        src: dummy
+        dest: dummy2
+        owner: false
+        group: false

src/ansiblelint/rules/fqcn.py

@@ -3,14 +3,18 @@
 
 import logging
 import sys
-from typing import Any
+from typing import TYPE_CHECKING, Any
 
 from ansible.plugins.loader import module_loader
 
 from ansiblelint.constants import LINE_NUMBER_KEY
 from ansiblelint.errors import MatchError
-from ansiblelint.file_utils import Lintable
-from ansiblelint.rules import AnsibleLintRule, RulesCollection
+from ansiblelint.rules import AnsibleLintRule, TransformMixin
+
+if TYPE_CHECKING:
+    from ruamel.yaml.comments import CommentedMap, CommentedSeq
+
+    from ansiblelint.file_utils import Lintable  # noqa: F811
 
 _logger = logging.getLogger(__name__)
 
@@ -86,7 +90,7 @@
 ]
 
 
-class FQCNBuiltinsRule(AnsibleLintRule):
+class FQCNBuiltinsRule(AnsibleLintRule, TransformMixin):
     """Use FQCN for builtin actions."""
 
     id = "fqcn"
@@ -176,9 +180,37 @@ def matchplay(self, file: Lintable, data: dict[str, Any]) -> list[MatchError]:
             ]
         return []
 
+    def transform(
+        self,
+        match: MatchError,
+        lintable: Lintable,
+        data: CommentedMap | CommentedSeq | str,
+    ) -> None:
+        if match.tag in {"fqcn[action-core]", "fqcn[action]", "fqcn[canonical]"}:
+            target_task = self.seek(match.yaml_path, data)
+            # Unfortunately, a lot of data about Ansible content gets lost here, you only get a simple dict.
+            # For now, just parse the error messages for the data about action names etc. and fix this later.
+            if match.tag == "fqcn[action-core]":
+                # split at the first bracket, cut off the last bracket and dot
+                current_action = match.message.split("(")[1][:-2]
+                # This will always replace builtin modules with "ansible.builtin" versions, not "ansible.legacy".
+                # The latter is technically more correct in what ansible has executed so far, the former is most likely better understood and more robust.
+                new_action = match.details.split("`")[1]
+            elif match.tag == "fqcn[action]":
+                current_action = match.details.split("`")[1]
+                new_action = match.message.split("`")[1]
+            elif match.tag == "fqcn[canonical]":
+                current_action = match.message.split("`")[3]
+                new_action = match.message.split("`")[1]
+            for _ in range(len(target_task)):
+                k, v = target_task.popitem(False)
+                target_task[new_action if k == current_action else k] = v
+            match.fixed = True
+
 
 # testing code to be loaded only with pytest or when executed the rule file
 if "pytest" in sys.modules:
+    from ansiblelint.rules import RulesCollection
     from ansiblelint.runner import Runner  # pylint: disable=ungrouped-imports
 
     def test_fqcn_builtin_fail() -> None:

test/test_transformer.py

@@ -78,6 +78,7 @@ def fixture_runner_result(
         ),
         pytest.param("examples/playbooks/vars/strings.yml", 0, True, id="strings"),
         pytest.param("examples/playbooks/name-case.yml", 1, True, id="name_case"),
+        pytest.param("examples/playbooks/fqcn.yml", 3, True, id="fqcn"),
     ),
 )
 def test_transformer(  # pylint: disable=too-many-arguments, too-many-locals

tools/install-reqs.sh

@@ -3,7 +3,7 @@ set -euo pipefail
 pushd examples/playbooks/collections >/dev/null
 MISSING=()
 export ANSIBLE_COLLECTIONS_PATH=.
-for COLLECTION in ansible.posix community.docker community.general community.molecule ansible.windows;
+for COLLECTION in ansible.posix community.docker community.general community.molecule ansible.windows community.crypto;
 do
     COL_NAME=${COLLECTION//\./-}
     FILENAME=$(find . -maxdepth 1 -name "$COL_NAME*" -print -quit)