From f3a3fe486336fb1c391a83205826fe638968471e Mon Sep 17 00:00:00 2001 From: Sorin Sbarnea Date: Tue, 25 Oct 2022 19:13:51 +0100 Subject: [PATCH] Refactor fqcn to recommend use of canonical names (#2604) Closs: #2572 --- examples/playbooks/test_skip_inside_yaml.yml | 4 +- .../role_for_no_same_owner/tasks/pass.yml | 4 +- src/ansiblelint/rules/fqcn.md | 19 ++++- src/ansiblelint/rules/fqcn.py | 79 +++++++++++++------ 4 files changed, 76 insertions(+), 30 deletions(-) diff --git a/examples/playbooks/test_skip_inside_yaml.yml b/examples/playbooks/test_skip_inside_yaml.yml index f413a36694..f60f197ba0 100644 --- a/examples/playbooks/test_skip_inside_yaml.yml +++ b/examples/playbooks/test_skip_inside_yaml.yml @@ -5,8 +5,8 @@ - skip_ansible_lint # should disable error at playbook level tasks: - name: Test # <-- 0 latest[hg] - action: ansible.builtin.hg - - name: Test latest[hg] (skipped) # noqa latest[hg] + action: ansible.builtin.hg # noqa fqcn[canonical] + - name: Test latest[hg] (skipped) # noqa latest[hg] fqcn[canonical] action: ansible.builtin.hg - name: Test latest[git] and partial-become diff --git a/examples/roles/role_for_no_same_owner/tasks/pass.yml b/examples/roles/role_for_no_same_owner/tasks/pass.yml index 6151dab469..03e27fc7ea 100644 --- a/examples/roles/role_for_no_same_owner/tasks/pass.yml +++ b/examples/roles/role_for_no_same_owner/tasks/pass.yml @@ -1,12 +1,12 @@ --- - name: Synchronize-delegate - ansible.builtin.synchronize: + ansible.posix.synchronize: src: dummy dest: dummy delegate_to: localhost - name: Synchronize-no-same-owner - ansible.builtin.synchronize: + ansible.posix.synchronize: src: dummy dest: dummy owner: false diff --git a/src/ansiblelint/rules/fqcn.md b/src/ansiblelint/rules/fqcn.md index 4d1cd5812d..2d81668eb4 100644 --- a/src/ansiblelint/rules/fqcn.md +++ b/src/ansiblelint/rules/fqcn.md @@ -7,9 +7,9 @@ This avoids ambiguity and conflicts that can cause operations to fail or produce The `fqcn` rule has the following checks: -- `fqcn[action]` - Checks all actions for FQCNs. +- `fqcn[action]` - Use FQCN for module actions, such ... - `fqcn[action-core]` - Checks for FQCNs from the `ansible.legacy` or `ansible.builtin` collection. -- `fqcn[action-redirect]` - Provides the correct FQCN to replace short actions. +- `fqcn[canonical]` - You should use canonical module name ... instead of ... ```{note} In most cases you should declare the `ansible.builtin` collection for internal Ansible actions. @@ -22,6 +22,21 @@ The `collections` keyword provided a temporary mechanism transitioning to Ansibl You should rewrite any content that uses the `collections:` key and avoid it where possible. ``` +## Canonical module names + +Canonical module names are also known as **resolved module names** and they +are to be preferred for most cases. Many Ansible modules have multiple aliases +and redirects, as these were created over time while the content was refactored. +Still, all of them do finally resolve to the same module name, but not without +adding some performance overhead. As very old aliases are at some point removed, +it makes to just refresh the content to make it point to the current canonical +name. + +The only exception for using a canonical name is if your code still needs to +be compatible with a very old version of Ansible, one that does not know how +to resolve that name. If you find yourself in such a situation, feel free to +add this rule to the ignored list. + ## Problematic Code ```yaml diff --git a/src/ansiblelint/rules/fqcn.py b/src/ansiblelint/rules/fqcn.py index 1a7ba060af..443f0cb7bf 100644 --- a/src/ansiblelint/rules/fqcn.py +++ b/src/ansiblelint/rules/fqcn.py @@ -1,13 +1,18 @@ """Rule definition for usage of fully qualified collection names for builtins.""" from __future__ import annotations +import logging import sys from typing import Any +from ansible.plugins.loader import module_loader + from ansiblelint.errors import MatchError from ansiblelint.file_utils import Lintable from ansiblelint.rules import AnsibleLintRule +_logger = logging.getLogger(__name__) + builtins = [ # spell-checker:disable "add_host", @@ -92,33 +97,62 @@ class FQCNBuiltinsRule(AnsibleLintRule): ) tags = ["formatting"] version_added = "v6.8.0" + module_aliases: dict[str, str] = {"block/always/rescue": "block/always/rescue"} def matchtask( self, task: dict[str, Any], file: Lintable | None = None ) -> list[MatchError]: result = [] module = task["action"]["__ansible_module_original__"] - if module in builtins: - result.append( - self.create_matcherror( - message=f"Use FQCN for builtin module actions ({module}).", - details=f"Use `ansible.builtin.{module}` or `ansible.legacy.{module}` instead.", - filename=file, - linenumber=task["__line__"], - tag="fqcn[action-core]", - ) - ) - # Add here implementation for fqcn[action-redirect] - elif module != "block/always/rescue" and module.count(".") < 2: - result.append( - self.create_matcherror( - message=f"Use FQCN for module actions, such `..{module}`.", - details=f"Action `{module}` is not FQCN.", - filename=file, - linenumber=task["__line__"], - tag="fqcn[action]", + + if module not in self.module_aliases: + loaded_module = module_loader.find_plugin_with_context(module) + target = loaded_module.resolved_fqcn + self.module_aliases[module] = target + if target is None: + _logger.warning("Unable to resolve FQCN for module %s", module) + self.module_aliases[module] = module + return [] + if target not in self.module_aliases: + self.module_aliases[target] = target + + if module != self.module_aliases[module]: + module_alias = self.module_aliases.get(module, "") + if module_alias.startswith("ansible.builtin"): + result.append( + self.create_matcherror( + message=f"Use FQCN for builtin module actions ({module}).", + details=f"Use `ansible.builtin.{module}` or `ansible.legacy.{module}` instead.", + filename=file, + linenumber=task["__line__"], + tag="fqcn[action-core]", + ) ) - ) + else: + if module.count(".") < 2: + result.append( + self.create_matcherror( + message=f"Use FQCN for module actions, such `{self.module_aliases[module]}`.", + details=f"Action `{module}` is not FQCN.", + filename=file, + linenumber=task["__line__"], + tag="fqcn[action]", + ) + ) + # TODO(ssbarnea): Remove the c.g. and c.n. exceptions from here once + # community team is flattening these. + # See: https://github.com/ansible-community/community-topics/issues/147 + elif not module.startswith("community.general.") or module.startswith( + "community.network." + ): + result.append( + self.create_matcherror( + message=f"You should use canonical module name `{self.module_aliases[module]}` instead of `{module}`.", + filename=file, + linenumber=task["__line__"], + tag="fqcn[canonical]", + ) + ) return result @@ -160,10 +194,7 @@ def test_fqcn_builtin_fail(rule_runner: RunFromText) -> None: assert results[0].tag == "fqcn[action-core]" assert "Use FQCN for builtin module actions" in results[0].message assert results[1].tag == "fqcn[action]" - assert ( - "Use FQCN for module actions, such `." - in results[1].message - ) + assert "Use FQCN for module actions, such" in results[1].message @pytest.mark.parametrize( "rule_runner", (FQCNBuiltinsRule,), indirect=["rule_runner"]