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

Refactor fqcn to recommend use of canonical names #2604

Merged
merged 1 commit into from
Oct 25, 2022
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions examples/playbooks/test_skip_inside_yaml.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions examples/roles/role_for_no_same_owner/tasks/pass.yml
Original file line number Diff line number Diff line change
@@ -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
Expand Down
19 changes: 17 additions & 2 deletions src/ansiblelint/rules/fqcn.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
Expand Down
79 changes: 55 additions & 24 deletions src/ansiblelint/rules/fqcn.py
Original file line number Diff line number Diff line change
@@ -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",
Expand Down Expand Up @@ -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 `<namespace>.<collection>.{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(
ssbarnea marked this conversation as resolved.
Show resolved Hide resolved
"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


Expand Down Expand Up @@ -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 `<namespace>.<collection>"
in results[1].message
)
assert "Use FQCN for module actions, such" in results[1].message

@pytest.mark.parametrize(
"rule_runner", (FQCNBuiltinsRule,), indirect=["rule_runner"]
Expand Down