Upgrade var-naming rule to include role name prefix (#3422)

.github/workflows/tox.yml

@@ -59,7 +59,7 @@ jobs:
       WSLENV: FORCE_COLOR:PYTEST_REQPASS:TOXENV:GITHUB_STEP_SUMMARY
       # Number of expected test passes, safety measure for accidental skip of
       # tests. Update value if you add/remove tests.
-      PYTEST_REQPASS: 797
+      PYTEST_REQPASS: 796
     steps:
       - name: Activate WSL1
         if: "contains(matrix.shell, 'wsl')"

examples/playbooks/rule-var-naming-fail.yml

@@ -12,7 +12,7 @@
   tasks:
     - name: Foo
       ansible.builtin.set_fact:
-        "{{ 'test_' }}var": "value" # valid
+        "{{ 'test_' }}var": "value" # noqa: var-naming[no-jinja]
     - name: Bar
       ansible.builtin.set_fact:
         CamelCaseButErrorIgnored: true # noqa: var-naming

examples/playbooks/vars/rule_var_naming_fail.yml

@@ -0,0 +1,10 @@
+---
+CamelCaseIsBad: false # invalid
+this_is_valid: # valid because content is a dict, not a variable
+  CamelCase: ...
+  ALL_CAPS: ...
+ALL_CAPS_ARE_BAD_TOO: ... # invalid
+"{{ 'test_' }}var": "value" # noqa: schema
+CamelCaseButErrorIgnored: true # noqa: var-naming
+assert: true # invalid due to reserved keyword
+é: true # invalid due to non-ascii character

examples/roles/var_naming_pattern/tasks/main.yml

@@ -1,4 +1,4 @@
 ---
 - name: Foobar
   ansible.builtin.set_fact:
-    k8s_grafana_users__namespace: "foo"
+    var_naming_pattern__namespace: "foo"

src/ansiblelint/file_utils.py

@@ -180,12 +180,14 @@ class Lintable:
     When symlinks are given, they will always be resolved to their target.
     """
 
-    def __init__(
+    # pylint: disable=too-many-arguments
+    def __init__(  # noqa: C901
         self,
         name: str | Path,
         content: str | None = None,
         kind: FileType | None = None,
         base_kind: str = "",
+        parent: Lintable | None = None,
     ):
         """Create a Lintable instance."""
         self.dir: str = ""
@@ -194,6 +196,7 @@ def __init__(
         self.state: Any = States.NOT_LOADED
         self.line_skips: dict[int, set[str]] = defaultdict(set)
         self.exc: Exception | None = None  # Stores data loading exceptions
+        self.parent = parent
 
         if isinstance(name, str):
             name = Path(name)
@@ -243,6 +246,9 @@ def __init__(
         self.base_kind = base_kind or kind_from_path(self.path, base=True)
         self.abspath = self.path.expanduser().absolute()
 
+        if self.kind == "tasks":
+            self.parent = _guess_parent(self)
+
         if self.kind == "yaml":
             _ = self.data  # pylint: disable=pointless-statement
 
@@ -553,3 +559,14 @@ def expand_dirs_in_lintables(lintables: set[Lintable]) -> None:
                 for filename in all_files:
                     if filename.startswith(str(item.path)):
                         lintables.add(Lintable(filename))
+
+
+def _guess_parent(lintable: Lintable) -> Lintable | None:
+    """Return a parent directory for a lintable."""
+    try:
+        if lintable.path.parents[2].name == "roles":
+            # role_name = lintable.parents[1].name
+            return Lintable(lintable.path.parents[1], kind="role")
+    except IndexError:
+        pass
+    return None

src/ansiblelint/rules/var_naming.md

@@ -9,6 +9,16 @@ alphabetic or underscore `_` character.
 For more information see the [creating valid variable names][var-names] topic in
 Ansible documentation and [Naming things (Good Practices for Ansible)][cop].
 
+Possible errors messages:
+
+- `var-naming[non-string]`: Variables names must be strings.
+- `var-naming[non-ascii]`: Variables names must be ASCII.
+- `var-naming[no-keyword]`: Variables names must not be Python keywords.
+- `var-naming[no-jinja]`: Variables names must not contain jinja2 templating.
+- `var-naming[pattern]`: Variables names should match ... regex.
+- `var-naming[no-role-prefix]`: Variables names from within roles should use
+  `role_name_` as a prefix.
+
 ## Settings
 
 This rule behavior can be changed by altering the below settings:

src/ansiblelint/rules/var_naming.py

@@ -10,31 +10,17 @@
 
 from ansiblelint.config import options
 from ansiblelint.constants import LINE_NUMBER_KEY, RC
+from ansiblelint.errors import MatchError
 from ansiblelint.file_utils import Lintable
 from ansiblelint.rules import AnsibleLintRule, RulesCollection
 from ansiblelint.runner import Runner
 from ansiblelint.skip_utils import get_rule_skips_from_line
 from ansiblelint.utils import parse_yaml_from_file
 
 if TYPE_CHECKING:
-    from pathlib import Path
-
-    from ansiblelint.errors import MatchError
     from ansiblelint.utils import Task
 
 
-# Should raise var-naming at line [2, 6].
-FAIL_VARS = """---
-CamelCaseIsBad: false  # invalid
-this_is_valid:  # valid because content is a dict, not a variable
-  CamelCase: ...
-  ALL_CAPS: ...
-ALL_CAPS_ARE_BAD_TOO: ...  # invalid
-"{{ 'test_' }}var": "value"  # valid
-CamelCaseButErrorIgnored: true  # noqa: var-naming
-"""
-
-
 class VariableNamingRule(AnsibleLintRule):
     """All variables should be named using only lowercase and underscores."""
 
@@ -43,30 +29,62 @@ class VariableNamingRule(AnsibleLintRule):
     tags = ["idiom"]
     version_added = "v5.0.10"
     needs_raw_task = True
-    re_pattern = re.compile(options.var_naming_pattern or "^[a-z_][a-z0-9_]*$")
+    re_pattern_str = options.var_naming_pattern or "^[a-z_][a-z0-9_]*$"
+    re_pattern = re.compile(re_pattern_str)
 
-    def is_invalid_variable_name(self, ident: str) -> bool:
-        """Check if variable name is using right pattern."""
-        # Based on /~https://github.com/ansible/ansible/blob/devel/lib/ansible/utils/vars.py#L235
+    # pylint: disable=too-many-return-statements)
+    def get_var_naming_matcherror(
+        self,
+        ident: str,
+        *,
+        prefix: str = "",
+    ) -> MatchError | None:
+        """Return a MatchError if the variable name is not valid, otherwise None."""
         if not isinstance(ident, str):  # pragma: no cover
-            return False
+            return MatchError(
+                tag="var-naming[non-string]",
+                message="Variables names must be strings.",
+                rule=self,
+            )
 
         try:
             ident.encode("ascii")
         except UnicodeEncodeError:
-            return False
+            return MatchError(
+                tag="var-naming[non-ascii]",
+                message="Variables names must be ASCII.",
+                rule=self,
+            )
 
         if keyword.iskeyword(ident):
-            return False
+            return MatchError(
+                tag="var-naming[no-keyword]",
+                message="Variables names must not be Python keywords.",
+                rule=self,
+            )
 
         # We want to allow use of jinja2 templating for variable names
         if "{{" in ident:
-            return False
+            return MatchError(
+                tag="var-naming[no-jinja]",
+                message="Variables names must not contain jinja2 templating.",
+                rule=self,
+            )
 
-        # previous tests should not be triggered as they would have raised a
-        # syntax-error when we loaded the files but we keep them here as a
-        # safety measure.
-        return not bool(self.re_pattern.match(ident))
+        if not bool(self.re_pattern.match(ident)):
+            return MatchError(
+                tag="var-naming[pattern]",
+                message=f"Variables names should match {self.re_pattern_str} regex.",
+                rule=self,
+            )
+
+        if prefix and not ident.startswith(f"{prefix}_"):
+            return MatchError(
+                tag="var-naming[no-role-prefix]",
+                message="Variables names from within roles should use role_name_ as a prefix.",
+                rule=self,
+            )
+        return None
 
     def matchplay(self, file: Lintable, data: dict[str, Any]) -> list[MatchError]:
         """Return matches found for a specific playbook."""
@@ -78,19 +96,15 @@ def matchplay(self, file: Lintable, data: dict[str, Any]) -> list[MatchError]:
         # If the Play uses the 'vars' section to set variables
         our_vars = data.get("vars", {})
         for key in our_vars:
-            if self.is_invalid_variable_name(key):
-                raw_results.append(
-                    self.create_matcherror(
-                        filename=file,
-                        lineno=key.ansible_pos[1]
-                        if isinstance(key, AnsibleUnicode)
-                        else our_vars[LINE_NUMBER_KEY],
-                        message="Play defines variable '"
-                        + key
-                        + "' within 'vars' section that violates variable naming standards",
-                        tag=f"var-naming[{key}]",
-                    ),
+            match_error = self.get_var_naming_matcherror(key)
+            if match_error:
+                match_error.filename = str(file.path)
+                match_error.lineno = (
+                    key.ansible_pos[1]
+                    if isinstance(key, AnsibleUnicode)
+                    else our_vars[LINE_NUMBER_KEY]
                 )
+                raw_results.append(match_error)
         if raw_results:
             lines = file.content.splitlines()
             for match in raw_results:
@@ -111,47 +125,44 @@ def matchtask(
     ) -> list[MatchError]:
         """Return matches for task based variables."""
         results = []
+        prefix = ""
+        filename = "" if file is None else str(file.path)
+        if file and file.parent and file.parent.kind == "role":
+            prefix = file.parent.path.name
         # If the task uses the 'vars' section to set variables
         our_vars = task.get("vars", {})
         for key in our_vars:
-            if self.is_invalid_variable_name(key):
-                results.append(
-                    self.create_matcherror(
-                        filename=file,
-                        lineno=our_vars[LINE_NUMBER_KEY],
-                        message=f"Task defines variable within 'vars' section that violates variable naming standards: {key}",
-                        tag=f"var-naming[{key}]",
-                    ),
-                )
+            match_error = self.get_var_naming_matcherror(key, prefix=prefix)
+            if match_error:
+                match_error.filename = filename
+                match_error.lineno = our_vars[LINE_NUMBER_KEY]
+                match_error.message += f" (vars: {key})"
+                results.append(match_error)
 
         # If the task uses the 'set_fact' module
+        # breakpoint()
         ansible_module = task["action"]["__ansible_module__"]
         if ansible_module == "set_fact":
             for key in filter(
                 lambda x: isinstance(x, str) and not x.startswith("__"),
                 task["action"].keys(),
             ):
-                if self.is_invalid_variable_name(key):
-                    results.append(
-                        self.create_matcherror(
-                            filename=file,
-                            lineno=task["action"][LINE_NUMBER_KEY],
-                            message=f"Task uses 'set_fact' to define variables that violates variable naming standards: {key}",
-                            tag=f"var-naming[{key}]",
-                        ),
-                    )
+                match_error = self.get_var_naming_matcherror(key, prefix=prefix)
+                if match_error:
+                    match_error.filename = filename
+                    match_error.lineno = task["action"][LINE_NUMBER_KEY]
+                    match_error.message += f" (set_fact: {key})"
+                    results.append(match_error)
 
         # If the task registers a variable
         registered_var = task.get("register", None)
-        if registered_var and self.is_invalid_variable_name(registered_var):
-            results.append(
-                self.create_matcherror(
-                    filename=file,
-                    lineno=task[LINE_NUMBER_KEY],
-                    message=f"Task registers a variable that violates variable naming standards: {registered_var}",
-                    tag=f"var-naming[{registered_var}]",
-                ),
-            )
+        if registered_var:
+            match_error = self.get_var_naming_matcherror(registered_var, prefix=prefix)
+            if match_error:
+                match_error.message += f" (register: {registered_var})"
+                match_error.filename = filename
+                match_error.lineno = task[LINE_NUMBER_KEY]
+                results.append(match_error)
 
         return results
 
@@ -160,20 +171,17 @@ def matchyaml(self, file: Lintable) -> list[MatchError]:
         results: list[MatchError] = []
         raw_results: list[MatchError] = []
         meta_data: dict[AnsibleUnicode, Any] = {}
+        filename = "" if file is None else str(file.path)
 
         if str(file.kind) == "vars" and file.data:
             meta_data = parse_yaml_from_file(str(file.path))
             for key in meta_data:
-                if self.is_invalid_variable_name(key):
-                    raw_results.append(
-                        self.create_matcherror(
-                            filename=file,
-                            lineno=key.ansible_pos[1],
-                            message="File defines variable '"
-                            + key
-                            + "' that violates variable naming standards",
-                        ),
-                    )
+                match_error = self.get_var_naming_matcherror(key)
+                if match_error:
+                    match_error.filename = filename
+                    match_error.lineno = key.ansible_pos[1]
+                    match_error.message += f" (vars: {key})"
+                    raw_results.append(match_error)
             if raw_results:
                 lines = file.content.splitlines()
                 for match in raw_results:
@@ -194,7 +202,6 @@ def matchyaml(self, file: Lintable) -> list[MatchError]:
     import pytest
 
     from ansiblelint.testing import (  # pylint: disable=ungrouped-imports
-        RunFromText,
         run_ansible_lint,
     )
 
@@ -217,28 +224,26 @@ def test_invalid_var_name_playbook(file: str, expected: int) -> None:
         # different versions of ruamel.yaml (and depending on presence/absence
         # of its c-extension)
 
-    @pytest.mark.parametrize(
-        "rule_runner",
-        (VariableNamingRule,),
-        indirect=["rule_runner"],
-    )
     def test_invalid_var_name_varsfile(
-        rule_runner: RunFromText,
-        tmp_path: Path,
+        default_rules_collection: RulesCollection,
     ) -> None:
         """Test rule matches."""
-        results = rule_runner.run_role_defaults_main(FAIL_VARS, tmp_path=tmp_path)
-        assert len(results) == 2
-        for result in results:
-            assert result.rule.id == VariableNamingRule.id
-
-        # list unexpected error lines or non-matching error lines
-        expected_error_lines = [2, 6]
-        lines = [i.lineno for i in results]
-        error_lines_difference = list(
-            set(expected_error_lines).symmetric_difference(set(lines)),
+        results = Runner(
+            Lintable("examples/playbooks/vars/rule_var_naming_fail.yml"),
+            rules=default_rules_collection,
+        ).run()
+        expected_errors = (
+            ("schema[vars]", 1),
+            ("var-naming[pattern]", 2),
+            ("var-naming[pattern]", 6),
+            ("var-naming[no-jinja]", 7),
+            ("var-naming[no-keyword]", 9),
+            ("var-naming[non-ascii]", 10),
         )
-        assert len(error_lines_difference) == 0
+        assert len(results) == len(expected_errors)
+        for idx, result in enumerate(results):
+            assert result.tag == expected_errors[idx][0]
+            assert result.lineno == expected_errors[idx][1]
 
     def test_var_naming_with_pattern() -> None:
         """Test rule matches."""
@@ -250,9 +255,3 @@ def test_var_naming_with_pattern() -> None:
         )
         assert result.returncode == RC.SUCCESS
         assert "var-naming" not in result.stdout
-
-    def test_is_invalid_variable_name() -> None:
-        """Test for invalid variable names."""
-        var_name_rule = VariableNamingRule()
-        assert var_name_rule.is_invalid_variable_name("assert") is False
-        assert var_name_rule.is_invalid_variable_name("é") is False