Skip to content

Commit

Permalink
Upgrade fqcn-builtins rule into fqcn
Browse files Browse the repository at this point in the history
- rename tag from fqcn-builtins to fqcn[builtin]
- add alias for old name
- update documentation
- add fqcn[module] for those that are not using known builtins.
  • Loading branch information
ssbarnea committed Sep 27, 2022
1 parent 2792ba0 commit 45842d5
Show file tree
Hide file tree
Showing 13 changed files with 69 additions and 30 deletions.
2 changes: 1 addition & 1 deletion examples/playbooks/custom_module.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,5 @@
tags:
- "{{ foo }}"
tasks:
- name: Run custom module
- name: Run custom module # noqa: fqcn[module]
fake_module: {}
4 changes: 2 additions & 2 deletions examples/playbooks/nomatchestest.transformed.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
hosts: whatever
tasks:
- name: Hello world
action: debug msg="Hello!" # noqa: fqcn-builtins
action: debug msg="Hello!" # noqa: fqcn[builtin]

- name: This should be fine too
action: file state=touch dest=./wherever mode=0600 # noqa: fqcn-builtins
action: file state=touch dest=./wherever mode=0600 # noqa: fqcn[builtin]
4 changes: 2 additions & 2 deletions examples/playbooks/nomatchestest.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
hosts: whatever
tasks:
- name: Hello world
action: debug msg="Hello!" # noqa: fqcn-builtins
action: debug msg="Hello!" # noqa: fqcn[builtin]

- name: This should be fine too
action: file state=touch dest=./wherever mode=0600 # noqa: fqcn-builtins
action: file state=touch dest=./wherever mode=0600 # noqa: fqcn[builtin]
2 changes: 1 addition & 1 deletion examples/playbooks/rule-risky-file-permissions-fail.yml
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@
hosts: all
tasks:
- name: Permissions missing
# noqa: fqcn-builtins
# noqa: fqcn[builtin]
get_url:
url: http://foo
dest: foo
Expand Down
1 change: 1 addition & 0 deletions src/ansiblelint/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ def main():
"hg-latest": "latest[hg]",
"no-jinja-nesting": "jinja[invalid]",
"no-loop-var-prefix": "loop-var-prefix",
"fqcn-builtins": "fqcn[builtin]",
}

