diff --git a/.ansible-lint b/.ansible-lint index 82eaf9145e4..6c7f6d1af81 100644 --- a/.ansible-lint +++ b/.ansible-lint @@ -4,3 +4,5 @@ mock_modules: - zuul_return mock_roles: - mocked_role +# Enable checking of loop variable prefixes in roles +loop_var_prefix: "{role}_" diff --git a/examples/playbooks/pass-loop-var-prefix.yml b/examples/playbooks/pass-loop-var-prefix.yml new file mode 100644 index 00000000000..3f0df6feb57 --- /dev/null +++ b/examples/playbooks/pass-loop-var-prefix.yml @@ -0,0 +1,17 @@ +- hosts: localhost + tasks: + # validate that we did not trigger loop-var-prefix on playbooks + - name: that should pass + debug: + var: item + loop: + - foo + - bar + - name: a block + block: + - name: that should also pass + debug: + var: item + loop: + - apples + - oranges diff --git a/examples/roles/fail_loop_var_prefix/tasks/main.yml b/examples/roles/fail_loop_var_prefix/tasks/main.yml new file mode 100644 index 00000000000..dc4b85c7a2b --- /dev/null +++ b/examples/roles/fail_loop_var_prefix/tasks/main.yml @@ -0,0 +1,23 @@ +# 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/src/ansiblelint/cli.py b/src/ansiblelint/cli.py index 41f3d512f30..1d884c481b7 100644 --- a/src/ansiblelint/cli.py +++ b/src/ansiblelint/cli.py @@ -208,7 +208,11 @@ def merge_config(file_config, cli_config: Namespace) -> Namespace: 'tags': [], 'warn_list': ['experimental'], 'mock_modules': [], - 'mock_roles': [] + 'mock_roles': [], + } + + scalar_map = { + "loop_var_prefix": None } if not file_config: @@ -223,6 +227,10 @@ def merge_config(file_config, cli_config: Namespace) -> Namespace: x = getattr(cli_config, entry) or file_config.get(entry, False) setattr(cli_config, entry, x) + for entry, default in scalar_map.items(): + x = getattr(cli_config, entry, None) or file_config.get(entry, default) + setattr(cli_config, entry, x) + # if either commandline parameter or config file option is set merge # with the other, if neither is set use the default for entry, default in lists_map.items(): diff --git a/src/ansiblelint/config.py b/src/ansiblelint/config.py index ad4fcb97d10..a60b6f22797 100644 --- a/src/ansiblelint/config.py +++ b/src/ansiblelint/config.py @@ -38,5 +38,6 @@ warn_list=[], kinds=DEFAULT_KINDS, mock_modules=[], - mock_roles=[] + mock_roles=[], + loop_var_prefix=None ) diff --git a/src/ansiblelint/file_utils.py b/src/ansiblelint/file_utils.py index 16ff5c46b0b..393994b4f18 100644 --- a/src/ansiblelint/file_utils.py +++ b/src/ansiblelint/file_utils.py @@ -106,6 +106,16 @@ def __init__( self.path = name self._content = content + # if the lintable is part of a role, we save role folder name + self.role = "" + parts = self.path.parent.parts + if 'roles' in parts: + role = self.path + while role.parent.name != "roles" and role.name: + role = role.parent + if role.exists: + self.role = role.name + self.kind = kind or kind_from_path(self.path) # We store absolute directory in dir if self.kind == "role": diff --git a/src/ansiblelint/rules/RoleLoopVarPrefix.py b/src/ansiblelint/rules/RoleLoopVarPrefix.py new file mode 100644 index 00000000000..2d6ef0c2995 --- /dev/null +++ b/src/ansiblelint/rules/RoleLoopVarPrefix.py @@ -0,0 +1,92 @@ +"""Optional Ansible-lint rule to enforce use of prefix on role loop vars.""" +from typing import TYPE_CHECKING, Any, List + +from ansiblelint.config import options +from ansiblelint.errors import MatchError +from ansiblelint.file_utils import Lintable +from ansiblelint.rules import AnsibleLintRule +from ansiblelint.text import toidentifier + +if TYPE_CHECKING: + from ansiblelint.constants import odict + + +class RoleLoopVarPrefix(AnsibleLintRule): + """Role loop_var should use configured prefix.""" + + id = 'no-loop-var-prefix' + shortdesc = __doc__ + url = "https://docs.ansible.com/ansible/latest/user_guide/playbooks_loops.html#defining-inner-and-outer-variable-names-with-loop-var" # noqa 501 + description = """\ +Looping inside roles has the risk of clashing with loops from user-playbooks. +This is recommended to always prefix them. + +See: %s +""" % url + + tags = ['no-loop-var-prefix', 'safety'] + prefix = "" + severity = 'MEDIUM' + + def matchplay( + self, + file: Lintable, + data: "odict[str, Any]") -> List[MatchError]: + """Return matches found for a specific playbook.""" + results: List[MatchError] = [] + + if not options.loop_var_prefix: + return results + self.prefix = options.loop_var_prefix.format(role=toidentifier(file.role)) + self.shortdesc = f"{self.__class__.shortdesc}: {self.prefix}" + + if file.kind not in ('tasks', 'handlers'): + return results + + results.extend(self.handle_play(file, data)) + return results + + def handle_play( + self, + lintable: Lintable, + task: "odict[str, Any]") -> List[MatchError]: + """Return matches for a playlist.""" + results = [] + if 'block' in task: + results.extend(self.handle_tasks(lintable, task['block'])) + else: + results.extend(self.handle_task(lintable, task)) + return results + + def handle_tasks( + self, + lintable: Lintable, + tasks: List["odict[str, Any]"]) -> List[MatchError]: + """Return matches for a list of tasks.""" + results = [] + for play in tasks: + results.extend(self.handle_play(lintable, play)) + return results + + def handle_task( + self, + lintable: Lintable, + task: "odict[str, Any]") -> List[MatchError]: + """Return matches for a specific task.""" + results = [] + has_loop = 'loop' in task + for key in task.keys(): + if key.startswith('with_'): + has_loop = True + + if has_loop: + loop_control = task.get('loop_control', {}) + loop_var = loop_control.get('loop_var', "") + + if not loop_var or not loop_var.startswith(self.prefix): + results.append( + self.create_matcherror( + filename=lintable, + linenumber=task['__line__'] + )) + return results diff --git a/src/ansiblelint/text.py b/src/ansiblelint/text.py index 6f15fd67b77..db07c837d9f 100644 --- a/src/ansiblelint/text.py +++ b/src/ansiblelint/text.py @@ -13,3 +13,11 @@ def strip_ansi_escape(data: Union[str, bytes]) -> str: data = data.decode("utf-8") return re.sub(r"\x1b[^m]*m", "", data) + + +def toidentifier(text: str) -> str: + """Convert unsafe chars to ones allowed in variables.""" + result = re.sub(r"[\s-]+", '_', text) + if not result.isidentifier: + raise RuntimeError("Unable to convert %s to valid variable name.", text) + return result