Rename var-spacing rule to jinja[spacing]

docs/rules.md

@@ -80,10 +80,10 @@ If the rule is line-based, `# noqa [rule_id]` must be at the end of the
 particular line to be skipped
 
 ```yaml
-- name: this would typically fire LineTooLongRule 204 and var-spacing
+- name: this would typically fire LineTooLongRule 204 and jinja[spacing]
   get_url:
     url: http://example.com/really_long_path/really_long_path/really_long_path/really_long_path/really_long_path/really_long_path/file.conf # noqa 204
-    dest: "{{dest_proj_path}}/foo.conf" # noqa var-spacing
+    dest: "{{dest_proj_path}}/foo.conf" # noqa jinja[spacing]
 ```
 
 It's also a good practice to comment the reasons why a task is being skipped.

examples/playbooks/var-spacing.yml

@@ -29,23 +29,23 @@
           {{- good_format -}}
     - name: bad variable format
       ansible.builtin.debug:
-        msg: "{{bad_format}}"
+        msg: "{{bad_format}}" # <-- 1
     - name: bad variable format
       ansible.builtin.debug:
-        msg: "Value: {{ bad_format}}"
+        msg: "Value: {{ bad_format}}" # <-- 2
     - name: bad variable format
       ansible.builtin.debug:
-        msg: "{{bad_format }}"
+        msg: "{{bad_format }}" # <-- 3
     - name: bad variable filter format
       ansible.builtin.debug:
-        msg: "{{ bad_format|filter }}"
+        msg: "{{ bad_format|filter }}" # <-- 4
     - name: bad variable filter format
       ansible.builtin.debug:
-        msg: "Value: {{ bad_format |filter }}"
+        msg: "Value: {{ bad_format |filter }}" # <-- 5
     - name: bad variable filter format
       ansible.builtin.debug:
-        msg: "{{ bad_format| filter }}"
-    - name: not a jinja variable # noqa var-spacing
+        msg: "{{ bad_format| filter }}" # <-- 6
+    - name: not a jinja variable # noqa: var-spacing
       ansible.builtin.debug:
         # spell-checker: disable-next-line
         msg: data = ${lookup{$local_part}lsearch{/etc/aliases}}
@@ -70,7 +70,7 @@
 
     - name: Invalid single line nested JSON
       ansible.builtin.debug:
-        msg: "{{ {'dummy_2': {'nested_dummy_1': 'value_1', 'nested_dummy_2': value_2}} | combine(dummy_1)}}"
+        msg: "{{ {'dummy_2': {'nested_dummy_1': 'value_1', 'nested_dummy_2': value_2}} | combine(dummy_1)}}" # <-- 7
 
     - name: Valid multiline nested JSON false positive
       ansible.builtin.debug:
@@ -81,6 +81,7 @@
 
     - name: Invalid multiline nested JSON
       ansible.builtin.debug:
