Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability to auto-fix fcqn rule violations #3316

Merged
merged 2 commits into from
Apr 21, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .config/dictionary.txt
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,7 @@ jsonfile
jsonschema
junitxml
keepends
keypair
keyserver
konstruktoid
kubernetes
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/tox.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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')"
Expand Down
16 changes: 16 additions & 0 deletions examples/playbooks/fqcn.transformed.yml
Original file line number Diff line number Diff line change
@@ -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
16 changes: 16 additions & 0 deletions examples/playbooks/fqcn.yml
Original file line number Diff line number Diff line change
@@ -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
40 changes: 36 additions & 4 deletions src/ansiblelint/rules/fqcn.py
Original file line number Diff line number Diff line change
Expand Up @@ -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__)

Expand Down Expand Up @@ -86,7 +90,7 @@
]


class FQCNBuiltinsRule(AnsibleLintRule):
class FQCNBuiltinsRule(AnsibleLintRule, TransformMixin):
"""Use FQCN for builtin actions."""

id = "fqcn"
Expand Down Expand Up @@ -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:
Expand Down
1 change: 1 addition & 0 deletions test/test_transformer.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion tools/install-reqs.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down