From e4de1cdfcbed65a105aeba1f9fbb904973dd4bea Mon Sep 17 00:00:00 2001 From: Don Naro Date: Fri, 23 Sep 2022 21:10:45 +0100 Subject: [PATCH] Refactor no-loop-var-prefix rule (#2470) * docs lint rule no-loop-var * chore: auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Refactor no-loop-var-prefix - rename rule to loop-var-prefix and keep alias for old name - make rule produce more detailed messages - add documentation to the rule Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Sorin Sbarnea --- .../roles/fail_loop_var_prefix/tasks/main.yml | 24 -------- examples/roles/loop_var_prefix/tasks/fail.yml | 4 +- examples/roles/loop_var_prefix/tasks/pass.yml | 2 +- src/ansiblelint/constants.py | 1 + src/ansiblelint/rules/loop_var_prefix.md | 58 +++++++++++++++++++ ..._loop_var_prefix.py => loop_var_prefix.py} | 37 ++++++++---- .../schemas/ansible-lint-config.json | 2 +- 7 files changed, 90 insertions(+), 38 deletions(-) delete mode 100644 examples/roles/fail_loop_var_prefix/tasks/main.yml create mode 100644 src/ansiblelint/rules/loop_var_prefix.md rename src/ansiblelint/rules/{no_loop_var_prefix.py => loop_var_prefix.py} (69%) diff --git a/examples/roles/fail_loop_var_prefix/tasks/main.yml b/examples/roles/fail_loop_var_prefix/tasks/main.yml deleted file mode 100644 index eb1f552d48..0000000000 --- a/examples/roles/fail_loop_var_prefix/tasks/main.yml +++ /dev/null @@ -1,24 +0,0 @@ ---- -# 3 expected no-loop-var-prefix failures at 2, 8, 18 -- name: That should trigger no-loop-var-prefix - debug: - var: item - loop: - - foo - - bar -- name: That should fail due to wrong custom - debug: - var: zz_item - loop: - - foo - - bar - loop_control: - loop_var: zz_item -- name: Using a block - block: - - name: That should also not pass - debug: - var: item - loop: - - apples - - oranges diff --git a/examples/roles/loop_var_prefix/tasks/fail.yml b/examples/roles/loop_var_prefix/tasks/fail.yml index 4fa26bf305..be0dc9a211 100644 --- a/examples/roles/loop_var_prefix/tasks/fail.yml +++ b/examples/roles/loop_var_prefix/tasks/fail.yml @@ -1,6 +1,6 @@ --- -# 5 expected no-loop-var-prefix failures at 3, 9, 19, 26, 33 -- name: That should trigger no-loop-var-prefix +# 5 expected loop-var-prefix failures at 3, 9, 19, 26, 33 +- name: That should trigger loop-var-prefix ansible.builtin.debug: var: item loop: diff --git a/examples/roles/loop_var_prefix/tasks/pass.yml b/examples/roles/loop_var_prefix/tasks/pass.yml index 527668e907..19b7f3040e 100644 --- a/examples/roles/loop_var_prefix/tasks/pass.yml +++ b/examples/roles/loop_var_prefix/tasks/pass.yml @@ -1,5 +1,5 @@ --- -# 0 expected no-loop-var-prefix failures +# 0 expected loop-var-prefix failures - name: That should pass ansible.builtin.debug: var: loop_var_prefix_item diff --git a/src/ansiblelint/constants.py b/src/ansiblelint/constants.py index 5a830f9061..85e3d5dd7c 100644 --- a/src/ansiblelint/constants.py +++ b/src/ansiblelint/constants.py @@ -102,6 +102,7 @@ def main(): "git-latest": "latest[git]", "hg-latest": "latest[hg]", "no-jinja-nesting": "jinja[invalid]", + "no-loop-var-prefix": "loop-var-prefix", } PLAYBOOK_TASK_KEYWORDS = [ diff --git a/src/ansiblelint/rules/loop_var_prefix.md b/src/ansiblelint/rules/loop_var_prefix.md new file mode 100644 index 0000000000..9445ceda3f --- /dev/null +++ b/src/ansiblelint/rules/loop_var_prefix.md @@ -0,0 +1,58 @@ +# loop-var-prefix + +This rule avoids conflicts with nested looping tasks by configuring a variable prefix with `loop_var`. +Ansible sets `item` as the loop variable. +You can use `loop_var` to specify a prefix for loop variables and ensure they are unique to each task. + +This rule can produce the following messages: + +- `[loop-var-prefix[missing]` - Replace unsafe implicit `item` loop variable by adding `loop_var: ...`. +- `[loop-var-prefix[wrong]` - Loop variable should start with + +This is an opt-in rule. +You must enable it in your Ansible-lint configuration as follows: + +```yaml +enable_list: + - loop-var-prefix +``` + +## Problematic Code + +```yaml +--- +- name: Example playbook + hosts: localhost + tasks: + - name: Does not set a prefix for loop variables. + ansible.builtin.debug: + var: item + loop: + - foo + - bar # <- These items do not have a unique prefix. + - name: Sets + ansible.builtin.debug: + var: zz_item + loop: + - foo + - bar + loop_control: + loop_var: zz_item # <- This prefix is not unique. +``` + +## Correct Code + +```yaml +--- +- name: Example playbook + hosts: localhost + tasks: + - name: Sets a unique prefix for loop variables. + ansible.builtin.debug: + var: zz_item + loop: + - foo + - bar + loop_control: + loop_var: my_prefix # <- Specifies a unique prefix for loop variables. +``` diff --git a/src/ansiblelint/rules/no_loop_var_prefix.py b/src/ansiblelint/rules/loop_var_prefix.py similarity index 69% rename from src/ansiblelint/rules/no_loop_var_prefix.py rename to src/ansiblelint/rules/loop_var_prefix.py index e84778d14c..a5c7f3bf62 100644 --- a/src/ansiblelint/rules/no_loop_var_prefix.py +++ b/src/ansiblelint/rules/loop_var_prefix.py @@ -5,6 +5,7 @@ from typing import TYPE_CHECKING, Any from ansiblelint.config import options +from ansiblelint.errors import MatchError from ansiblelint.rules import AnsibleLintRule from ansiblelint.text import toidentifier @@ -15,7 +16,7 @@ class RoleLoopVarPrefix(AnsibleLintRule): """Role loop_var should use configured prefix.""" - id = "no-loop-var-prefix" + id = "loop-var-prefix" link = ( "https://docs.ansible.com/ansible/latest/user_guide/" "playbooks_loops.html#defining-inner-and-outer-variable-names-with-loop-var" @@ -30,12 +31,13 @@ class RoleLoopVarPrefix(AnsibleLintRule): def matchtask( self, task: dict[str, Any], file: Lintable | None = None - ) -> bool | str: + ) -> list[MatchError]: """Return matches for a task.""" if not file or not file.role or not options.loop_var_prefix: - return False + return [] self.prefix = options.loop_var_prefix.format(role=toidentifier(file.role)) + # self.prefix becomes `loop_var_prefix_` because role name is loop_var_prefix has_loop = "loop" in task for key in task.keys(): @@ -46,10 +48,25 @@ def matchtask( loop_control = task.get("loop_control", {}) loop_var = loop_control.get("loop_var", "") - if not loop_var or not loop_var.startswith(self.prefix): - return True - - return False + if loop_var: + if not loop_var.startswith(self.prefix): + return [ + self.create_matcherror( + message=f"Loop variable should start with {self.prefix}", + filename=file, + tag="loop-var-prefix[wrong]", + ) + ] + else: + return [ + self.create_matcherror( + message=f"Replace unsafe implicit `item` loop variable by adding `loop_var: {self.prefix}...`.", + filename=file, + tag="loop-var-prefix[missing]", + ) + ] + + return [] # testing code to be loaded only with pytest or when executed the rule file @@ -71,13 +88,13 @@ def matchtask( ), ), ) - def test_no_loop_var_prefix( + def test_loop_var_prefix( default_rules_collection: RulesCollection, test_file: str, failures: int ) -> None: """Test rule matches.""" # Enable checking of loop variable prefixes in roles options.loop_var_prefix = "{role}_" results = Runner(test_file, rules=default_rules_collection).run() - assert len(results) == failures for result in results: - assert result.message == RoleLoopVarPrefix().shortdesc + assert result.rule.id == RoleLoopVarPrefix().id + assert len(results) == failures diff --git a/src/ansiblelint/schemas/ansible-lint-config.json b/src/ansiblelint/schemas/ansible-lint-config.json index 2f0b50a8cd..1b9d4ccf20 100644 --- a/src/ansiblelint/schemas/ansible-lint-config.json +++ b/src/ansiblelint/schemas/ansible-lint-config.json @@ -112,7 +112,7 @@ "no-handler", "no-jinja-when", "no-log-password", - "no-loop-var-prefix", + "loop-var-prefix", "no-prompting", "no-relative-paths", "no-same-owner",