Add optional rule to check for loop var prefix

Based on original custom rule written by Albin Vass on
https://opendev.org/zuul/zuul-jobs/blame/branch/master/.rules/ZuulJobsNamespaceLoopVar.py

This new rule becomes active only when loop_var_prefix is defined
inside the config and allows user to use dynamic prefix based
on role folder name.

.ansible-lint

@@ -1,6 +1,35 @@
+# .ansible-lint
 exclude_paths:
 - .github/
+# parseable: true
+# quiet: true
+# verbosity: 1
+
+# Mock modules or roles in order to pass ansible-playbook --syntax-check
 mock_modules:
   - zuul_return
 mock_roles:
   - mocked_role
+
+# Enable checking of loop variable prefixes in roles
+loop_var_prefix: "{role}_"
+
+use_default_rules: true
+# Load custom rules from this specific folder
+# rulesdir:
+#   - ./rule/directory/
+
+# This makes linter to fully ignore rules/tags listed below
+skip_list:
+  - skip_this_tag
+  - '401'
+
+# Report only a subset of tags and fully ignore any others
+# tags:
+#   - '206'
+
+# This makes the linter display but not fail for rules/tags listed below:
+warn_list:
+  - skip_this_tag
+  - '401'
+  - experimetal  # experimental is included in the implicit list

docs/configuring.rst

@@ -22,36 +22,8 @@ will be preferred, in the case of something like **quiet**.
 The following values are supported, and function identically to their CLI
 counterparts:
 
-.. code-block:: yaml
-
-    exclude_paths:
-      - ./my/excluded/directory/
-      - ./my/other/excluded/directory/
-      - ./last/excluded/directory/
-    parseable: true
-    quiet: true
-    rulesdir:
-      - ./rule/directory/
-    skip_list:
-      - skip_this_tag
-      - and_this_one_too
-      - skip_this_id
-      - '401'
-    tags:
-      - run_this_tag
-    use_default_rules: true
-    verbosity: 1
-    warn_list:
-      - skip_this_tag
-      - and_this_one_too
-      - skip_this_id
-      - '401'
-    # Mock modules or roles in order to pass ansible-playbook --syntax-check
-    mock_modules:
-      - some_module
-    mock_roles:
-      - some_role
-
+.. literalinclude:: ../.ansible-lint
+  :language: yaml
 
 Pre-commit Setup
 ----------------

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

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

src/ansiblelint/_internal/rules.py

@@ -18,6 +18,7 @@ class BaseRule:
     description: str = ""
     version_added: str = ""
     severity: str = ""
+    link: str = ""
 
     def getmatches(self, file: "Lintable") -> List["MatchError"]:
         """Return all matches while ignoring exceptions."""

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():

src/ansiblelint/config.py

@@ -38,5 +38,6 @@
     warn_list=[],
     kinds=DEFAULT_KINDS,
     mock_modules=[],
-    mock_roles=[]
+    mock_roles=[],
+    loop_var_prefix=None
 )

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":

src/ansiblelint/generate_docs.py

@@ -44,19 +44,28 @@ def rules_as_rst(rules: RulesCollection) -> str:
             r += f'\n\n{section}\n{ "-" * len(section) }'
 
         title = f"{d.id}: {d.shortdesc}"
-        r += f"\n\n.. _{d.id}:\n\n{title}\n{'*' * len(title)}\n\n{d.description}"
+
+        description = d.description
+        if d.link:
+            description += " `(more) <%s>`_" % d.link
+
+        r += f"\n\n.. _{d.id}:\n\n{title}\n{'*' * len(title)}\n\n{description}"
 
     return r
 
 
 @render_group()
 def rules_as_rich(rules: RulesCollection) -> Iterable[Table]:
     """Print documentation for a list of rules, returns empty string."""
+    width = max(16, *[len(rule.id) for rule in rules])
     for rule in rules:
         table = Table(show_header=True, header_style="title", box=box.MINIMAL)
-        table.add_column(rule.id, style="dim", width=16)
+        table.add_column(rule.id, style="dim", width=width)
         table.add_column(Markdown(rule.shortdesc))
-        table.add_row("description", Markdown(rule.description))
+        description = rule.description
+        if hasattr(rule, 'link'):
+            description += " [(more)](%s)" % rule.link
+        table.add_row("description", Markdown(description))
         if rule.version_added:
             table.add_row("version_added", rule.version_added)
         if rule.tags:

src/ansiblelint/rules/RoleLoopVarPrefix.py

@@ -0,0 +1,93 @@
+"""Optional Ansible-lint rule to enforce use of prefix on role loop vars."""
+from typing import TYPE_CHECKING, 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 typing import Any
+
+    from ansiblelint.constants import odict
+
+
+class RoleLoopVarPrefix(AnsibleLintRule):
+    """Role loop_var should use configured prefix."""
+
+    id = 'no-loop-var-prefix'
+    shortdesc = __doc__
+    link = (
+        "https://docs.ansible.com/ansible/latest/user_guide/"
+        "playbooks_loops.html#defining-inner-and-outer-variable-names-with-loop-var")
+    description = """\
+Looping inside roles has the risk of clashing with loops from user-playbooks.\
+"""
+
+    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

src/ansiblelint/text.py

@@ -13,3 +13,12 @@ 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 role name '%s' to valid variable name." % text)
+    return result