PLAYBOOK_TASK_KEYWORDS = [
Expand Down
2 changes: 1 addition & 1 deletion src/ansiblelint/data/profiles.yml
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ production:
url: https://github.com/ansible/ansible-lint/issues/2174
disallowed-ignore: # [sanity]
url: https://github.com/ansible/ansible-lint/issues/2121
fqcn-builtins:
fqcn:
import-task-no-when:
url: https://github.com/ansible/ansible-lint/issues/2219
meta-no-dependencies:
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,19 @@
# fqcn-builtins
# fqcn

This rule checks that playbooks use the fully qualified collection name (FQCN) for modules in the `ansible.builtin` collection.
This rule checks that playbooks use the fully qualified collection name (FQCN)
for all including builtins and 3rd party ones.

If you do not specify the FQCN, Ansible uses the `ansible.legacy` collection for some modules by default.
You can use local overrides with the `ansible.legacy` collection but not with the `ansible.builtin` collection.

Keep in mind that this rule disregards values of `collections:` key as these
were mostly used as a transitional mechanism for Ansible 2.9.

This rule can generate the following error messages:

- `fqcn[builtin]` - Use the FQCN for the module known as being builtin.
- `fqcn[module]` - Use FQCN for any modules, to avoid accidents.

## Problematic Code

```yaml
Expand All @@ -20,7 +29,7 @@ You can use local overrides with the `ansible.legacy` collection but not with th

```yaml
---
- name: Example playbook
- name: Example playbook (1st solution)
hosts: all
tasks:
- name: Create an SSH connection
Expand All @@ -29,7 +38,7 @@ You can use local overrides with the `ansible.legacy` collection but not with th

```yaml
---
- name: Example playbook
- name: Example playbook (2nd solution)
hosts: all
tasks:
- name: Create an SSH connection
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import sys
from typing import Any

from ansiblelint.errors import MatchError
from ansiblelint.file_utils import Lintable
from ansiblelint.rules import AnsibleLintRule

Expand Down Expand Up @@ -84,18 +85,40 @@
class FQCNBuiltinsRule(AnsibleLintRule):
"""Use FQCN for builtin actions."""

id = "fqcn-builtins"
id = "fqcn"
severity = "MEDIUM"
description = (
"Check whether the long version starting with ``ansible.builtin`` "
"is used in the playbook"
"Check whether modules are called using full qualified collection name."
)
tags = ["formatting"]
version_added = "v6.8.0"

def matchtask(
self, task: dict[str, Any], file: Lintable | None = None
) -> bool | str:
return task["action"]["__ansible_module_original__"] in builtins
) -> list[MatchError]:
result = []
module = task["action"]["__ansible_module_original__"]
if module in builtins:
result.append(
self.create_matcherror(
message="Use FQCN for builtin actions.",
details=f"Use `ansible.builtin.{module}` or `ansible.legacy.{module}` instead.",
filename=file,
linenumber=task["__line__"],
tag="fqcn[builtin]",
)
)
elif module != "block/always/rescue" and module.count(".") != 2:
result.append(
self.create_matcherror(
message="Use FQCN for module calls (actions), such `namespace.collection_name.module_name`.",
details=f"Action `{module}` is not FQCN.",
filename=file,
linenumber=task["__line__"],
tag="fqcn[module]",
)
)
return result


# testing code to be loaded only with pytest or when executed the rule file
Expand All @@ -109,14 +132,17 @@ def matchtask(
- hosts: localhost
tasks:
- name: Shell (fqcn)
ansible.builtin.shell: echo This rule should not get matched by the fqcn-builtins rule
ansible.builtin.shell: echo This rule should not get matched by the fqcn[builtin] rule
"""

FAIL_PLAY = """
- hosts: localhost
tasks:
- name: Shell (fqcn)
shell: echo This rule should get matched by the fqcn-builtins rule
- name: Shell (fqcn[builtin])
shell: echo This rule should get matched by the fqcn[builtin] rule
- name: Shell (fqcn[module])
ini_file:
path: /tmp/test.ini
"""

@pytest.mark.parametrize(
Expand All @@ -125,9 +151,11 @@ def matchtask(
def test_fqcn_builtin_fail(rule_runner: RunFromText) -> None:
"""Test rule matches."""
results = rule_runner.run_playbook(FAIL_PLAY)
assert len(results) == 1
for result in results:
assert result.message == FQCNBuiltinsRule().shortdesc
assert len(results) == 2
assert results[0].tag == "fqcn[builtin]"
assert "Use FQCN for builtin actions" in results[0].message
assert results[1].tag == "fqcn[module]"
assert "Use FQCN for module calls" in results[1].message

@pytest.mark.parametrize(
"rule_runner", (FQCNBuiltinsRule,), indirect=["rule_runner"]
Expand Down
2 changes: 1 addition & 1 deletion src/ansiblelint/rules/only_builtins.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
from ansiblelint.rules import AnsibleLintRule

# fqcn_builtins was added in 5.1.0 as FQCNBuiltinsRule, renamed to fqcn_builtins in 6.0.0
from ansiblelint.rules.fqcn_builtins import builtins
from ansiblelint.rules.fqcn import builtins
from ansiblelint.skip_utils import is_nested_task


Expand Down
3 changes: 2 additions & 1 deletion src/ansiblelint/schemas/ansible-lint-config.json
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,8 @@
"deprecated-local-action",
"deprecated-module",
"empty-string-compare",
"fqcn-builtins",
"fqcn",
"fqcn[builtin]",
"latest",
"galaxy-collection-version",
"ignore-errors",
Expand Down
4 changes: 2 additions & 2 deletions test/test_eco.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,10 @@ def test_eco(repo: str) -> None:
# run ansible lint and paths from user home in order to produce
# consistent results regardless on its location.

# we exclude `fqcn-builtins` until repository owners fix it.
# we exclude `fqcn[builtin]` until repository owners fix it.
for step in ["before", "after"]:

args = ["-f", "pep8", "-x", "fqcn-builtins"]
args = ["-f", "pep8", "-x", "fqcn[builtin]"]
executable = (
"ansible-lint"
if step == "after"
Expand Down
6 changes: 3 additions & 3 deletions test/test_file_path_evaluation.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
"""\
---
- name: From subtask 2 do something
debug: # <-- expected to raise fqcn-builtins
debug: # <-- expected to raise fqcn[builtin]
msg: |
Something...
"""
Expand Down Expand Up @@ -88,7 +88,7 @@
"""\
---
- name: From subtask 2 do something
debug: # <-- expected to raise fqcn-builtins
debug: # <-- expected to raise fqcn[builtin]
msg: |
Something...
"""
Expand Down Expand Up @@ -121,4 +121,4 @@ def test_file_path_evaluation(
result = Runner(str(tmp_path), rules=default_rules_collection).run()

assert len(result) == 1
assert result[0].rule.id == "fqcn-builtins"
assert result[0].rule.id == "fqcn"
2 changes: 1 addition & 1 deletion test/test_import_include_role.py
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ def test_import_role2(
) -> None:
"""Test that include_role digs deeper than import_role."""
runner = Runner(
playbook_path, rules=default_rules_collection, skip_list=["fqcn-builtins"]
playbook_path, rules=default_rules_collection, skip_list=["fqcn[builtin]"]
)
results = runner.run()
for message in messages:
Expand Down

0 comments on commit 45842d5

Please sign in to comment.