+        # <-- 8
         msg: >-
           {{ {'dummy_2': {'nested_dummy_1': value_1,
                           'nested_dummy_2': value_2}} |

src/ansiblelint/_internal/rules.py

@@ -70,7 +70,7 @@ def matchlines(self, file: "Lintable") -> List["MatchError"]:
 
     def matchtask(
         self, task: Dict[str, Any], file: "Optional[Lintable]" = None
-    ) -> Union[bool, str]:
+    ) -> Union[bool, str, "MatchError"]:
         """Confirm if current rule is matching a specific task.
 
         If ``needs_raw_task`` (a class level attribute) is ``True``, then

src/ansiblelint/constants.py

@@ -65,7 +65,7 @@ def main():
 # playbooks and tasks.
 odict = dict  # pylint: disable=invalid-name
 
-# Deprecated tags/ids and their newer names
+# Aliases for deprecated tags/ids and their newer names
 RENAMED_TAGS = {
     "102": "no-jinja-when",
     "104": "deprecated-bare-vars",
@@ -74,7 +74,7 @@ def main():
     "202": "risky-octal",
     "203": "no-tabs",
     "205": "playbook-extension",
-    "206": "var-spacing",
+    "206": "jinja[spacing]",
     "207": "no-jinja-nesting",
     "208": "risky-file-permissions",
     "301": "no-changed-when",
@@ -99,6 +99,7 @@ def main():
     "703": "meta-incorrect",
     "704": "meta-video-links",
     "911": "syntax-check",
+    "var-spacing": "jinja[spacing]",
 }
 
 PLAYBOOK_TASK_KEYWORDS = [

src/ansiblelint/data/profiles.yml

@@ -30,16 +30,14 @@ basic:
     key-order:
     literal-compare:
     no-jinja-nesting:
-    jinja: # [jinja[readability]]
-      url: https://github.com/ansible/ansible-lint/issues/2120
+    jinja:
     no-jinja-when:
     no-tabs:
     partial-become:
     playbook-extension:
     role-name:
     unnamed-task:
     var-naming:
-    var-spacing:
     yaml:
 moderate:
   description: >

src/ansiblelint/rules/__init__.py

@@ -147,34 +147,39 @@ def matchtasks(self, file: Lintable) -> List[MatchError]:
         ):
             return matches
 
-        tasks_iterator = ansiblelint.yaml_utils.iter_tasks_in_file(file, self.id)
-        for raw_task, task, skipped, error in tasks_iterator:
+        tasks_iterator = ansiblelint.yaml_utils.iter_tasks_in_file(file)
+        for raw_task, task, skipped_tags, error in tasks_iterator:
             if error is not None:
                 # normalize_task converts AnsibleParserError to MatchError
                 return [error]
 
-            if skipped or "action" not in task:
+            if self.id in skipped_tags or ("action" not in task):
                 continue
 
             if self.needs_raw_task:
                 task["__raw_task__"] = raw_task
 
             result = self.matchtask(task, file=file)
-
             if not result:
                 continue
 
-            message = None
-            if isinstance(result, str):
-                message = result
-            task_msg = "Task/Handler: " + ansiblelint.utils.task_to_str(task)
-            match = self.create_matcherror(
-                message=message,
-                linenumber=task[ansiblelint.utils.LINE_NUMBER_KEY],
-                details=task_msg,
-                filename=file,
-            )
-            match.task = task
+            if isinstance(result, MatchError):
+                if result.tag in skipped_tags:
+                    continue
+                match = result
+            else:
+                message = None
+                if isinstance(result, str):
+                    message = result
+                task_msg = "Task/Handler: " + ansiblelint.utils.task_to_str(task)
+                match = self.create_matcherror(
+                    message=message,
+                    linenumber=task[ansiblelint.utils.LINE_NUMBER_KEY],
+                    details=task_msg,
+                    filename=file,
+                )
+                match.task = task
+
             matches.append(match)
         return matches
 

src/ansiblelint/rules/jinja.md

@@ -0,0 +1,20 @@
+## jinja
+
+This rule can report problems related to jinja2 string templates. The current
+version can report `jinja[spacing]` when there are no spaces between variables
+and operators, including filters, like `{{ var_name | filter }}`. This
+improves readability and makes it less likely to introduce typos.
+
+### Problematic code
+
+```yaml
+---
+foo: "{{some|dict2items}}"
+```
+
+### Correct code
+
+```yaml
+---
+foo: "{{ some | dict2items }}"
+```

src/ansiblelint/rules/var_spacing.py → src/ansiblelint/rules/jinja.py

@@ -1,4 +1,4 @@
-"""Rule for checking whitespace around variables."""
+"""Rule for checking content of jinja template strings."""
 # Copyright (c) 2016, Will Thames and contributors
 # Copyright (c) 2018, Ansible Project
 
@@ -11,17 +11,22 @@
 from ansiblelint.file_utils import Lintable
 from ansiblelint.rules import AnsibleLintRule
 from ansiblelint.skip_utils import get_rule_skips_from_line
-from ansiblelint.utils import parse_yaml_from_file
+from ansiblelint.utils import LINE_NUMBER_KEY, parse_yaml_from_file, task_to_str
 from ansiblelint.yaml_utils import nested_items_path
 
 if TYPE_CHECKING:
     from ansiblelint.errors import MatchError
 
+# Please note that current implementation is only able to report
+# `jinja[spacing]` occurrences, formerly named `var-spacing`. We aim to refactor
+# the implementation to replace currently unreliable regex with use of jinja2
+# parser. That will also allow use to implement additional checks.
 
-class VariableHasSpacesRule(AnsibleLintRule):
-    """Jinja2 variables and filters should have spaces before and after."""
 
-    id = "var-spacing"
+class JinjaRule(AnsibleLintRule):
+    """Rule that looks inside jinja2 templates."""
+
+    id = "jinja"
     severity = "LOW"
     tags = ["formatting"]
     version_added = "v6.3.0"
@@ -34,14 +39,21 @@ class VariableHasSpacesRule(AnsibleLintRule):
 
     def matchtask(
         self, task: Dict[str, Any], file: Optional[Lintable] = None
-    ) -> Union[bool, str]:
+    ) -> Union[bool, str, "MatchError"]:
         for _, v, _ in nested_items_path(task):
             if isinstance(v, str):
                 cleaned = self.exclude_json_re.sub("", v)
                 if bool(self.bracket_regex.search(cleaned)) or bool(
                     self.pipe_spacing_regex.search(cleaned)
                 ):
-                    return self.shortdesc.format(var_name=v)
+                    task_msg = "Task/Handler: " + task_to_str(task)
+                    return self.create_matcherror(
+                        message=f"Jinja2 variables and filters should have spaces before and after. ({v})",
+                        linenumber=task[LINE_NUMBER_KEY],
+                        details=task_msg,
+                        filename=file,
+                        tag=f"{self.id}[spacing]",
+                    )
         return False
 
     def matchyaml(self, file: Lintable) -> List["MatchError"]:
@@ -67,6 +79,7 @@ def matchyaml(self, file: Lintable) -> List["MatchError"]:
                                 linenumber=v.ansible_pos[1],
                                 message=self.shortdesc.format(var_name=v),
                                 details=f".{'.'.join(path_elem)}",
+                                tag="jinja[spacing]",
                             )
                         )
             if raw_results:
@@ -102,7 +115,7 @@ def fixture_test_playbook() -> str:
     def fixture_lint_error_lines(test_playbook: str) -> List[int]:
         """Get VarHasSpacesRules linting results on test_playbook."""
         collection = RulesCollection()
-        collection.register(VariableHasSpacesRule())
+        collection.register(JinjaRule())
         lintable = Lintable(test_playbook)
         results = Runner(lintable, rules=collection).run()
         return list(map(lambda item: item.linenumber, results))
@@ -148,7 +161,7 @@ def fixture_lint_error_results_varsfile(
     ) -> List["MatchError"]:
         """Get VarHasSpacesRules linting results on test_vars."""
         collection = RulesCollection()
-        collection.register(VariableHasSpacesRule())
+        collection.register(JinjaRule())
         lintable = Lintable(test_varsfile_path)
         results = Runner(lintable, rules=collection).run()
         return results

src/ansiblelint/skip_utils.py

@@ -46,8 +46,18 @@
 def get_rule_skips_from_line(line: str) -> List[str]:
     """Return list of rule ids skipped via comment on the line of yaml."""
     _before_noqa, _noqa_marker, noqa_text = line.partition("# noqa")
-    noqa_text = noqa_text.lstrip(" :")
-    return noqa_text.split()
+    result = []
+    for v in noqa_text.lstrip(" :").split():
+        if v in RENAMED_TAGS:
+            tag = RENAMED_TAGS[v]
+            _logger.warning(
+                "Replaced outdated tag '%s' with '%s', replace it to avoid future regressions.",
+                v,
+                tag,
+            )
+            v = tag
+        result.append(v)
+    return result
 
 
 def append_skipped_rules(

src/ansiblelint/yaml_utils.py

@@ -96,11 +96,11 @@ def load_yamllint_config() -> YamlLintConfig:
 
 
 def iter_tasks_in_file(
-    lintable: Lintable, rule_id: str
-) -> Iterator[Tuple[Dict[str, Any], Dict[str, Any], bool, Optional[MatchError]]]:
+    lintable: Lintable,
+) -> Iterator[Tuple[Dict[str, Any], Dict[str, Any], List[str], Optional[MatchError]]]:
     """Iterate over tasks in file.
 
-    This yields a 4-tuple of raw_task, normalized_task, skipped, and error.
+    This yields a 4-tuple of raw_task, normalized_task, skip_tags, and error.
 
     raw_task:
         When looping through the tasks in the file, each "raw_task" is minimally
@@ -110,8 +110,8 @@ def iter_tasks_in_file(
         by ansible into python objects and the action key gets normalized. If the task
         should be skipped (skipped is True) or normalizing it fails (error is not None)
         then this is just the raw_task instead of a normalized copy.
-    skipped:
-        Whether or not the task should be skipped according to tags or skipped_rules.
+    skip_tags:
+        List of tags found to be skipped, from tags block or noqa comments
     error:
         This is normally None. It will be a MatchError when the raw_task cannot be
         normalized due to an AnsibleParserError.
@@ -131,24 +131,22 @@ def iter_tasks_in_file(
     for raw_task in raw_tasks:
         err: Optional[MatchError] = None
 
-        # An empty `tags` block causes `None` to be returned if
-        # the `or []` is not present - `task.get('tags', [])`
-        # does not suffice.
-        skipped_in_task_tag = "skip_ansible_lint" in (raw_task.get("tags") or [])
-        skipped_in_yaml_comment = rule_id in raw_task.get("skipped_rules", ())
-        skipped = skipped_in_task_tag or skipped_in_yaml_comment
-        if skipped:
-            yield raw_task, raw_task, skipped, err
-            continue
+        skip_tags: List[str] = raw_task.get("skipped_rules", [])
 
         try:
             normalized_task = normalize_task(raw_task, str(lintable.path))
         except MatchError as err:
             # normalize_task converts AnsibleParserError to MatchError
-            yield raw_task, raw_task, skipped, err
+            yield raw_task, raw_task, skip_tags, err
             return
 
-        yield raw_task, normalized_task, skipped, err
+        if "skip_ansible_lint" in (raw_task.get("tags") or []):
+            skip_tags.append("skip_ansible_lint")
+        if skip_tags:
+            yield raw_task, normalized_task, skip_tags, err
+            continue
+
+        yield raw_task, normalized_task, skip_tags, err
 
 
 def nested_items_path(