Update var_naming for set_fact module (ansible#2496)

* Update var_naming for set_fact module

Signed-off-by: nishipy <[email protected]>

* Add test for var_naming with pattern

Signed-off-by: nishipy <[email protected]>

* More fixes

Signed-off-by: nishipy <[email protected]>
Co-authored-by: Sorin Sbarnea <[email protected]>

.github/workflows/tox.yml

@@ -166,7 +166,7 @@ jobs:
       WSLENV: FORCE_COLOR:PYTEST_REQPASS:TOXENV:TOX_PARALLEL_NO_SPINNER
       # Number of expected test passes, safety measure for accidental skip of
       # tests. Update value if you add/remove tests.
-      PYTEST_REQPASS: 700
+      PYTEST_REQPASS: 701
 
     steps:
       - name: Activate WSL1

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

@@ -0,0 +1,28 @@
+---
+- name: Fixture
+  hosts: localhost
+  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
+    CamelCaseButErrorIgnored: true # noqa: var-naming
+
+  tasks:
+    - name: Foo
+      ansible.builtin.set_fact:
+        "{{ 'test_' }}var": "value" # valid
+    - name: Bar
+      ansible.builtin.set_fact:
+        CamelCaseButErrorIgnored: true # noqa: var-naming
+    - name: Test in a block
+      vars:
+        BAD: false # invalid
+        MoreBad: ... # invalid
+      block:
+        - name: Foo
+          vars:
+            ALL_CAPS_ARE_BAD_TOO: "{{ MoreBad }}" # invalid
+          ansible.builtin.set_fact:
+            CamelCaseIsBad: "{{ BAD }}" # invalid

examples/roles/var_naming_pattern/.ansible-lint

@@ -0,0 +1,2 @@
+---
+var_naming_pattern: "^[a-z][a-z0-9_]*[a-z0-9]__[a-z][a-z0-9_]*[a-z0-9]$"

examples/roles/var_naming_pattern/tasks/main.yml

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

src/ansiblelint/rules/var_naming.py

@@ -9,7 +9,7 @@
 from ansible.parsing.yaml.objects import AnsibleUnicode
 
 from ansiblelint.config import options
-from ansiblelint.constants import LINE_NUMBER_KEY
+from ansiblelint.constants import LINE_NUMBER_KEY, SUCCESS_RC
 from ansiblelint.file_utils import Lintable
 from ansiblelint.rules import AnsibleLintRule
 from ansiblelint.skip_utils import get_rule_skips_from_line
@@ -18,38 +18,6 @@
 if TYPE_CHECKING:
     from ansiblelint.errors import MatchError
 
-
-# Should raise var-naming at line [4, 8, 18, 20].
-FAIL_PLAY = """---
-- hosts: localhost
-  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
-    CamelCaseButErrorIgnored: true  # noqa: var-naming
-
-  tasks:
-    - name: Foo
-      ansible.builtin.set_fact:
-        "{{ 'test_' }}var": "value"  # valid
-    - name: Bar
-      ansible.builtin.set_fact:
-        CamelCaseButErrorIgnored: true  # noqa: var-naming
-    - name: Test in a block
-      block:
-        - name:
-          ansible.builtin.set_fact:
-            CamelCaseIsBad: "{{ BAD }}" # invalid
-          vars:
-            ALL_CAPS_ARE_BAD_TOO: "{{ MoreBad }}" # invalid
-      vars:
-        BAD: false # invalid
-        MoreBad: ... # invalid
-"""
-
-
 # Should raise var-naming at line [2, 6].
 FAIL_VARS = """---
 CamelCaseIsBad: false  # invalid
@@ -75,6 +43,7 @@ class VariableNamingRule(AnsibleLintRule):
     severity = "MEDIUM"
     tags = ["idiom", "experimental"]
     version_added = "v5.0.10"
+    needs_raw_task = True
     re_pattern = re.compile(options.var_naming_pattern or "^[a-z_][a-z0-9_]*$")
 
     def is_invalid_variable_name(self, ident: str) -> bool:
@@ -120,6 +89,7 @@ def matchplay(self, file: Lintable, data: dict[str, Any]) -> list[MatchError]:
                         message="Play defines variable '"
                         + key
                         + "' within 'vars' section that violates variable naming standards",
+                        tag=f"var-naming[{key}]",
                     )
                 )
         if raw_results:
@@ -134,28 +104,52 @@ def matchplay(self, file: Lintable, data: dict[str, Any]) -> list[MatchError]:
 
     def matchtask(
         self, task: dict[str, Any], file: Lintable | None = None
-    ) -> bool | str:
+    ) -> list[MatchError]:
         """Return matches for task based variables."""
+        results = []
         # If the task uses the 'vars' section to set variables
         our_vars = task.get("vars", {})
         for key in our_vars.keys():
             if self.is_invalid_variable_name(key):
-                return "Task defines variables within 'vars' section that violates variable naming standards"
+                results.append(
+                    self.create_matcherror(
+                        filename=file,
+                        linenumber=our_vars[LINE_NUMBER_KEY],
+                        message=f"Task defines variable within 'vars' section that violates variable naming standards: {key}",
+                        tag=f"var-naming[{key}]",
+                    )
+                )
 
         # If the task uses the 'set_fact' module
         ansible_module = task["action"]["__ansible_module__"]
-        ansible_action = task["action"]
         if ansible_module == "set_fact":
-            for key in ansible_action.keys():
+            for key in filter(
+                lambda x: isinstance(x, str) and not x.startswith("__"),
+                task["action"].keys(),
+            ):
                 if self.is_invalid_variable_name(key):
-                    return "Task uses 'set_fact' to define variables that violates variable naming standards"
+                    results.append(
+                        self.create_matcherror(
+                            filename=file,
+                            linenumber=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}]",
+                        )
+                    )
 
         # If the task registers a variable
         registered_var = task.get("register", None)
         if registered_var and self.is_invalid_variable_name(registered_var):
-            return "Task registers a variable that violates variable naming standards"
+            results.append(
+                self.create_matcherror(
+                    filename=file,
+                    linenumber=0,
+                    message=f"Task registers a variable that violates variable naming standards: {registered_var}",
+                    tag=f"var-naming[{registered_var}]",
+                )
+            )
 
-        return False
+        return results
 
     def matchyaml(self, file: Lintable) -> list[MatchError]:
         """Return matches for variables defined in vars files."""
@@ -193,20 +187,23 @@ def matchyaml(self, file: Lintable) -> list[MatchError]:
 
     import pytest
 
-    from ansiblelint.testing import RunFromText  # pylint: disable=ungrouped-imports
+    from ansiblelint.testing import (  # pylint: disable=ungrouped-imports
+        RunFromText,
+        run_ansible_lint,
+    )
 
     @pytest.mark.parametrize(
         "rule_runner", (VariableNamingRule,), indirect=["rule_runner"]
     )
     def test_invalid_var_name_playbook(rule_runner: RunFromText) -> None:
         """Test rule matches."""
-        results = rule_runner.run_playbook(FAIL_PLAY)
-        assert len(results) == 4
+        results = rule_runner.run("examples/playbooks/rule-var-naming-fail.yml")
+        assert len(results) == 6
         for result in results:
             assert result.rule.id == VariableNamingRule.id
 
         # list unexpected error lines or non-matching error lines
-        expected_error_lines = [4, 8, 18, 20]
+        expected_error_lines = [5, 9, 21, 21, 26, 28]
         lines = [i.linenumber for i in results]
         error_lines_difference = list(
             set(expected_error_lines).symmetric_difference(set(lines))
@@ -230,3 +227,14 @@ def test_invalid_var_name_varsfile(rule_runner: RunFromText) -> None:
             set(expected_error_lines).symmetric_difference(set(lines))
         )
         assert len(error_lines_difference) == 0
+
+    def test_var_naming_with_pattern() -> None:
+        """Test rule matches."""
+        role_path = "examples/roles/var_naming_pattern/tasks/main.yml"
+        conf_path = "examples/roles/var_naming_pattern/.ansible-lint"
+        result = run_ansible_lint(
+            f"--config-file={conf_path}",
+            role_path,
+        )
+        assert result.returncode == SUCCESS_RC
+        assert "var-naming" not in result.stdout