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

Upgrade fqcn-builtins rule into fqcn #2505

Merged
merged 3 commits into from
Sep 29, 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
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[action]
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[action-core]

- 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[action-core]
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[action-core]

- 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[action-core]
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[action-core]
get_url:
url: http://foo
dest: foo
Expand Down
2 changes: 2 additions & 0 deletions src/ansiblelint/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
"name[casing]",
"name[play]",
"role-name",
"fqcn[action]",
"fqcn[redirect]",
]

DEFAULT_KINDS = [
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[action-core]",
}

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
56 changes: 56 additions & 0 deletions src/ansiblelint/rules/fqcn.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
# fqcn

This rule checks for fully-qualified collection names (FQCN) in Ansible content.

Declaring an FQCN ensures that an action uses code from the correct namespace.
This avoids ambiguity and conflicts that can cause operations to fail or produce unexpected results.

The `fqcn` rule has the following checks:

- `fqcn[action]` - Checks all actions for FQCNs.
- `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.

```{note}
In most cases you should declare the `ansible.builtin` collection for internal Ansible actions.
You should declare the `ansible.legacy` collection if you use local overrides with actions, such with as the ``shell`` module.
```

```{warning}
This rule does not take [`collections` keyword](https://docs.ansible.com/ansible/latest/user_guide/collections_using.html#simplifying-module-names-with-the-collections-keyword) into consideration.
The `collections` keyword provided a temporary mechanism transitioning to Ansible 2.9.
You should rewrite any content that uses the `collections:` key and avoid it where possible.
```

## Problematic Code

```yaml
---
- name: Example playbook
hosts: all
tasks:
- name: Create an SSH connection
shell: ssh ssh_user@{{ ansible_ssh_host }} # <- Does not use the FQCN for the shell module.
```

## Correct Code

```yaml
---
- name: Example playbook (1st solution)
hosts: all
tasks:
- name: Create an SSH connection
# Use the FQCN for the legacy shell module and allow local overrides.
ansible.legacy.shell: ssh ssh_user@{{ ansible_ssh_host }} -o IdentityFile=path/to/my_rsa
```

```yaml
---
- name: Example playbook (2nd solution)
hosts: all
tasks:
- name: Create an SSH connection
# Use the FQCN for the builtin shell module.
ansible.builtin.shell: ssh ssh_user@{{ ansible_ssh_host }}
```
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,41 @@
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 actions are using using full qualified collection names."
)
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=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]",
)
)
return result


# testing code to be loaded only with pytest or when executed the rule file
Expand All @@ -109,14 +133,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 rule
"""

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

@pytest.mark.parametrize(
Expand All @@ -125,9 +152,14 @@ 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[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
)

@pytest.mark.parametrize(
"rule_runner", (FQCNBuiltinsRule,), indirect=["rule_runner"]
Expand Down
37 changes: 0 additions & 37 deletions src/ansiblelint/rules/fqcn_builtins.md

This file was deleted.

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
5 changes: 4 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,10 @@
"deprecated-local-action",
"deprecated-module",
"empty-string-compare",
"fqcn-builtins",
"fqcn",
"fqcn[action-core]",
"fqcn[action-redirect]",
"fqcn[action]",
"latest",
"galaxy-collection-version",
"ignore-errors",
Expand Down
4 changes: 1 addition & 3 deletions test/test_eco.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,9 @@ 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.
for step in ["before", "after"]:

args = ["-f", "pep8", "-x", "fqcn-builtins"]
args = ["-f", "pep8"]
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[action-core]
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[action-core]
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"
4 changes: 3 additions & 1 deletion test/test_import_include_role.py
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,9 @@ 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[action-core]"],
)
results = runner.run()
for message in messages:
Expand Down