diff options
Diffstat (limited to 'src/ansiblelint/rules')
69 files changed, 1992 insertions, 467 deletions
diff --git a/src/ansiblelint/rules/__init__.py b/src/ansiblelint/rules/__init__.py index acb7df1..a1743a0 100644 --- a/src/ansiblelint/rules/__init__.py +++ b/src/ansiblelint/rules/__init__.py @@ -1,4 +1,5 @@ """All internal ansible-lint rules.""" + from __future__ import annotations import copy @@ -23,7 +24,7 @@ from ansiblelint._internal.rules import ( WarningRule, ) from ansiblelint.app import App, get_app -from ansiblelint.config import PROFILES, Options, get_rule_config +from ansiblelint.config import PROFILES, Options from ansiblelint.config import options as default_options from ansiblelint.constants import LINE_NUMBER_KEY, RULE_DOC_URL, SKIPPED_RULES_KEY from ansiblelint.errors import MatchError @@ -32,6 +33,8 @@ from ansiblelint.file_utils import Lintable, expand_paths_vars if TYPE_CHECKING: from ruamel.yaml.comments import CommentedMap, CommentedSeq + from ansiblelint.errors import RuleMatchTransformMeta + _logger = logging.getLogger(__name__) match_types = { @@ -53,11 +56,6 @@ class AnsibleLintRule(BaseRule): """Return rule documentation url.""" return RULE_DOC_URL + self.id + "/" - @property - def rule_config(self) -> dict[str, Any]: - """Retrieve rule specific configuration.""" - return get_rule_config(self.id) - def get_config(self, key: str) -> Any: """Return a configured value for given key string.""" return self.rule_config.get(key, None) @@ -78,6 +76,7 @@ class AnsibleLintRule(BaseRule): details: str = "", filename: Lintable | None = None, tag: str = "", + transform_meta: RuleMatchTransformMeta | None = None, ) -> MatchError: """Instantiate a new MatchError.""" match = MatchError( @@ -87,13 +86,14 @@ class AnsibleLintRule(BaseRule): lintable=filename or Lintable(""), rule=copy.copy(self), tag=tag, + transform_meta=transform_meta, ) # search through callers to find one of the match* methods frame = inspect.currentframe() match_type: str | None = None while not match_type and frame is not None: func_name = frame.f_code.co_name - match_type = match_types.get(func_name, None) + match_type = match_types.get(func_name) if match_type: # add the match_type to the match match.match_type = match_type @@ -109,8 +109,8 @@ class AnsibleLintRule(BaseRule): match.task = task if not match.details: match.details = "Task/Handler: " + ansiblelint.utils.task_to_str(task) - if match.lineno < task[LINE_NUMBER_KEY]: - match.lineno = task[LINE_NUMBER_KEY] + + match.lineno = max(match.lineno, task[LINE_NUMBER_KEY]) def matchlines(self, file: Lintable) -> list[MatchError]: matches: list[MatchError] = [] @@ -224,7 +224,16 @@ class AnsibleLintRule(BaseRule): if isinstance(yaml, str): if yaml.startswith("$ANSIBLE_VAULT"): return [] - return [MatchError(lintable=file, rule=LoadingFailureRule())] + if self._collection is None: + msg = f"Rule {self.id} was not added to a collection." + raise RuntimeError(msg) + return [ + # pylint: disable=E1136 + MatchError( + lintable=file, + rule=self._collection["load-failure"], + ), + ] if not yaml: return matches @@ -250,7 +259,7 @@ class AnsibleLintRule(BaseRule): class TransformMixin: """A mixin for AnsibleLintRule to enable transforming files. - If ansible-lint is started with the ``--write`` option, then the ``Transformer`` + If ansible-lint is started with the ``--fix`` option, then the ``Transformer`` will call the ``transform()`` method for every MatchError identified if the rule that identified it subclasses this ``TransformMixin``. Only the rule that identified a MatchError can do transforms to fix that match. @@ -324,7 +333,6 @@ class TransformMixin: return target -# pylint: disable=too-many-nested-blocks def load_plugins( dirs: list[str], ) -> Iterator[AnsibleLintRule]: @@ -370,7 +378,7 @@ def load_plugins( class RulesCollection: """Container for a collection of rules.""" - def __init__( + def __init__( # pylint: disable=too-many-arguments self, rulesdirs: list[str] | list[Path] | None = None, options: Options | None = None, @@ -388,7 +396,7 @@ class RulesCollection: else: self.options = options self.profile = [] - self.app = app or get_app(offline=True) + self.app = app or get_app(cached=True) if profile_name: self.profile = PROFILES[profile_name] @@ -405,6 +413,8 @@ class RulesCollection: WarningRule(), ], ) + for rule in self.rules: + rule._collection = self # noqa: SLF001 for rule in load_plugins(rulesdirs_str): self.register(rule, conditional=conditional) self.rules = sorted(self.rules) @@ -443,6 +453,17 @@ class RulesCollection: """Return the length of the RulesCollection data.""" return len(self.rules) + def __getitem__(self, item: Any) -> BaseRule: + """Return a rule from inside the collection based on its id.""" + if not isinstance(item, str): + msg = f"Expected str but got {type(item)} when trying to access rule by it's id" + raise TypeError(msg) + for rule in self.rules: + if rule.id == item: + return rule + msg = f"Rule {item} is not present inside this collection." + raise ValueError(msg) + def extend(self, more: list[AnsibleLintRule]) -> None: """Combine rules.""" self.rules.extend(more) @@ -469,7 +490,7 @@ class RulesCollection: MatchError( message=str(exc), lintable=file, - rule=LoadingFailureRule(), + rule=self["load-failure"], tag=f"{LoadingFailureRule.id}[{exc.__class__.__name__.lower()}]", ), ] @@ -482,10 +503,18 @@ class RulesCollection: or rule.has_dynamic_tags or not set(rule.tags).union([rule.id]).isdisjoint(tags) ): - rule_definition = set(rule.tags) - rule_definition.add(rule.id) - if set(rule_definition).isdisjoint(skip_list): - matches.extend(rule.getmatches(file)) + if tags and set(rule.tags).union(list(rule.ids().keys())).isdisjoint( + tags, + ): + _logger.debug("Skipping rule %s", rule.id) + else: + _logger.debug("Running rule %s", rule.id) + rule_definition = set(rule.tags) + rule_definition.add(rule.id) + if set(rule_definition).isdisjoint(skip_list): + matches.extend(rule.getmatches(file)) + else: + _logger.debug("Skipping rule %s", rule.id) # some rules can produce matches with tags that are inside our # skip_list, so we need to cleanse the matches @@ -499,6 +528,15 @@ class RulesCollection: [rule.verbose() for rule in sorted(self.rules, key=lambda x: x.id)], ) + def known_tags(self) -> list[str]: + """Return a list of known tags, without returning no sub-tags.""" + tags = set() + for rule in self.rules: + tags.add(rule.id) + for tag in rule.tags: + tags.add(tag) + return sorted(tags) + def list_tags(self) -> str: """Return a string with all the tags in the RulesCollection.""" tag_desc = { @@ -525,11 +563,10 @@ class RulesCollection: msg = f"Rule {rule} does not have any of the required tags: {', '.join(tag_desc.keys())}" raise RuntimeError(msg) for tag in rule.tags: - for id_ in rule.ids(): - tags[tag].append(id_) + tags[tag] = list(rule.ids()) result = "# List of tags and rules they cover\n" for tag in sorted(tags): - desc = tag_desc.get(tag, None) + desc = tag_desc.get(tag) if desc: result += f"{tag}: # {desc}\n" else: @@ -550,6 +587,8 @@ def filter_rules_with_profile(rule_col: list[BaseRule], profile: str) -> None: included.add(rule) extends = PROFILES[extends].get("extends", None) for rule in rule_col.copy(): + if rule.unloadable: + continue if rule.id not in included: _logger.debug( "Unloading %s rule due to not being part of %s profile.", diff --git a/src/ansiblelint/rules/args.py b/src/ansiblelint/rules/args.py index 2acf32e..fb9f991 100644 --- a/src/ansiblelint/rules/args.py +++ b/src/ansiblelint/rules/args.py @@ -1,4 +1,5 @@ """Rule definition to validate task options.""" + from __future__ import annotations import contextlib @@ -8,7 +9,6 @@ import json import logging import re import sys -from functools import lru_cache from typing import TYPE_CHECKING, Any # pylint: disable=preferred-module @@ -18,11 +18,11 @@ from unittest.mock import patch # pylint: disable=reimported import ansible.module_utils.basic as mock_ansible_module from ansible.module_utils import basic -from ansible.plugins.loader import PluginLoadContext, module_loader from ansiblelint.constants import LINE_NUMBER_KEY from ansiblelint.rules import AnsibleLintRule, RulesCollection from ansiblelint.text import has_jinja +from ansiblelint.utils import load_plugin from ansiblelint.yaml_utils import clean_json if TYPE_CHECKING: @@ -66,12 +66,6 @@ workarounds_inject_map = { } -@lru_cache -def load_module(module_name: str) -> PluginLoadContext: - """Load plugin from module name and cache it.""" - return module_loader.find_plugin_with_context(module_name) - - class ValidationPassedError(Exception): """Exception to be raised when validation passes.""" @@ -103,7 +97,7 @@ class ArgsRule(AnsibleLintRule): task: Task, file: Lintable | None = None, ) -> list[MatchError]: - # pylint: disable=too-many-locals,too-many-return-statements + # pylint: disable=too-many-return-statements results: list[MatchError] = [] module_name = task["action"]["__ansible_module_original__"] failed_msg = None @@ -111,7 +105,7 @@ class ArgsRule(AnsibleLintRule): if module_name in self.module_aliases: return [] - loaded_module = load_module(module_name) + loaded_module = load_plugin(module_name) # https://github.com/ansible/ansible-lint/issues/3200 # since "ps1" modules cannot be executed on POSIX platforms, we will @@ -150,14 +144,10 @@ class ArgsRule(AnsibleLintRule): CustomAnsibleModule, ): spec = importlib.util.spec_from_file_location( - name=loaded_module.resolved_fqcn, + name=loaded_module.plugin_resolved_name, location=loaded_module.plugin_resolved_path, ) - if spec: - assert spec.loader is not None - module = importlib.util.module_from_spec(spec) - spec.loader.exec_module(module) - else: + if not spec: assert file is not None _logger.warning( "Unable to load module %s at %s:%s for options validation", @@ -166,6 +156,9 @@ class ArgsRule(AnsibleLintRule): task[LINE_NUMBER_KEY], ) return [] + assert spec.loader is not None + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) try: if not hasattr(module, "main"): @@ -196,9 +189,9 @@ class ArgsRule(AnsibleLintRule): ) sanitized_results = self._sanitize_results(results, module_name) - return sanitized_results except ValidationPassedError: return [] + return sanitized_results # pylint: disable=unused-argument def _sanitize_results( diff --git a/src/ansiblelint/rules/avoid_implicit.py b/src/ansiblelint/rules/avoid_implicit.py index 8d1fe26..d752ec7 100644 --- a/src/ansiblelint/rules/avoid_implicit.py +++ b/src/ansiblelint/rules/avoid_implicit.py @@ -1,4 +1,5 @@ """Implementation of avoid-implicit rule.""" + # https://github.com/ansible/ansible-lint/issues/2501 from __future__ import annotations @@ -40,8 +41,8 @@ class AvoidImplicitRule(AnsibleLintRule): # testing code to be loaded only with pytest or when executed the rule file if "pytest" in sys.modules: - from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports - from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + from ansiblelint.rules import RulesCollection + from ansiblelint.runner import Runner def test_template_instead_of_copy_positive() -> None: """Positive test for avoid-implicit.""" diff --git a/src/ansiblelint/rules/command_instead_of_module.py b/src/ansiblelint/rules/command_instead_of_module.py index 068e430..538141b 100644 --- a/src/ansiblelint/rules/command_instead_of_module.py +++ b/src/ansiblelint/rules/command_instead_of_module.py @@ -1,4 +1,5 @@ """Implementation of command-instead-of-module rule.""" + # Copyright (c) 2013-2014 Will Thames <will@thames.id.au> # # Permission is hereby granted, free of charge, to any person obtaining a copy @@ -25,7 +26,7 @@ from pathlib import Path from typing import TYPE_CHECKING from ansiblelint.rules import AnsibleLintRule -from ansiblelint.utils import convert_to_boolean, get_first_cmd_arg, get_second_cmd_arg +from ansiblelint.utils import get_first_cmd_arg, get_second_cmd_arg if TYPE_CHECKING: from ansiblelint.file_utils import Lintable @@ -68,9 +69,17 @@ class CommandsInsteadOfModulesRule(AnsibleLintRule): } _executable_options = { - "git": ["branch", "log", "lfs"], - "systemctl": ["--version", "kill", "set-default", "show-environment", "status"], - "yum": ["clean"], + "git": ["branch", "log", "lfs", "rev-parse"], + "systemctl": [ + "--version", + "get-default", + "kill", + "set-default", + "set-property", + "show-environment", + "status", + ], + "yum": ["clean", "history", "info"], "rpm": ["--nodeps"], } @@ -97,9 +106,7 @@ class CommandsInsteadOfModulesRule(AnsibleLintRule): ): return False - if executable in self._modules and convert_to_boolean( - task["action"].get("warn", True), - ): + if executable in self._modules: message = "{0} used in place of {1} module" return message.format(executable, self._modules[executable]) return False @@ -108,8 +115,9 @@ class CommandsInsteadOfModulesRule(AnsibleLintRule): if "pytest" in sys.modules: import pytest - from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports - from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + # pylint: disable=ungrouped-imports + from ansiblelint.rules import RulesCollection + from ansiblelint.runner import Runner @pytest.mark.parametrize( ("file", "expected"), diff --git a/src/ansiblelint/rules/command_instead_of_shell.md b/src/ansiblelint/rules/command_instead_of_shell.md index 0abf69d..1e64d2c 100644 --- a/src/ansiblelint/rules/command_instead_of_shell.md +++ b/src/ansiblelint/rules/command_instead_of_shell.md @@ -28,3 +28,7 @@ environment variable expansion or chaining multiple commands using pipes. ansible.builtin.command: echo hello changed_when: false ``` + +!!! note + + This rule can be automatically fixed using [`--fix`](../autofix.md) option. diff --git a/src/ansiblelint/rules/command_instead_of_shell.py b/src/ansiblelint/rules/command_instead_of_shell.py index 346a071..789adca 100644 --- a/src/ansiblelint/rules/command_instead_of_shell.py +++ b/src/ansiblelint/rules/command_instead_of_shell.py @@ -1,4 +1,5 @@ """Implementation of command-instead-of-shell rule.""" + # Copyright (c) 2016 Will Thames <will@thames.id.au> # # Permission is hereby granted, free of charge, to any person obtaining a copy @@ -23,15 +24,18 @@ from __future__ import annotations import sys from typing import TYPE_CHECKING -from ansiblelint.rules import AnsibleLintRule +from ansiblelint.rules import AnsibleLintRule, TransformMixin from ansiblelint.utils import get_cmd_args if TYPE_CHECKING: + from ruamel.yaml.comments import CommentedMap, CommentedSeq + + from ansiblelint.errors import MatchError from ansiblelint.file_utils import Lintable from ansiblelint.utils import Task -class UseCommandInsteadOfShellRule(AnsibleLintRule): +class UseCommandInsteadOfShellRule(AnsibleLintRule, TransformMixin): """Use shell only when shell functionality is required.""" id = "command-instead-of-shell" @@ -62,13 +66,27 @@ class UseCommandInsteadOfShellRule(AnsibleLintRule): return not any(ch in jinja_stripped_cmd for ch in "&|<>;$\n*[]{}?`") return False + def transform( + self, + match: MatchError, + lintable: Lintable, + data: CommentedMap | CommentedSeq | str, + ) -> None: + if match.tag == "command-instead-of-shell": + target_task = self.seek(match.yaml_path, data) + for _ in range(len(target_task)): + k, v = target_task.popitem(False) + target_task["ansible.builtin.command" if "shell" in k else k] = v + match.fixed = True + # testing code to be loaded only with pytest or when executed the rule file if "pytest" in sys.modules: import pytest - from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports - from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + # pylint: disable=ungrouped-imports + from ansiblelint.rules import RulesCollection + from ansiblelint.runner import Runner @pytest.mark.parametrize( ("file", "expected"), diff --git a/src/ansiblelint/rules/complexity.md b/src/ansiblelint/rules/complexity.md new file mode 100644 index 0000000..aa25a1e --- /dev/null +++ b/src/ansiblelint/rules/complexity.md @@ -0,0 +1,19 @@ +# complexity + +This rule aims to warn about Ansible content that seems to be overly complex, +suggesting refactoring for better readability and maintainability. + +## complexity[tasks] + +`complexity[tasks]` will be triggered if the total number of tasks inside a file +is above 100. If encountered, you should consider using +[`ansible.builtin.include_tasks`](https://docs.ansible.com/ansible/latest/collections/ansible/builtin/include_tasks_module.html) +to split your tasks into smaller files. + +## complexity[nesting] + +`complexity[nesting]` will appear when a block contains too many tasks, by +default that number is 20 but it can be changed inside the configuration file by +defining `max_block_depth` value. + + Replace nested block with an include_tasks to make code easier to maintain. Maximum block depth allowed is ... diff --git a/src/ansiblelint/rules/complexity.py b/src/ansiblelint/rules/complexity.py new file mode 100644 index 0000000..04d92d0 --- /dev/null +++ b/src/ansiblelint/rules/complexity.py @@ -0,0 +1,115 @@ +"""Implementation of limiting number of tasks.""" + +from __future__ import annotations + +import re +import sys +from typing import TYPE_CHECKING, Any + +from ansiblelint.constants import LINE_NUMBER_KEY +from ansiblelint.rules import AnsibleLintRule, RulesCollection + +if TYPE_CHECKING: + from ansiblelint.config import Options + from ansiblelint.errors import MatchError + from ansiblelint.file_utils import Lintable + from ansiblelint.utils import Task + + +class ComplexityRule(AnsibleLintRule): + """Rule for limiting number of tasks inside a file.""" + + id = "complexity" + description = "There should be limited tasks executed inside any file" + severity = "MEDIUM" + tags = ["experimental", "idiom"] + version_added = "v6.18.0 (last update)" + _re_templated_inside = re.compile(r".*\{\{.*\}\}.*\w.*$") + + def matchplay(self, file: Lintable, data: dict[str, Any]) -> list[MatchError]: + """Call matchplay for up to no_of_max_tasks inside file and return aggregate results.""" + results: list[MatchError] = [] + + if file.kind != "playbook": + return [] + tasks = data.get("tasks", []) + if not isinstance(self._collection, RulesCollection): + msg = "Rules cannot be run outside a rule collection." + raise TypeError(msg) + if len(tasks) > self._collection.options.max_tasks: + results.append( + self.create_matcherror( + message=f"Maximum tasks allowed in a play is {self._collection.options.max_tasks}.", + lineno=data[LINE_NUMBER_KEY], + tag=f"{self.id}[play]", + filename=file, + ), + ) + return results + + def matchtask(self, task: Task, file: Lintable | None = None) -> list[MatchError]: + """Check if the task is a block and count the number of items inside it.""" + results: list[MatchError] = [] + + if not isinstance(self._collection, RulesCollection): + msg = "Rules cannot be run outside a rule collection." + raise TypeError(msg) + + if task.action == "block/always/rescue": + block_depth = self.calculate_block_depth(task) + if block_depth > self._collection.options.max_block_depth: + results.append( + self.create_matcherror( + message=f"Replace nested block with an include_tasks to make code easier to maintain. Maximum block depth allowed is {self._collection.options.max_block_depth}.", + lineno=task[LINE_NUMBER_KEY], + tag=f"{self.id}[nesting]", + filename=file, + ), + ) + return results + + def calculate_block_depth(self, task: Task) -> int: + """Recursively calculate the block depth of a task.""" + if not isinstance(task.position, str): + raise NotImplementedError + return task.position.count(".block") + + +if "pytest" in sys.modules: + import pytest + + # pylint: disable=ungrouped-imports + from ansiblelint.runner import Runner + + @pytest.mark.parametrize( + ("file", "expected_results"), + ( + pytest.param( + "examples/playbooks/rule-complexity-pass.yml", + [], + id="pass", + ), + pytest.param( + "examples/playbooks/rule-complexity-fail.yml", + ["complexity[play]", "complexity[nesting]"], + id="fail", + ), + ), + ) + def test_complexity( + file: str, + expected_results: list[str], + monkeypatch: pytest.MonkeyPatch, + config_options: Options, + ) -> None: + """Test rule.""" + monkeypatch.setattr(config_options, "max_tasks", 5) + monkeypatch.setattr(config_options, "max_block_depth", 3) + collection = RulesCollection(options=config_options) + collection.register(ComplexityRule()) + results = Runner(file, rules=collection).run() + + assert len(results) == len(expected_results) + for i, result in enumerate(results): + assert result.rule.id == ComplexityRule.id, result + assert result.tag == expected_results[i] diff --git a/src/ansiblelint/rules/conftest.py b/src/ansiblelint/rules/conftest.py index f4df7a5..5a22ffd 100644 --- a/src/ansiblelint/rules/conftest.py +++ b/src/ansiblelint/rules/conftest.py @@ -1,3 +1,4 @@ """Makes pytest fixtures available.""" + # pylint: disable=wildcard-import,unused-wildcard-import from ansiblelint.testing.fixtures import * # noqa: F403 diff --git a/src/ansiblelint/rules/deprecated_bare_vars.py b/src/ansiblelint/rules/deprecated_bare_vars.py index 1756e92..7b1ab08 100644 --- a/src/ansiblelint/rules/deprecated_bare_vars.py +++ b/src/ansiblelint/rules/deprecated_bare_vars.py @@ -27,7 +27,7 @@ import sys from typing import TYPE_CHECKING, Any from ansiblelint.rules import AnsibleLintRule -from ansiblelint.text import has_glob, has_jinja +from ansiblelint.text import has_glob, has_jinja, is_fqcn_or_name if TYPE_CHECKING: from ansiblelint.file_utils import Lintable @@ -66,7 +66,7 @@ class UsingBareVariablesIsDeprecatedRule(AnsibleLintRule): # we just need to check that one variable, and not iterate over it like # it's a list. Otherwise, loop through and check all items. items = task[loop_type] - if not isinstance(items, (list, tuple)): + if not isinstance(items, list | tuple): items = [items] for var in items: return self._matchvar(var, task, loop_type) @@ -84,7 +84,11 @@ class UsingBareVariablesIsDeprecatedRule(AnsibleLintRule): task: dict[str, Any], loop_type: str, ) -> bool | str: - if isinstance(varstring, str) and not has_jinja(varstring): + if ( + isinstance(varstring, str) + and not has_jinja(varstring) + and is_fqcn_or_name(varstring) + ): valid = loop_type == "with_fileglob" and bool( has_jinja(varstring) or has_glob(varstring), ) @@ -121,4 +125,4 @@ if "pytest" in sys.modules: failure = "examples/playbooks/rule-deprecated-bare-vars-fail.yml" bad_runner = Runner(failure, rules=collection) errs = bad_runner.run() - assert len(errs) == 12 + assert len(errs) == 11 diff --git a/src/ansiblelint/rules/deprecated_local_action.md b/src/ansiblelint/rules/deprecated_local_action.md index c52eb9d..68f4345 100644 --- a/src/ansiblelint/rules/deprecated_local_action.md +++ b/src/ansiblelint/rules/deprecated_local_action.md @@ -19,3 +19,7 @@ This rule recommends using `delegate_to: localhost` instead of the ansible.builtin.debug: delegate_to: localhost # <-- recommended way to run on localhost ``` + +!!! note + + This rule can be automatically fixed using [`--fix`](../autofix.md) option. diff --git a/src/ansiblelint/rules/deprecated_local_action.py b/src/ansiblelint/rules/deprecated_local_action.py index fc3e4ff..4e09795 100644 --- a/src/ansiblelint/rules/deprecated_local_action.py +++ b/src/ansiblelint/rules/deprecated_local_action.py @@ -1,19 +1,33 @@ """Implementation for deprecated-local-action rule.""" + # Copyright (c) 2016, Tsukinowa Inc. <info@tsukinowa.jp> # Copyright (c) 2018, Ansible Project from __future__ import annotations +import copy +import logging +import os import sys +from pathlib import Path from typing import TYPE_CHECKING -from ansiblelint.rules import AnsibleLintRule +from ansiblelint.rules import AnsibleLintRule, TransformMixin +from ansiblelint.runner import get_matches +from ansiblelint.transformer import Transformer if TYPE_CHECKING: + from ruamel.yaml.comments import CommentedMap, CommentedSeq + + from ansiblelint.config import Options + from ansiblelint.errors import MatchError from ansiblelint.file_utils import Lintable from ansiblelint.utils import Task -class TaskNoLocalAction(AnsibleLintRule): +_logger = logging.getLogger(__name__) + + +class TaskNoLocalAction(AnsibleLintRule, TransformMixin): """Do not use 'local_action', use 'delegate_to: localhost'.""" id = "deprecated-local-action" @@ -35,11 +49,46 @@ class TaskNoLocalAction(AnsibleLintRule): return False + def transform( + self, + match: MatchError, + lintable: Lintable, + data: CommentedMap | CommentedSeq | str, + ) -> None: + if match.tag == self.id: + # we do not want perform a partial modification accidentally + original_target_task = self.seek(match.yaml_path, data) + target_task = copy.deepcopy(original_target_task) + for _ in range(len(target_task)): + k, v = target_task.popitem(False) + if k == "local_action": + if isinstance(v, dict): + module_name = v["module"] + target_task[module_name] = None + target_task["delegate_to"] = "localhost" + elif isinstance(v, str): + module_name, module_value = v.split(" ", 1) + target_task[module_name] = module_value + target_task["delegate_to"] = "localhost" + else: + _logger.debug( + "Ignored unexpected data inside %s transform.", + self.id, + ) + return + else: + target_task[k] = v + match.fixed = True + original_target_task.clear() + original_target_task.update(target_task) + # testing code to be loaded only with pytest or when executed the rule file if "pytest" in sys.modules: - from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports - from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + from unittest import mock + + from ansiblelint.rules import RulesCollection + from ansiblelint.runner import Runner def test_local_action(default_rules_collection: RulesCollection) -> None: """Positive test deprecated_local_action.""" @@ -50,3 +99,34 @@ if "pytest" in sys.modules: assert len(results) == 1 assert results[0].tag == "deprecated-local-action" + + @mock.patch.dict(os.environ, {"ANSIBLE_LINT_WRITE_TMP": "1"}, clear=True) + def test_local_action_transform( + config_options: Options, + default_rules_collection: RulesCollection, + ) -> None: + """Test transform functionality for no-log-password rule.""" + playbook = Path("examples/playbooks/tasks/local_action.yml") + config_options.write_list = ["all"] + + config_options.lintables = [str(playbook)] + runner_result = get_matches( + rules=default_rules_collection, + options=config_options, + ) + transformer = Transformer(result=runner_result, options=config_options) + transformer.run() + matches = runner_result.matches + assert len(matches) == 3 + + orig_content = playbook.read_text(encoding="utf-8") + expected_content = playbook.with_suffix( + f".transformed{playbook.suffix}", + ).read_text(encoding="utf-8") + transformed_content = playbook.with_suffix(f".tmp{playbook.suffix}").read_text( + encoding="utf-8", + ) + + assert orig_content != transformed_content + assert expected_content == transformed_content + playbook.with_suffix(f".tmp{playbook.suffix}").unlink() diff --git a/src/ansiblelint/rules/deprecated_module.py b/src/ansiblelint/rules/deprecated_module.py index 03c9361..72e328f 100644 --- a/src/ansiblelint/rules/deprecated_module.py +++ b/src/ansiblelint/rules/deprecated_module.py @@ -1,4 +1,5 @@ """Implementation of deprecated-module rule.""" + # Copyright (c) 2018, Ansible Project from __future__ import annotations diff --git a/src/ansiblelint/rules/empty_string_compare.py b/src/ansiblelint/rules/empty_string_compare.py index 5c7cafc..6870ed2 100644 --- a/src/ansiblelint/rules/empty_string_compare.py +++ b/src/ansiblelint/rules/empty_string_compare.py @@ -1,4 +1,5 @@ """Implementation of empty-string-compare rule.""" + # Copyright (c) 2016, Will Thames and contributors # Copyright (c) 2018, Ansible Project @@ -54,8 +55,8 @@ class ComparisonToEmptyStringRule(AnsibleLintRule): # testing code to be loaded only with pytest or when executed the rule file if "pytest" in sys.modules: - from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports - from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + from ansiblelint.rules import RulesCollection + from ansiblelint.runner import Runner def test_rule_empty_string_compare_fail() -> None: """Test rule matches.""" diff --git a/src/ansiblelint/rules/fqcn.md b/src/ansiblelint/rules/fqcn.md index 0165477..a64a324 100644 --- a/src/ansiblelint/rules/fqcn.md +++ b/src/ansiblelint/rules/fqcn.md @@ -87,3 +87,7 @@ structure in a backward-compatible way by adding redirects like in # Use the FQCN for the builtin shell module. ansible.builtin.shell: ssh ssh_user@{{ ansible_ssh_host }} ``` + +!!! note + + This rule can be automatically fixed using [`--fix`](../autofix.md) option. diff --git a/src/ansiblelint/rules/fqcn.py b/src/ansiblelint/rules/fqcn.py index 768fb9e..b571db3 100644 --- a/src/ansiblelint/rules/fqcn.py +++ b/src/ansiblelint/rules/fqcn.py @@ -1,17 +1,19 @@ """Rule definition for usage of fully qualified collection names for builtins.""" + from __future__ import annotations import logging import sys from typing import TYPE_CHECKING, Any -from ansible.plugins.loader import module_loader +from ruamel.yaml.comments import CommentedSeq from ansiblelint.constants import LINE_NUMBER_KEY from ansiblelint.rules import AnsibleLintRule, TransformMixin +from ansiblelint.utils import load_plugin if TYPE_CHECKING: - from ruamel.yaml.comments import CommentedMap, CommentedSeq + from ruamel.yaml.comments import CommentedMap from ansiblelint.errors import MatchError from ansiblelint.file_utils import Lintable @@ -114,11 +116,16 @@ class FQCNBuiltinsRule(AnsibleLintRule, TransformMixin): task: Task, file: Lintable | None = None, ) -> list[MatchError]: - result = [] + result: list[MatchError] = [] + if file and file.failed(): + return result module = task["action"]["__ansible_module_original__"] + if not isinstance(module, str): + msg = "Invalid data for module." + raise TypeError(msg) if module not in self.module_aliases: - loaded_module = module_loader.find_plugin_with_context(module) + loaded_module = load_plugin(module) target = loaded_module.resolved_fqcn self.module_aliases[module] = target if target is None: @@ -137,40 +144,45 @@ class FQCNBuiltinsRule(AnsibleLintRule, TransformMixin): 1, ) if module != legacy_module: + if module == "ansible.builtin.include": + message = f"Avoid deprecated module ({module})" + details = "Use `ansible.builtin.include_task` or `ansible.builtin.import_tasks` instead." + else: + message = f"Use FQCN for builtin module actions ({module})." + details = f"Use `{module_alias}` or `{legacy_module}` instead." result.append( self.create_matcherror( - message=f"Use FQCN for builtin module actions ({module}).", - details=f"Use `{module_alias}` or `{legacy_module}` instead.", + message=message, + details=details, filename=file, lineno=task["__line__"], tag="fqcn[action-core]", ), ) - else: - if module.count(".") < 2: - result.append( - self.create_matcherror( - message=f"Use FQCN for module actions, such `{self.module_aliases[module]}`.", - details=f"Action `{module}` is not FQCN.", - filename=file, - lineno=task["__line__"], - tag="fqcn[action]", - ), - ) - # TODO(ssbarnea): Remove the c.g. and c.n. exceptions from here once # noqa: FIX002 - # community team is flattening these. - # https://github.com/ansible-community/community-topics/issues/147 - elif not module.startswith("community.general.") or module.startswith( - "community.network.", - ): - result.append( - self.create_matcherror( - message=f"You should use canonical module name `{self.module_aliases[module]}` instead of `{module}`.", - filename=file, - lineno=task["__line__"], - tag="fqcn[canonical]", - ), - ) + elif module.count(".") < 2: + result.append( + self.create_matcherror( + message=f"Use FQCN for module actions, such `{self.module_aliases[module]}`.", + details=f"Action `{module}` is not FQCN.", + filename=file, + lineno=task["__line__"], + tag="fqcn[action]", + ), + ) + # TODO(ssbarnea): Remove the c.g. and c.n. exceptions from here once # noqa: FIX002 + # community team is flattening these. + # https://github.com/ansible-community/community-topics/issues/147 + elif not module.startswith("community.general.") or module.startswith( + "community.network.", + ): + result.append( + self.create_matcherror( + message=f"You should use canonical module name `{self.module_aliases[module]}` instead of `{module}`.", + filename=file, + lineno=task["__line__"], + tag="fqcn[canonical]", + ), + ) return result def matchyaml(self, file: Lintable) -> list[MatchError]: @@ -220,6 +232,8 @@ class FQCNBuiltinsRule(AnsibleLintRule, TransformMixin): target_task = self.seek(match.yaml_path, data) # Unfortunately, a lot of data about Ansible content gets lost here, you only get a simple dict. # For now, just parse the error messages for the data about action names etc. and fix this later. + current_action = "" + new_action = "" if match.tag == "fqcn[action-core]": # split at the first bracket, cut off the last bracket and dot current_action = match.message.split("(")[1][:-2] @@ -233,6 +247,8 @@ class FQCNBuiltinsRule(AnsibleLintRule, TransformMixin): current_action = match.message.split("`")[3] new_action = match.message.split("`")[1] for _ in range(len(target_task)): + if isinstance(target_task, CommentedSeq): + continue k, v = target_task.popitem(False) target_task[new_action if k == current_action else k] = v match.fixed = True @@ -241,7 +257,7 @@ class FQCNBuiltinsRule(AnsibleLintRule, TransformMixin): # testing code to be loaded only with pytest or when executed the rule file if "pytest" in sys.modules: from ansiblelint.rules import RulesCollection - from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + from ansiblelint.runner import Runner def test_fqcn_builtin_fail() -> None: """Test rule matches.""" @@ -269,7 +285,7 @@ if "pytest" in sys.modules: """Test rule matches.""" collection = RulesCollection() collection.register(FQCNBuiltinsRule()) - failure = "examples/collection/plugins/modules/deep/beta.py" + failure = "examples/.collection/plugins/modules/deep/beta.py" results = Runner(failure, rules=collection).run() assert len(results) == 1 assert results[0].tag == "fqcn[deep]" @@ -279,6 +295,6 @@ if "pytest" in sys.modules: """Test rule does not match.""" collection = RulesCollection() collection.register(FQCNBuiltinsRule()) - success = "examples/collection/plugins/modules/alpha.py" + success = "examples/.collection/plugins/modules/alpha.py" results = Runner(success, rules=collection).run() assert len(results) == 0 diff --git a/src/ansiblelint/rules/galaxy.md b/src/ansiblelint/rules/galaxy.md index 61fc5c5..d719e30 100644 --- a/src/ansiblelint/rules/galaxy.md +++ b/src/ansiblelint/rules/galaxy.md @@ -26,6 +26,8 @@ This rule can produce messages such: - `galaxy[tags]` - `galaxy.yaml` must have one of the required tags: `application`, `cloud`, `database`, `infrastructure`, `linux`, `monitoring`, `networking`, `security`, `storage`, `tools`, `windows`. +- `galaxy[invalid-dependency-version]` = Invalid collection metadata. Dependency + version spec range is invalid If you want to ignore some of the messages above, you can add any of them to the `ignore_list`. @@ -60,12 +62,14 @@ description: "..." # Changelog Details -This rule expects a `CHANGELOG.md` or `.rst` file in the collection root or a -`changelogs/changelog.yaml` file. +This rule expects a `CHANGELOG.md`, `CHANGELOG.rst`, +`changelogs/changelog.yaml`, or `changelogs/changelog.yml` file in the +collection root. -If a `changelogs/changelog.yaml` file exists, the schema will be checked. +If a `changelogs/changelog.yaml` or `changelogs/changelog.yml` file exists, the +schema will be checked. -## Minimum required changelog.yaml file +## Minimum required changelog.yaml/changelog.yml file ```yaml # changelog.yaml diff --git a/src/ansiblelint/rules/galaxy.py b/src/ansiblelint/rules/galaxy.py index 2f627f5..e9b21d3 100644 --- a/src/ansiblelint/rules/galaxy.py +++ b/src/ansiblelint/rules/galaxy.py @@ -1,11 +1,12 @@ """Implementation of GalaxyRule.""" + from __future__ import annotations import sys from functools import total_ordering from typing import TYPE_CHECKING, Any -from ansiblelint.constants import LINE_NUMBER_KEY +from ansiblelint.constants import FILENAME_KEY, LINE_NUMBER_KEY from ansiblelint.rules import AnsibleLintRule if TYPE_CHECKING: @@ -27,6 +28,7 @@ class GalaxyRule(AnsibleLintRule): "galaxy[version-missing]": "galaxy.yaml should have version tag.", "galaxy[version-incorrect]": "collection version should be greater than or equal to 1.0.0", "galaxy[no-runtime]": "meta/runtime.yml file not found.", + "galaxy[invalid-dependency-version]": "Invalid collection metadata. Dependency version spec range is invalid", } def matchplay(self, file: Lintable, data: dict[str, Any]) -> list[MatchError]: @@ -39,6 +41,7 @@ class GalaxyRule(AnsibleLintRule): "application", "cloud", "database", + "eda", "infrastructure", "linux", "monitoring", @@ -55,6 +58,7 @@ class GalaxyRule(AnsibleLintRule): changelog_found = 0 changelog_paths = [ base_path / "changelogs" / "changelog.yaml", + base_path / "changelogs" / "changelog.yml", base_path / "CHANGELOG.rst", base_path / "CHANGELOG.md", ] @@ -62,8 +66,21 @@ class GalaxyRule(AnsibleLintRule): for path in changelog_paths: if path.is_file(): changelog_found = 1 - - galaxy_tag_list = data.get("tags", None) + galaxy_tag_list = data.get("tags") + collection_deps = data.get("dependencies") + if collection_deps: + for dep, ver in collection_deps.items(): + if ( + dep not in [LINE_NUMBER_KEY, FILENAME_KEY] + and len(str(ver).strip()) == 0 + ): + results.append( + self.create_matcherror( + message=f"Invalid collection metadata. Dependency version spec range is invalid for '{dep}'.", + tag="galaxy[invalid-dependency-version]", + filename=file, + ), + ) # Changelog Check - building off Galaxy rule as there is no current way to check # for a nonexistent file @@ -108,7 +125,6 @@ class GalaxyRule(AnsibleLintRule): results.append( self.create_matcherror( message="collection version should be greater than or equal to 1.0.0", - # pylint: disable=protected-access lineno=version._line_number, # noqa: SLF001 tag="galaxy[version-incorrect]", filename=file, @@ -154,7 +170,7 @@ class Version: def _coerce(other: object) -> Version: if isinstance(other, str): other = Version(other) - if isinstance(other, (int, float)): + if isinstance(other, int | float): other = Version(str(other)) if isinstance(other, Version): return other @@ -172,7 +188,7 @@ if "pytest" in sys.modules: """Positive test for collection version in galaxy.""" collection = RulesCollection() collection.register(GalaxyRule()) - success = "examples/collection/galaxy.yml" + success = "examples/.collection/galaxy.yml" good_runner = Runner(success, rules=collection) assert [] == good_runner.run() @@ -189,7 +205,7 @@ if "pytest" in sys.modules: """Test for no collection version in galaxy.""" collection = RulesCollection() collection.register(GalaxyRule()) - failure = "examples/no_collection_version/galaxy.yml" + failure = "examples/.no_collection_version/galaxy.yml" bad_runner = Runner(failure, rules=collection) errs = bad_runner.run() assert len(errs) == 1 @@ -222,17 +238,25 @@ if "pytest" in sys.modules: id="pass", ), pytest.param( - "examples/collection/galaxy.yml", + "examples/.collection/galaxy.yml", ["schema[galaxy]"], id="schema", ), pytest.param( - "examples/no_changelog/galaxy.yml", + "examples/.invalid_dependencies/galaxy.yml", + [ + "galaxy[invalid-dependency-version]", + "galaxy[invalid-dependency-version]", + ], + id="invalid-dependency-version", + ), + pytest.param( + "examples/.no_changelog/galaxy.yml", ["galaxy[no-changelog]"], id="no-changelog", ), pytest.param( - "examples/no_collection_version/galaxy.yml", + "examples/.no_collection_version/galaxy.yml", ["schema[galaxy]", "galaxy[version-missing]"], id="no-collection-version", ), diff --git a/src/ansiblelint/rules/ignore_errors.py b/src/ansiblelint/rules/ignore_errors.py index 4144f2d..29f0408 100644 --- a/src/ansiblelint/rules/ignore_errors.py +++ b/src/ansiblelint/rules/ignore_errors.py @@ -1,4 +1,5 @@ """IgnoreErrorsRule used with ansible-lint.""" + from __future__ import annotations import sys @@ -44,7 +45,7 @@ if "pytest" in sys.modules: import pytest if TYPE_CHECKING: - from ansiblelint.testing import RunFromText # pylint: disable=ungrouped-imports + from ansiblelint.testing import RunFromText IGNORE_ERRORS_TRUE = """ - hosts: all diff --git a/src/ansiblelint/rules/inline_env_var.py b/src/ansiblelint/rules/inline_env_var.py index f578fb7..1f0747e 100644 --- a/src/ansiblelint/rules/inline_env_var.py +++ b/src/ansiblelint/rules/inline_env_var.py @@ -1,4 +1,5 @@ """Implementation of inside-env-var rule.""" + # Copyright (c) 2016 Will Thames <will@thames.id.au> # # Permission is hereby granted, free of charge, to any person obtaining a copy @@ -48,7 +49,6 @@ class EnvVarsInCommandRule(AnsibleLintRule): "executable", "removes", "stdin", - "warn", "stdin_add_newline", "strip_empty_ends", "cmd", diff --git a/src/ansiblelint/rules/jinja.md b/src/ansiblelint/rules/jinja.md index 8e1732e..e4720d7 100644 --- a/src/ansiblelint/rules/jinja.md +++ b/src/ansiblelint/rules/jinja.md @@ -12,7 +12,7 @@ version can report: As jinja2 syntax is closely following Python one we aim to follow [black](https://black.readthedocs.io/en/stable/) formatting rules. If you are -curious how black would reformat a small sniped feel free to visit +curious how black would reformat a small snippet feel free to visit [online black formatter](https://black.vercel.app/) site. Keep in mind to not include the entire jinja2 template, so instead of `{{ 1+2==3 }}`, do paste only `1+2==3`. @@ -53,3 +53,7 @@ In its current form, this rule presents the following limitations: does not support tilde as a binary operator. Example: `{{ a ~ b }}`. - Jinja2 blocks that use dot notation with numbers are ignored because python and black do not allow it. Example: `{{ foo.0.bar }}` + +!!! note + + This rule can be automatically fixed using [`--fix`](../autofix.md) option. diff --git a/src/ansiblelint/rules/jinja.py b/src/ansiblelint/rules/jinja.py index 08254bc..ff124a8 100644 --- a/src/ansiblelint/rules/jinja.py +++ b/src/ansiblelint/rules/jinja.py @@ -1,28 +1,35 @@ """Rule for checking content of jinja template strings.""" + from __future__ import annotations import logging +import os import re import sys -from collections import namedtuple +from dataclasses import dataclass from pathlib import Path -from typing import TYPE_CHECKING, Any +from typing import TYPE_CHECKING, Any, NamedTuple import black import jinja2 -from ansible.errors import AnsibleError, AnsibleParserError +from ansible.errors import AnsibleError, AnsibleFilterError, AnsibleParserError from ansible.parsing.yaml.objects import AnsibleUnicode from jinja2.exceptions import TemplateSyntaxError from ansiblelint.constants import LINE_NUMBER_KEY +from ansiblelint.errors import RuleMatchTransformMeta from ansiblelint.file_utils import Lintable -from ansiblelint.rules import AnsibleLintRule +from ansiblelint.rules import AnsibleLintRule, TransformMixin +from ansiblelint.runner import get_matches from ansiblelint.skip_utils import get_rule_skips_from_line from ansiblelint.text import has_jinja from ansiblelint.utils import parse_yaml_from_file, template from ansiblelint.yaml_utils import deannotate, nested_items_path if TYPE_CHECKING: + from ruamel.yaml.comments import CommentedMap, CommentedSeq + + from ansiblelint.config import Options from ansiblelint.errors import MatchError from ansiblelint.utils import Task @@ -30,7 +37,14 @@ if TYPE_CHECKING: _logger = logging.getLogger(__package__) KEYWORDS_WITH_IMPLICIT_TEMPLATE = ("changed_when", "failed_when", "until", "when") -Token = namedtuple("Token", "lineno token_type value") + +class Token(NamedTuple): + """Token.""" + + lineno: int + token_type: str + value: str + ignored_re = re.compile( "|".join( # noqa: FLY002 @@ -53,7 +67,27 @@ ignored_re = re.compile( ) -class JinjaRule(AnsibleLintRule): +@dataclass(frozen=True) +class JinjaRuleTMetaSpacing(RuleMatchTransformMeta): + """JinjaRule transform metadata. + + :param key: Key or index within the task + :param value: Value of the key + :param path: Path to the key + :param fixed: Value with spacing fixed + """ + + key: str | int + value: str | int + path: tuple[str | int, ...] + fixed: str + + def __str__(self) -> str: + """Return string representation.""" + return f"{self.key}={self.value} at {self.path} fixed to {self.fixed}" + + +class JinjaRule(AnsibleLintRule, TransformMixin): """Rule that looks inside jinja2 templates.""" id = "jinja" @@ -94,11 +128,13 @@ class JinjaRule(AnsibleLintRule): if isinstance(v, str): try: template( - basedir=file.path.parent if file else Path("."), + basedir=file.path.parent if file else Path(), value=v, variables=deannotate(task.get("vars", {})), fail_on_error=True, # we later decide which ones to ignore or not ) + except AnsibleFilterError: + bypass = True # ValueError RepresenterError except AnsibleError as exc: bypass = False @@ -111,7 +147,7 @@ class JinjaRule(AnsibleLintRule): ) if ignored_re.search(orig_exc_message) or isinstance( orig_exc, - AnsibleParserError, + AnsibleParserError | TypeError, ): # An unhandled exception occurred while running the lookup plugin 'template'. Error was a <class 'ansible.errors.AnsibleError'>, original message: the template file ... could not be found for the lookup. the template file ... could not be found for the lookup @@ -119,7 +155,7 @@ class JinjaRule(AnsibleLintRule): # AnsibleError(TemplateSyntaxError): template error while templating string: Could not load "ipwrap": 'Invalid plugin FQCN (ansible.netcommon.ipwrap): unable to locate collection ansible.netcommon'. String: Foo {{ buildset_registry.host | ipwrap }}. Could not load "ipwrap": 'Invalid plugin FQCN (ansible.netcommon.ipwrap): unable to locate collection ansible.netcommon' bypass = True elif ( - isinstance(orig_exc, (AnsibleError, TemplateSyntaxError)) + isinstance(orig_exc, AnsibleError | TemplateSyntaxError) and match ): error = match.group("error") @@ -166,6 +202,12 @@ class JinjaRule(AnsibleLintRule): details=details, filename=file, tag=f"{self.id}[{tag}]", + transform_meta=JinjaRuleTMetaSpacing( + key=key, + value=v, + path=tuple(path), + fixed=reformatted, + ), ), ) except Exception as exc: @@ -181,7 +223,6 @@ class JinjaRule(AnsibleLintRule): if str(file.kind) == "vars": data = parse_yaml_from_file(str(file.path)) - # pylint: disable=unused-variable for key, v, _path in nested_items_path(data): if isinstance(v, AnsibleUnicode): reformatted, details, tag = self.check_whitespace( @@ -249,7 +290,7 @@ class JinjaRule(AnsibleLintRule): last_value = value return result - # pylint: disable=too-many-statements,too-many-locals + # pylint: disable=too-many-locals def check_whitespace( self, text: str, @@ -327,7 +368,7 @@ class JinjaRule(AnsibleLintRule): # process expression # pylint: disable=unsupported-membership-test if isinstance(expr_str, str) and "\n" in expr_str: - raise NotImplementedError + raise NotImplementedError # noqa: TRY301 leading_spaces = " " * (len(expr_str) - len(expr_str.lstrip())) expr_str = leading_spaces + blacken(expr_str.lstrip()) if tokens[ @@ -348,7 +389,6 @@ class JinjaRule(AnsibleLintRule): except jinja2.exceptions.TemplateSyntaxError as exc: return "", str(exc.message), "invalid" - # https://github.com/PyCQA/pylint/issues/7433 - py311 only # pylint: disable=c-extension-no-member except (NotImplementedError, black.parsing.InvalidInput) as exc: # black is not able to recognize all valid jinja2 templates, so we @@ -370,6 +410,68 @@ class JinjaRule(AnsibleLintRule): ) return reformatted, details, "spacing" + def transform( + self: JinjaRule, + match: MatchError, + lintable: Lintable, + data: CommentedMap | CommentedSeq | str, + ) -> None: + """Transform jinja2 errors. + + :param match: MatchError instance + :param lintable: Lintable instance + :param data: data to transform + """ + if match.tag == "jinja[spacing]": + self._transform_spacing(match, data) + + def _transform_spacing( + self: JinjaRule, + match: MatchError, + data: CommentedMap | CommentedSeq | str, + ) -> None: + """Transform jinja2 spacing errors. + + The match error was found on a normalized task so we cannot compare the path + instead we only compare the key and value, if the task has 2 identical keys with the + exact same jinja spacing issue, we may transform them out of order + + :param match: MatchError instance + :param data: data to transform + """ + if not isinstance(match.transform_meta, JinjaRuleTMetaSpacing): + return + if isinstance(data, str): + return + + obj = self.seek(match.yaml_path, data) + if obj is None: + return + + ignored_keys = ("block", "ansible.builtin.block", "ansible.legacy.block") + for key, value, path in nested_items_path( + data_collection=obj, + ignored_keys=ignored_keys, + ): + if key == match.transform_meta.key and value == match.transform_meta.value: + if not path: + continue + for pth in path[:-1]: + try: + obj = obj[pth] + except (KeyError, TypeError) as exc: + err = f"Unable to transform {match.transform_meta}: {exc}" + _logger.error(err) # noqa: TRY400 + return + try: + obj[path[-1]][key] = match.transform_meta.fixed + match.fixed = True + + except (KeyError, TypeError) as exc: + err = f"Unable to transform {match.transform_meta}: {exc}" + _logger.error(err) # noqa: TRY400 + return + def blacken(text: str) -> str: """Format Jinja2 template using black.""" @@ -380,10 +482,14 @@ def blacken(text: str) -> str: if "pytest" in sys.modules: + from unittest import mock + import pytest - from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports - from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + # pylint: disable=ungrouped-imports + from ansiblelint.rules import RulesCollection + from ansiblelint.runner import Runner + from ansiblelint.transformer import Transformer @pytest.fixture(name="error_expected_lines") def fixture_error_expected_lines() -> list[int]: @@ -725,6 +831,38 @@ if "pytest" in sys.modules: errs = Runner(success, rules=collection).run() assert len(errs) == 0 + @mock.patch.dict(os.environ, {"ANSIBLE_LINT_WRITE_TMP": "1"}, clear=True) + def test_jinja_transform( + config_options: Options, + default_rules_collection: RulesCollection, + ) -> None: + """Test transform functionality for jinja rule.""" + playbook = Path("examples/playbooks/rule-jinja-before.yml") + config_options.write_list = ["all"] + + config_options.lintables = [str(playbook)] + runner_result = get_matches( + rules=default_rules_collection, + options=config_options, + ) + transformer = Transformer(result=runner_result, options=config_options) + transformer.run() + + matches = runner_result.matches + assert len(matches) == 2 + + orig_content = playbook.read_text(encoding="utf-8") + expected_content = playbook.with_suffix( + f".transformed{playbook.suffix}", + ).read_text(encoding="utf-8") + transformed_content = playbook.with_suffix(f".tmp{playbook.suffix}").read_text( + encoding="utf-8", + ) + + assert orig_content != transformed_content + assert expected_content == transformed_content + playbook.with_suffix(f".tmp{playbook.suffix}").unlink() + def _get_error_line(task: dict[str, Any], path: list[str | int]) -> int: """Return error line number.""" @@ -736,5 +874,5 @@ def _get_error_line(task: dict[str, Any], path: list[str | int]) -> int: line = ctx[LINE_NUMBER_KEY] if not isinstance(line, int): msg = "Line number is not an integer" - raise RuntimeError(msg) + raise TypeError(msg) return line diff --git a/src/ansiblelint/rules/key_order.md b/src/ansiblelint/rules/key_order.md index 378d8a5..bcef36a 100644 --- a/src/ansiblelint/rules/key_order.md +++ b/src/ansiblelint/rules/key_order.md @@ -61,3 +61,7 @@ we concluded that the block keys must be the last ones. Another common practice was to put `tags` as the last property. Still, for the same reasons, we decided that they should not be put after block keys either. + +!!! note + + This rule can be automatically fixed using [`--fix`](../autofix.md) option. diff --git a/src/ansiblelint/rules/key_order.py b/src/ansiblelint/rules/key_order.py index 897da64..0c0a2f1 100644 --- a/src/ansiblelint/rules/key_order.py +++ b/src/ansiblelint/rules/key_order.py @@ -1,14 +1,19 @@ """All tasks should be have name come first.""" + from __future__ import annotations import functools import sys -from typing import TYPE_CHECKING +from dataclasses import dataclass +from typing import TYPE_CHECKING, Any -from ansiblelint.rules import AnsibleLintRule +from ansiblelint.constants import ANNOTATION_KEYS, LINE_NUMBER_KEY +from ansiblelint.errors import MatchError, RuleMatchTransformMeta +from ansiblelint.rules import AnsibleLintRule, TransformMixin if TYPE_CHECKING: - from ansiblelint.errors import MatchError + from ruamel.yaml.comments import CommentedMap, CommentedSeq + from ansiblelint.file_utils import Lintable from ansiblelint.utils import Task @@ -46,7 +51,21 @@ def task_property_sorter(property1: str, property2: str) -> int: return (v_1 > v_2) - (v_1 < v_2) -class KeyOrderRule(AnsibleLintRule): +@dataclass(frozen=True) +class KeyOrderTMeta(RuleMatchTransformMeta): + """Key Order transform metadata. + + :param fixed: tuple with updated key order + """ + + fixed: tuple[str | int, ...] + + def __str__(self) -> str: + """Return string representation.""" + return f"Fixed to {self.fixed}" + + +class KeyOrderRule(AnsibleLintRule, TransformMixin): """Ensure specific order of keys in mappings.""" id = "key-order" @@ -59,6 +78,25 @@ class KeyOrderRule(AnsibleLintRule): "key-order[task]": "You can improve the task key order", } + def matchplay(self, file: Lintable, data: dict[str, Any]) -> list[MatchError]: + """Return matches found for a specific play (entry in playbook).""" + result: list[MatchError] = [] + if file.kind != "playbook": + return result + keys = [str(key) for key, val in data.items() if key not in ANNOTATION_KEYS] + sorted_keys = sorted(keys, key=functools.cmp_to_key(task_property_sorter)) + if keys != sorted_keys: + result.append( + self.create_matcherror( + f"You can improve the play key order to: {', '.join(sorted_keys)}", + filename=file, + tag=f"{self.id}[play]", + lineno=data[LINE_NUMBER_KEY], + transform_meta=KeyOrderTMeta(fixed=tuple(sorted_keys)), + ), + ) + return result + def matchtask( self, task: Task, @@ -66,7 +104,7 @@ class KeyOrderRule(AnsibleLintRule): ) -> list[MatchError]: result = [] raw_task = task["__raw_task__"] - keys = [key for key in raw_task if not key.startswith("_")] + keys = [str(key) for key in raw_task if not key.startswith("_")] sorted_keys = sorted(keys, key=functools.cmp_to_key(task_property_sorter)) if keys != sorted_keys: result.append( @@ -74,17 +112,43 @@ class KeyOrderRule(AnsibleLintRule): f"You can improve the task key order to: {', '.join(sorted_keys)}", filename=file, tag="key-order[task]", + transform_meta=KeyOrderTMeta(fixed=tuple(sorted_keys)), ), ) return result + def transform( + self, + match: MatchError, + lintable: Lintable, + data: CommentedMap | CommentedSeq | str, + ) -> None: + if not isinstance(match.transform_meta, KeyOrderTMeta): + return + + if match.tag == f"{self.id}[play]": + play = self.seek(match.yaml_path, data) + for key in match.transform_meta.fixed: + # other transformation might change the key + if key in play: + play[key] = play.pop(key) + match.fixed = True + if match.tag == f"{self.id}[task]": + task = self.seek(match.yaml_path, data) + for key in match.transform_meta.fixed: + # other transformation might change the key + if key in task: + task[key] = task.pop(key) + match.fixed = True + # testing code to be loaded only with pytest or when executed the rule file if "pytest" in sys.modules: import pytest - from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports - from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + # pylint: disable=ungrouped-imports + from ansiblelint.rules import RulesCollection + from ansiblelint.runner import Runner @pytest.mark.parametrize( ("test_file", "failures"), diff --git a/src/ansiblelint/rules/latest.py b/src/ansiblelint/rules/latest.py index 0838feb..ef57b94 100644 --- a/src/ansiblelint/rules/latest.py +++ b/src/ansiblelint/rules/latest.py @@ -1,4 +1,5 @@ """Implementation of latest rule.""" + from __future__ import annotations from typing import TYPE_CHECKING diff --git a/src/ansiblelint/rules/literal_compare.py b/src/ansiblelint/rules/literal_compare.py index 1129d1d..151398a 100644 --- a/src/ansiblelint/rules/literal_compare.py +++ b/src/ansiblelint/rules/literal_compare.py @@ -1,4 +1,5 @@ """Implementation of the literal-compare rule.""" + # Copyright (c) 2016, Will Thames and contributors # Copyright (c) 2018-2021, Ansible Project @@ -55,8 +56,9 @@ class ComparisonToLiteralBoolRule(AnsibleLintRule): if "pytest" in sys.modules: import pytest - from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports - from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + # pylint: disable=ungrouped-imports + from ansiblelint.rules import RulesCollection + from ansiblelint.runner import Runner @pytest.mark.parametrize( ("test_file", "failures"), diff --git a/src/ansiblelint/rules/loop_var_prefix.md b/src/ansiblelint/rules/loop_var_prefix.md index 33adbd7..5d1b9b0 100644 --- a/src/ansiblelint/rules/loop_var_prefix.md +++ b/src/ansiblelint/rules/loop_var_prefix.md @@ -1,15 +1,15 @@ # loop-var-prefix -This rule avoids conflicts with nested looping tasks by configuring a variable -prefix with `loop_var`. Ansible sets `item` as the loop variable. You can use -`loop_var` to specify a prefix for loop variables and ensure they are unique to -each task. +This rule avoids conflicts with nested looping tasks by enforcing an individual +variable name in loops. Ansible defaults to `item` as the loop variable. You can +use `loop_var` to rename it. Optionally require a prefix on the variable name. +The prefix can be configured via the `<loop_var_prefix>` setting. This rule can produce the following messages: - `loop-var-prefix[missing]` - Replace any unsafe implicit `item` loop variable - by adding `loop_var: <loop_var_prefix>...`. -- `loop-var-prefix[wrong]` - Ensure loop variables start with + by adding `loop_var: <variable_name>...`. +- `loop-var-prefix[wrong]` - Ensure the loop variable starts with `<loop_var_prefix>`. This rule originates from the [Naming parameters section of Ansible Best @@ -41,20 +41,20 @@ enable_list: - name: Example playbook hosts: localhost tasks: - - name: Does not set a prefix for loop variables. + - name: Does not set a variable name for loop variables. ansible.builtin.debug: - var: item + var: item # <- When in a nested loop, "item" is ambiguous loop: - foo - - bar # <- These items do not have a unique prefix. - - name: Sets a prefix that is not unique. + - bar + - name: Sets a variable name that doesn't start with <loop_var_prefix>. ansible.builtin.debug: var: zz_item loop: - foo - bar loop_control: - loop_var: zz_item # <- This prefix is not unique. + loop_var: zz_item # <- zz is not the role name so the prefix is wrong ``` ## Correct Code @@ -64,14 +64,14 @@ enable_list: - name: Example playbook hosts: localhost tasks: - - name: Sets a unique prefix for loop variables. + - name: Sets a unique variable_name with role as prefix for loop variables. ansible.builtin.debug: - var: zz_item + var: myrole_item loop: - foo - bar loop_control: - loop_var: my_prefix # <- Specifies a unique prefix for loop variables. + loop_var: myrole_item # <- Unique variable name with role as prefix ``` [cop314]: diff --git a/src/ansiblelint/rules/loop_var_prefix.py b/src/ansiblelint/rules/loop_var_prefix.py index 8f1bb56..9f7a2ca 100644 --- a/src/ansiblelint/rules/loop_var_prefix.py +++ b/src/ansiblelint/rules/loop_var_prefix.py @@ -1,4 +1,5 @@ """Optional Ansible-lint rule to enforce use of prefix on role loop vars.""" + from __future__ import annotations import re @@ -81,8 +82,9 @@ Looping inside roles has the risk of clashing with loops from user-playbooks.\ if "pytest" in sys.modules: import pytest - from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports - from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + # pylint: disable=ungrouped-imports + from ansiblelint.rules import RulesCollection + from ansiblelint.runner import Runner @pytest.mark.parametrize( ("test_file", "failures"), diff --git a/src/ansiblelint/rules/meta_incorrect.py b/src/ansiblelint/rules/meta_incorrect.py index 4252254..ed8d8d9 100644 --- a/src/ansiblelint/rules/meta_incorrect.py +++ b/src/ansiblelint/rules/meta_incorrect.py @@ -1,4 +1,5 @@ """Implementation of meta-incorrect rule.""" + # Copyright (c) 2018, Ansible Project from __future__ import annotations @@ -56,8 +57,8 @@ class MetaChangeFromDefaultRule(AnsibleLintRule): if "pytest" in sys.modules: - from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports - from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + from ansiblelint.rules import RulesCollection + from ansiblelint.runner import Runner def test_default_galaxy_info( default_rules_collection: RulesCollection, diff --git a/src/ansiblelint/rules/meta_no_tags.py b/src/ansiblelint/rules/meta_no_tags.py index c27a30e..3e9b636 100644 --- a/src/ansiblelint/rules/meta_no_tags.py +++ b/src/ansiblelint/rules/meta_no_tags.py @@ -1,4 +1,5 @@ """Implementation of meta-no-tags rule.""" + from __future__ import annotations import re diff --git a/src/ansiblelint/rules/meta_runtime.md b/src/ansiblelint/rules/meta_runtime.md index 6ed6f17..3e05c59 100644 --- a/src/ansiblelint/rules/meta_runtime.md +++ b/src/ansiblelint/rules/meta_runtime.md @@ -1,26 +1,21 @@ # meta-runtime -This rule checks the meta/runtime.yml `requires_ansible` key against the list of currently supported versions of ansible-core. - -This rule can produce messages such: - -- `requires_ansible` key must be set to a supported version. - -Currently supported versions of ansible-core are: - -- `2.9.10` -- `2.11.x` -- `2.12.x` -- `2.13.x` -- `2.14.x` -- `2.15.x` -- `2.16.x` (in development) +This rule checks the meta/runtime.yml `requires_ansible` key against the list of +currently supported versions of ansible-core. This rule can produce messages such as: -- `meta-runtime[unsupported-version]` - `requires_ansible` key must contain a supported version, shown in the list above. -- `meta-runtime[invalid-version]` - `requires_ansible` key must be a valid version identifier. +- `meta-runtime[unsupported-version]` - `requires_ansible` key must refer to a + currently supported version such as: >=2.14.0, >=2.15.0, >=2.16.0 +- `meta-runtime[invalid-version]` - `requires_ansible` is not a valid + requirement specification +Please note that the linter will allow only a full version of Ansible such +`2.16.0` and not allow their short form, like `2.16`. This is a safety measure +for asking authors to mention an explicit version that they tested with. Over +the years we spotted multiple problems caused by the use of the short versions, +users ended up trying an outdated version that was never tested against by the +collection maintainer. ## Problematic code @@ -30,11 +25,10 @@ This rule can produce messages such as: requires_ansible: ">=2.9" ``` - ```yaml # runtime.yml --- -requires_ansible: "2.9" +requires_ansible: "2.15" ``` ## Correct code @@ -42,5 +36,17 @@ requires_ansible: "2.9" ```yaml # runtime.yml --- -requires_ansible: ">=2.9.10" +requires_ansible: ">=2.15.0" +``` + +## Configuration + +In addition to the internal list of supported Ansible versions, users can +configure additional values. This allows those that want to maintain content +that requires a version of ansible-core that is already out of support. + +```yaml +# Also recognize these versions of Ansible as supported: +supported_ansible_also: + - "2.14" ``` diff --git a/src/ansiblelint/rules/meta_runtime.py b/src/ansiblelint/rules/meta_runtime.py index fed7121..3df2826 100644 --- a/src/ansiblelint/rules/meta_runtime.py +++ b/src/ansiblelint/rules/meta_runtime.py @@ -1,4 +1,5 @@ """Implementation of meta-runtime rule.""" + from __future__ import annotations import sys @@ -22,17 +23,15 @@ class CheckRequiresAnsibleVersion(AnsibleLintRule): id = "meta-runtime" description = ( "The ``requires_ansible`` key in runtime.yml must specify " - "a supported platform version of ansible-core and be a valid version." + "a supported platform version of ansible-core and be a valid version value " + "in x.y.z format." ) severity = "VERY_HIGH" tags = ["metadata"] version_added = "v6.11.0 (last update)" - # Refer to https://access.redhat.com/support/policy/updates/ansible-automation-platform - # Also add devel to this list - supported_ansible = ["2.9.10", "2.11.", "2.12.", "2.13.", "2.14.", "2.15.", "2.16."] _ids = { - "meta-runtime[unsupported-version]": "requires_ansible key must be set to a supported version.", + "meta-runtime[unsupported-version]": "'requires_ansible' key must refer to a currently supported version", "meta-runtime[invalid-version]": "'requires_ansible' is not a valid requirement specification", } @@ -47,22 +46,26 @@ class CheckRequiresAnsibleVersion(AnsibleLintRule): if file.kind != "meta-runtime": return [] - version_required = file.data.get("requires_ansible", None) + requires_ansible = file.data.get("requires_ansible", None) - if version_required: - if not any( - version in version_required for version in self.supported_ansible + if requires_ansible: + if self.options and not any( + version in requires_ansible + for version in self.options.supported_ansible ): + supported_ansible = [f">={x}0" for x in self.options.supported_ansible] + msg = f"'requires_ansible' key must refer to a currently supported version such as: {', '.join(supported_ansible)}" + results.append( self.create_matcherror( - message="requires_ansible key must be set to a supported version.", + message=msg, tag="meta-runtime[unsupported-version]", filename=file, ), ) try: - SpecifierSet(version_required) + SpecifierSet(requires_ansible) except ValueError: results.append( self.create_matcherror( @@ -79,17 +82,18 @@ class CheckRequiresAnsibleVersion(AnsibleLintRule): if "pytest" in sys.modules: import pytest - from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports - from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + # pylint: disable=ungrouped-imports + from ansiblelint.rules import RulesCollection + from ansiblelint.runner import Runner @pytest.mark.parametrize( ("test_file", "failures", "tags"), ( pytest.param( - "examples/meta_runtime_version_checks/pass/meta/runtime.yml", + "examples/meta_runtime_version_checks/pass_0/meta/runtime.yml", 0, "meta-runtime[unsupported-version]", - id="pass", + id="pass0", ), pytest.param( "examples/meta_runtime_version_checks/fail_0/meta/runtime.yml", @@ -111,16 +115,37 @@ if "pytest" in sys.modules: ), ), ) - def test_meta_supported_version( + def test_default_meta_supported_version( default_rules_collection: RulesCollection, test_file: str, failures: int, tags: str, ) -> None: - """Test rule matches.""" + """Test for default supported ansible versions.""" default_rules_collection.register(CheckRequiresAnsibleVersion()) results = Runner(test_file, rules=default_rules_collection).run() for result in results: assert result.rule.id == CheckRequiresAnsibleVersion().id assert result.tag == tags assert len(results) == failures + + @pytest.mark.parametrize( + ("test_file", "failures"), + ( + pytest.param( + "examples/meta_runtime_version_checks/pass_1/meta/runtime.yml", + 0, + id="pass1", + ), + ), + ) + def test_added_meta_supported_version( + default_rules_collection: RulesCollection, + test_file: str, + failures: int, + ) -> None: + """Test for added supported ansible versions in the config.""" + default_rules_collection.register(CheckRequiresAnsibleVersion()) + default_rules_collection.options.supported_ansible_also = ["2.9"] + results = Runner(test_file, rules=default_rules_collection).run() + assert len(results) == failures diff --git a/src/ansiblelint/rules/meta_video_links.py b/src/ansiblelint/rules/meta_video_links.py index 5d4941a..fa19cc6 100644 --- a/src/ansiblelint/rules/meta_video_links.py +++ b/src/ansiblelint/rules/meta_video_links.py @@ -1,4 +1,5 @@ """Implementation of meta-video-links rule.""" + # Copyright (c) 2018, Ansible Project from __future__ import annotations @@ -86,8 +87,9 @@ class MetaVideoLinksRule(AnsibleLintRule): if "pytest" in sys.modules: import pytest - from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports - from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + # pylint: disable=ungrouped-imports + from ansiblelint.rules import RulesCollection + from ansiblelint.runner import Runner @pytest.mark.parametrize( ("test_file", "failures"), diff --git a/src/ansiblelint/rules/name.md b/src/ansiblelint/rules/name.md index 9df4213..0c5080a 100644 --- a/src/ansiblelint/rules/name.md +++ b/src/ansiblelint/rules/name.md @@ -21,12 +21,21 @@ If you want to ignore some of the messages above, you can add any of them to the ## name[prefix] -This rule applies only to included task files that are not named `main.yml`. It -suggests adding the stem of the file as a prefix to the task name. +This rule applies only to included task files that are not named `main.yml` or +are embedded within subdirectories. It suggests adding the stems of the file +path as a prefix to the task name. For example, if you have a task named `Restart server` inside a file named `tasks/deploy.yml`, this rule suggests renaming it to `deploy | Restart server`, -so it would be easier to identify where it comes from. +so it would be easier to identify where it comes from. If the file was named +`tasks/main.yml`, then the rule would have no effect. + +For task files that are embedded within subdirectories, these subdirectories +will also be appended as part of the prefix. For example, if you have a task +named `Terminate server` inside a file named `tasks/foo/destroy.yml`, this rule +suggests renaming it to `foo | destroy | Terminate server`. If the file was +named `tasks/foo/main.yml` then the rule would recommend renaming the task to +`foo | main | Terminate server`. For the moment, this sub-rule is just an **opt-in**, so you need to add it to your `enable_list` to activate it. @@ -59,3 +68,7 @@ your `enable_list` to activate it. - name: Create placeholder file ansible.builtin.command: touch /tmp/.placeholder ``` + +!!! note + + `name[casing]` can be automatically fixed using [`--fix`](../autofix.md) option. diff --git a/src/ansiblelint/rules/name.py b/src/ansiblelint/rules/name.py index 41ce5cb..b814a41 100644 --- a/src/ansiblelint/rules/name.py +++ b/src/ansiblelint/rules/name.py @@ -1,19 +1,23 @@ """Implementation of NameRule.""" + from __future__ import annotations import re import sys -from copy import deepcopy from typing import TYPE_CHECKING, Any +import wcmatch.pathlib +import wcmatch.wcmatch + from ansiblelint.constants import LINE_NUMBER_KEY +from ansiblelint.file_utils import Lintable from ansiblelint.rules import AnsibleLintRule, TransformMixin if TYPE_CHECKING: from ruamel.yaml.comments import CommentedMap, CommentedSeq + from ansiblelint.config import Options from ansiblelint.errors import MatchError - from ansiblelint.file_utils import Lintable from ansiblelint.utils import Task @@ -39,9 +43,11 @@ class NameRule(AnsibleLintRule, TransformMixin): def matchplay(self, file: Lintable, data: dict[str, Any]) -> list[MatchError]: """Return matches found for a specific play (entry in playbook).""" - results = [] + results: list[MatchError] = [] if file.kind != "playbook": return [] + if file.failed(): + return results if "name" not in data: return [ self.create_matcherror( @@ -65,7 +71,9 @@ class NameRule(AnsibleLintRule, TransformMixin): task: Task, file: Lintable | None = None, ) -> list[MatchError]: - results = [] + results: list[MatchError] = [] + if file and file.failed(): + return results name = task.get("name") if not name: results.append( @@ -84,6 +92,7 @@ class NameRule(AnsibleLintRule, TransformMixin): lineno=task[LINE_NUMBER_KEY], ), ) + return results def _prefix_check( @@ -120,10 +129,15 @@ class NameRule(AnsibleLintRule, TransformMixin): # stage one check prefix effective_name = name if self._collection and lintable: - prefix = self._collection.options.task_name_prefix.format( - stem=lintable.path.stem, - ) - if lintable.kind == "tasks" and lintable.path.stem != "main": + full_stem = self._find_full_stem(lintable) + stems = [ + self._collection.options.task_name_prefix.format(stem=stem) + for stem in wcmatch.pathlib.PurePath( + full_stem, + ).parts + ] + prefix = "".join(stems) + if lintable.kind == "tasks" and full_stem != "main": if not name.startswith(prefix): # For the moment in order to raise errors this rule needs to be # enabled manually. Still, we do allow use of prefixes even without @@ -165,6 +179,38 @@ class NameRule(AnsibleLintRule, TransformMixin): ) return results + def _find_full_stem(self, lintable: Lintable) -> str: + lintable_dir = wcmatch.pathlib.PurePath(lintable.dir) + stem = lintable.path.stem + kind = str(lintable.kind) + + stems = [lintable_dir.name] + lintable_dir = lintable_dir.parent + pathex = lintable_dir / stem + glob = "" + + if self.options: + for entry in self.options.kinds: + for key, value in entry.items(): + if kind == key: + glob = value + + while pathex.globmatch( + glob, + flags=( + wcmatch.pathlib.GLOBSTAR + | wcmatch.pathlib.BRACE + | wcmatch.pathlib.DOTGLOB + ), + ): + stems.insert(0, lintable_dir.name) + lintable_dir = lintable_dir.parent + pathex = lintable_dir / stem + + if stems[0].startswith(kind): + del stems[0] + return str(wcmatch.pathlib.PurePath(*stems, stem)) + def transform( self, match: MatchError, @@ -172,17 +218,44 @@ class NameRule(AnsibleLintRule, TransformMixin): data: CommentedMap | CommentedSeq | str, ) -> None: if match.tag == "name[casing]": + + def update_task_name(task_name: str) -> str: + """Capitalize the first work of the task name.""" + # Not using capitalize(), since that rewrites the rest of the name to lower case + if "|" in task_name: # if using prefix + [file_name, update_task_name] = task_name.split("|") + return f"{file_name.strip()} | {update_task_name.strip()[:1].upper()}{update_task_name.strip()[1:]}" + + return f"{task_name[:1].upper()}{task_name[1:]}" + target_task = self.seek(match.yaml_path, data) - # Not using capitalize(), since that rewrites the rest of the name to lower case - target_task[ - "name" - ] = f"{target_task['name'][:1].upper()}{target_task['name'][1:]}" - match.fixed = True + orig_task_name = target_task.get("name", None) + # pylint: disable=too-many-nested-blocks + if orig_task_name: + updated_task_name = update_task_name(orig_task_name) + for item in data: + if isinstance(item, dict) and "tasks" in item: + for task in item["tasks"]: + # We want to rewrite task names in the notify keyword, but + # if there isn't a notify section, there's nothing to do. + if "notify" not in task: + continue + + if ( + isinstance(task["notify"], str) + and orig_task_name == task["notify"] + ): + task["notify"] = updated_task_name + elif isinstance(task["notify"], list): + for idx in range(len(task["notify"])): + if orig_task_name == task["notify"][idx]: + task["notify"][idx] = updated_task_name + + target_task["name"] = updated_task_name + match.fixed = True if "pytest" in sys.modules: - from ansiblelint.config import options - from ansiblelint.file_utils import Lintable # noqa: F811 from ansiblelint.rules import RulesCollection from ansiblelint.runner import Runner @@ -203,11 +276,23 @@ if "pytest" in sys.modules: errs = bad_runner.run() assert len(errs) == 5 - def test_name_prefix_negative() -> None: + def test_name_prefix_positive(config_options: Options) -> None: + """Positive test for name[prefix].""" + config_options.enable_list = ["name[prefix]"] + collection = RulesCollection(options=config_options) + collection.register(NameRule()) + success = Lintable( + "examples/playbooks/tasks/main.yml", + kind="tasks", + ) + good_runner = Runner(success, rules=collection) + results = good_runner.run() + assert len(results) == 0 + + def test_name_prefix_negative(config_options: Options) -> None: """Negative test for name[missing].""" - custom_options = deepcopy(options) - custom_options.enable_list = ["name[prefix]"] - collection = RulesCollection(options=custom_options) + config_options.enable_list = ["name[prefix]"] + collection = RulesCollection(options=config_options) collection.register(NameRule()) failure = Lintable( "examples/playbooks/tasks/rule-name-prefix-fail.yml", @@ -221,6 +306,36 @@ if "pytest" in sys.modules: assert results[1].tag == "name[prefix]" assert results[2].tag == "name[prefix]" + def test_name_prefix_negative_2(config_options: Options) -> None: + """Negative test for name[prefix].""" + config_options.enable_list = ["name[prefix]"] + collection = RulesCollection(options=config_options) + collection.register(NameRule()) + failure = Lintable( + "examples/playbooks/tasks/partial_prefix/foo.yml", + kind="tasks", + ) + bad_runner = Runner(failure, rules=collection) + results = bad_runner.run() + assert len(results) == 2 + assert results[0].tag == "name[prefix]" + assert results[1].tag == "name[prefix]" + + def test_name_prefix_negative_3(config_options: Options) -> None: + """Negative test for name[prefix].""" + config_options.enable_list = ["name[prefix]"] + collection = RulesCollection(options=config_options) + collection.register(NameRule()) + failure = Lintable( + "examples/playbooks/tasks/partial_prefix/main.yml", + kind="tasks", + ) + bad_runner = Runner(failure, rules=collection) + results = bad_runner.run() + assert len(results) == 2 + assert results[0].tag == "name[prefix]" + assert results[1].tag == "name[prefix]" + def test_rule_name_lowercase() -> None: """Negative test for a task that starts with lowercase.""" collection = RulesCollection() @@ -255,6 +370,5 @@ if "pytest" in sys.modules: def test_when_no_lintable() -> None: """Test when lintable is None.""" name_rule = NameRule() - # pylint: disable=protected-access result = name_rule._prefix_check("Foo", None, 1) # noqa: SLF001 assert len(result) == 0 diff --git a/src/ansiblelint/rules/no_changed_when.py b/src/ansiblelint/rules/no_changed_when.py index 28ba427..e71934d 100644 --- a/src/ansiblelint/rules/no_changed_when.py +++ b/src/ansiblelint/rules/no_changed_when.py @@ -1,4 +1,5 @@ """Implementation of the no-changed-when rule.""" + # Copyright (c) 2016 Will Thames <will@thames.id.au> # # Permission is hereby granted, free of charge, to any person obtaining a copy @@ -75,8 +76,9 @@ class CommandHasChangesCheckRule(AnsibleLintRule): if "pytest" in sys.modules: import pytest - from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports - from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + # pylint: disable=ungrouped-imports + from ansiblelint.rules import RulesCollection + from ansiblelint.runner import Runner @pytest.mark.parametrize( ("file", "expected"), diff --git a/src/ansiblelint/rules/no_free_form.md b/src/ansiblelint/rules/no_free_form.md index 0ffc0ac..ae05d0f 100644 --- a/src/ansiblelint/rules/no_free_form.md +++ b/src/ansiblelint/rules/no_free_form.md @@ -56,3 +56,7 @@ This rule can produce messages as: executable: /bin/bash # <-- explicit is better changed_when: false ``` + +!!! note + + This rule can be automatically fixed using [`--fix`](../autofix.md) option. diff --git a/src/ansiblelint/rules/no_free_form.py b/src/ansiblelint/rules/no_free_form.py index e89333b..13489ef 100644 --- a/src/ansiblelint/rules/no_free_form.py +++ b/src/ansiblelint/rules/no_free_form.py @@ -1,20 +1,25 @@ """Implementation of NoFreeFormRule.""" + from __future__ import annotations +import functools import re import sys -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, Any from ansiblelint.constants import INCLUSION_ACTION_NAMES, LINE_NUMBER_KEY -from ansiblelint.rules import AnsibleLintRule +from ansiblelint.rules import AnsibleLintRule, TransformMixin +from ansiblelint.rules.key_order import task_property_sorter if TYPE_CHECKING: + from ruamel.yaml.comments import CommentedMap, CommentedSeq + from ansiblelint.errors import MatchError from ansiblelint.file_utils import Lintable from ansiblelint.utils import Task -class NoFreeFormRule(AnsibleLintRule): +class NoFreeFormRule(AnsibleLintRule, TransformMixin): """Rule for detecting discouraged free-form syntax for action modules.""" id = "no-free-form" @@ -75,7 +80,7 @@ class NoFreeFormRule(AnsibleLintRule): "win_command", "win_shell", ): - if self.cmd_shell_re.match(action_value): + if self.cmd_shell_re.search(action_value): fail = True else: fail = True @@ -89,12 +94,97 @@ class NoFreeFormRule(AnsibleLintRule): ) return results + def transform( + self, + match: MatchError, + lintable: Lintable, + data: CommentedMap | CommentedSeq | str, + ) -> None: + if "no-free-form" in match.tag: + task = self.seek(match.yaml_path, data) + + def filter_values( + val: str, + filter_key: str, + filter_dict: dict[str, Any], + ) -> str: + """Pull out key=value pairs from a string and set them in filter_dict. + + Returns unmatched strings. + """ + if filter_key not in val: + return val + + extra = "" + [k, v] = val.split(filter_key, 1) + if " " in k: + extra, k = k.rsplit(" ", 1) + + if v[0] in "\"'": + # Keep quoted strings together + quote = v[0] + _, v, remainder = v.split(quote, 2) + v = f"{quote}{v}{quote}" + else: + try: + v, remainder = v.split(" ", 1) + except ValueError: + remainder = "" + + filter_dict[k] = v + + extra = " ".join( + (extra, filter_values(remainder, filter_key, filter_dict)), + ) + return extra.strip() + + if match.tag == "no-free-form": + module_opts: dict[str, Any] = {} + for _ in range(len(task)): + k, v = task.popitem(False) + # identify module as key and process its value + if len(k.split(".")) == 3 and isinstance(v, str): + cmd = filter_values(v, "=", module_opts) + if cmd: + module_opts["cmd"] = cmd + + sorted_module_opts = {} + for key in sorted( + module_opts.keys(), + key=functools.cmp_to_key(task_property_sorter), + ): + sorted_module_opts[key] = module_opts[key] + + task[k] = sorted_module_opts + else: + task[k] = v + + match.fixed = True + elif match.tag == "no-free-form[raw]": + exec_key_val: dict[str, Any] = {} + for _ in range(len(task)): + k, v = task.popitem(False) + if isinstance(v, str) and "executable" in v: + # Filter the executable and other parts from the string + task[k] = " ".join( + [ + item + for item in v.split(" ") + if filter_values(item, "=", exec_key_val) + ], + ) + task["args"] = exec_key_val + else: + task[k] = v + match.fixed = True + if "pytest" in sys.modules: import pytest - from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports - from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + # pylint: disable=ungrouped-imports + from ansiblelint.rules import RulesCollection + from ansiblelint.runner import Runner @pytest.mark.parametrize( ("file", "expected"), diff --git a/src/ansiblelint/rules/no_handler.py b/src/ansiblelint/rules/no_handler.py index 380fd61..ae8f820 100644 --- a/src/ansiblelint/rules/no_handler.py +++ b/src/ansiblelint/rules/no_handler.py @@ -69,25 +69,27 @@ class UseHandlerRatherThanWhenChangedRule(AnsibleLintRule): task: Task, file: Lintable | None = None, ) -> bool | str: - if task["__ansible_action_type__"] != "task": + if task["__ansible_action_type__"] != "task" or task.is_handler(): return False when = task.get("when") + result = False if isinstance(when, list): - if len(when) > 1: - return False - return _changed_in_when(when[0]) - if isinstance(when, str): - return _changed_in_when(when) - return False + if len(when) <= 1: + result = _changed_in_when(when[0]) + elif isinstance(when, str): + result = _changed_in_when(when) + return result if "pytest" in sys.modules: import pytest - from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports - from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + # pylint: disable=ungrouped-imports + from ansiblelint.rules import RulesCollection + from ansiblelint.runner import Runner + from ansiblelint.testing import run_ansible_lint @pytest.mark.parametrize( ("test_file", "failures"), @@ -106,3 +108,10 @@ if "pytest" in sys.modules: assert len(results) == failures for result in results: assert result.tag == "no-handler" + + def test_role_with_handler() -> None: + """Test role with handler.""" + role_path = "examples/roles/role_with_handler" + + results = run_ansible_lint("-v", role_path) + assert "no-handler" not in results.stdout diff --git a/src/ansiblelint/rules/no_jinja_when.md b/src/ansiblelint/rules/no_jinja_when.md index 702e807..5a2c736 100644 --- a/src/ansiblelint/rules/no_jinja_when.md +++ b/src/ansiblelint/rules/no_jinja_when.md @@ -30,3 +30,7 @@ anti-pattern and does not produce expected results. ansible.builtin.command: /sbin/shutdown -t now when: ansible_facts['os_family'] == "Debian" # <- Uses facts in a conditional statement. ``` + +!!! note + + This rule can be automatically fixed using [`--fix`](../autofix.md) option. diff --git a/src/ansiblelint/rules/no_jinja_when.py b/src/ansiblelint/rules/no_jinja_when.py index 807081d..a5fc030 100644 --- a/src/ansiblelint/rules/no_jinja_when.py +++ b/src/ansiblelint/rules/no_jinja_when.py @@ -1,19 +1,23 @@ """Implementation of no-jinja-when rule.""" + from __future__ import annotations +import re import sys from typing import TYPE_CHECKING, Any from ansiblelint.constants import LINE_NUMBER_KEY -from ansiblelint.rules import AnsibleLintRule +from ansiblelint.rules import AnsibleLintRule, TransformMixin if TYPE_CHECKING: + from ruamel.yaml.comments import CommentedMap, CommentedSeq + from ansiblelint.errors import MatchError from ansiblelint.file_utils import Lintable from ansiblelint.utils import Task -class NoFormattingInWhenRule(AnsibleLintRule): +class NoFormattingInWhenRule(AnsibleLintRule, TransformMixin): """No Jinja2 in when.""" id = "no-jinja-when" @@ -44,19 +48,19 @@ class NoFormattingInWhenRule(AnsibleLintRule): if isinstance(data, dict): if "roles" not in data or data["roles"] is None: return errors - for role in data["roles"]: + errors = [ + self.create_matcherror( + details=str({"when": role}), + filename=file, + lineno=role[LINE_NUMBER_KEY], + ) + for role in data["roles"] if ( isinstance(role, dict) and "when" in role and not self._is_valid(role["when"]) - ): - errors.append( - self.create_matcherror( - details=str({"when": role}), - filename=file, - lineno=role[LINE_NUMBER_KEY], - ), - ) + ) + ] return errors def matchtask( @@ -66,6 +70,37 @@ class NoFormattingInWhenRule(AnsibleLintRule): ) -> bool | str: return "when" in task.raw_task and not self._is_valid(task.raw_task["when"]) + def transform( + self, + match: MatchError, + lintable: Lintable, + data: CommentedMap | CommentedSeq | str, + ) -> None: + if match.tag == self.id: + task = self.seek(match.yaml_path, data) + key_to_check = ("when", "changed_when", "failed_when") + for _ in range(len(task)): + k, v = task.popitem(False) + if k == "roles" and isinstance(v, list): + transform_for_roles(v, key_to_check=key_to_check) + elif k in key_to_check: + v = re.sub(r"{{ (.*?) }}", r"\1", v) + task[k] = v + match.fixed = True + + +def transform_for_roles(v: list[Any], key_to_check: tuple[str, ...]) -> None: + """Additional transform logic in case of roles.""" + for idx, new_dict in enumerate(v): + for new_key, new_value in new_dict.items(): + if new_key in key_to_check: + if isinstance(new_value, list): + for index, nested_value in enumerate(new_value): + new_value[index] = re.sub(r"{{ (.*?) }}", r"\1", nested_value) + v[idx][new_key] = new_value + if isinstance(new_value, str): + v[idx][new_key] = re.sub(r"{{ (.*?) }}", r"\1", new_value) + if "pytest" in sys.modules: # Tests for no-jinja-when rule. diff --git a/src/ansiblelint/rules/no_log_password.md b/src/ansiblelint/rules/no_log_password.md index 579dd11..3629ef6 100644 --- a/src/ansiblelint/rules/no_log_password.md +++ b/src/ansiblelint/rules/no_log_password.md @@ -43,3 +43,7 @@ Explicitly adding `no_log: true` prevents accidentally exposing secrets. - wow no_log: true # <- Sets the no_log attribute to a non-false value. ``` + +!!! note + + This rule can be automatically fixed using [`--fix`](../autofix.md) option. diff --git a/src/ansiblelint/rules/no_log_password.py b/src/ansiblelint/rules/no_log_password.py index 7cc7439..c3f6d34 100644 --- a/src/ansiblelint/rules/no_log_password.py +++ b/src/ansiblelint/rules/no_log_password.py @@ -15,17 +15,25 @@ """NoLogPasswordsRule used with ansible-lint.""" from __future__ import annotations +import os import sys +from pathlib import Path from typing import TYPE_CHECKING -from ansiblelint.rules import AnsibleLintRule +from ansiblelint.rules import AnsibleLintRule, RulesCollection, TransformMixin +from ansiblelint.runner import get_matches +from ansiblelint.transformer import Transformer from ansiblelint.utils import Task, convert_to_boolean if TYPE_CHECKING: + from ruamel.yaml.comments import CommentedMap, CommentedSeq + + from ansiblelint.config import Options + from ansiblelint.errors import MatchError from ansiblelint.file_utils import Lintable -class NoLogPasswordsRule(AnsibleLintRule): +class NoLogPasswordsRule(AnsibleLintRule, TransformMixin): """Password should not be logged.""" id = "no-log-password" @@ -72,12 +80,26 @@ class NoLogPasswordsRule(AnsibleLintRule): has_password and not convert_to_boolean(no_log) and len(has_loop) > 0, ) + def transform( + self, + match: MatchError, + lintable: Lintable, + data: CommentedMap | CommentedSeq | str, + ) -> None: + if match.tag == self.id: + task = self.seek(match.yaml_path, data) + task["no_log"] = True + + match.fixed = True + if "pytest" in sys.modules: + from unittest import mock + import pytest if TYPE_CHECKING: - from ansiblelint.testing import RunFromText # pylint: disable=ungrouped-imports + from ansiblelint.testing import RunFromText NO_LOG_UNUSED = """ - name: Test @@ -304,3 +326,33 @@ if "pytest" in sys.modules: """The task does not actually lock the user.""" results = rule_runner.run_playbook(PASSWORD_LOCK_FALSE) assert len(results) == 0 + + @mock.patch.dict(os.environ, {"ANSIBLE_LINT_WRITE_TMP": "1"}, clear=True) + def test_no_log_password_transform( + config_options: Options, + ) -> None: + """Test transform functionality for no-log-password rule.""" + playbook = Path("examples/playbooks/transform-no-log-password.yml") + config_options.write_list = ["all"] + rules = RulesCollection(options=config_options) + rules.register(NoLogPasswordsRule()) + + config_options.lintables = [str(playbook)] + runner_result = get_matches(rules=rules, options=config_options) + transformer = Transformer(result=runner_result, options=config_options) + transformer.run() + + matches = runner_result.matches + assert len(matches) == 2 + + orig_content = playbook.read_text(encoding="utf-8") + expected_content = playbook.with_suffix( + f".transformed{playbook.suffix}", + ).read_text(encoding="utf-8") + transformed_content = playbook.with_suffix(f".tmp{playbook.suffix}").read_text( + encoding="utf-8", + ) + + assert orig_content != transformed_content + assert expected_content == transformed_content + playbook.with_suffix(f".tmp{playbook.suffix}").unlink() diff --git a/src/ansiblelint/rules/no_prompting.py b/src/ansiblelint/rules/no_prompting.py index 6622771..c5d11d8 100644 --- a/src/ansiblelint/rules/no_prompting.py +++ b/src/ansiblelint/rules/no_prompting.py @@ -1,4 +1,5 @@ """Implementation of no-prompting rule.""" + from __future__ import annotations import sys @@ -8,6 +9,7 @@ from ansiblelint.constants import LINE_NUMBER_KEY from ansiblelint.rules import AnsibleLintRule if TYPE_CHECKING: + from ansiblelint.config import Options from ansiblelint.errors import MatchError from ansiblelint.file_utils import Lintable from ansiblelint.utils import Task @@ -32,7 +34,7 @@ class NoPromptingRule(AnsibleLintRule): if file.kind != "playbook": # pragma: no cover return [] - vars_prompt = data.get("vars_prompt", None) + vars_prompt = data.get("vars_prompt") if not vars_prompt: return [] return [ @@ -60,15 +62,14 @@ class NoPromptingRule(AnsibleLintRule): if "pytest" in sys.modules: - from ansiblelint.config import options - from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports - from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + from ansiblelint.rules import RulesCollection + from ansiblelint.runner import Runner - def test_no_prompting_fail() -> None: + def test_no_prompting_fail(config_options: Options) -> None: """Negative test for no-prompting.""" # For testing we want to manually enable opt-in rules - options.enable_list = ["no-prompting"] - rules = RulesCollection(options=options) + config_options.enable_list = ["no-prompting"] + rules = RulesCollection(options=config_options) rules.register(NoPromptingRule()) results = Runner("examples/playbooks/rule-no-prompting.yml", rules=rules).run() assert len(results) == 2 diff --git a/src/ansiblelint/rules/no_relative_paths.py b/src/ansiblelint/rules/no_relative_paths.py index 470b1b8..de22641 100644 --- a/src/ansiblelint/rules/no_relative_paths.py +++ b/src/ansiblelint/rules/no_relative_paths.py @@ -1,4 +1,5 @@ """Implementation of no-relative-paths rule.""" + # Copyright (c) 2016, Tsukinowa Inc. <info@tsukinowa.jp> # Copyright (c) 2018, Ansible Project @@ -53,8 +54,9 @@ class RoleRelativePath(AnsibleLintRule): if "pytest" in sys.modules: import pytest - from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports - from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + # pylint: disable=ungrouped-imports + from ansiblelint.rules import RulesCollection + from ansiblelint.runner import Runner @pytest.mark.parametrize( ("test_file", "failures"), diff --git a/src/ansiblelint/rules/no_same_owner.py b/src/ansiblelint/rules/no_same_owner.py index 021900e..23290e0 100644 --- a/src/ansiblelint/rules/no_same_owner.py +++ b/src/ansiblelint/rules/no_same_owner.py @@ -1,4 +1,5 @@ """Optional rule for avoiding keeping owner/group when transferring files.""" + from __future__ import annotations import re @@ -84,8 +85,9 @@ should not be preserved when transferring files between them. if "pytest" in sys.modules: import pytest - from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports - from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + # pylint: disable=ungrouped-imports + from ansiblelint.rules import RulesCollection + from ansiblelint.runner import Runner @pytest.mark.parametrize( ("test_file", "failures"), diff --git a/src/ansiblelint/rules/no_tabs.py b/src/ansiblelint/rules/no_tabs.py index c53f1bb..2614a1a 100644 --- a/src/ansiblelint/rules/no_tabs.py +++ b/src/ansiblelint/rules/no_tabs.py @@ -1,4 +1,5 @@ """Implementation of no-tabs rule.""" + # Copyright (c) 2016, Will Thames and contributors # Copyright (c) 2018, Ansible Project from __future__ import annotations @@ -7,6 +8,7 @@ import sys from typing import TYPE_CHECKING from ansiblelint.rules import AnsibleLintRule +from ansiblelint.text import has_jinja from ansiblelint.yaml_utils import nested_items_path if TYPE_CHECKING: @@ -27,6 +29,10 @@ class NoTabsRule(AnsibleLintRule): ("lineinfile", "insertbefore"), ("lineinfile", "regexp"), ("lineinfile", "line"), + ("win_lineinfile", "insertafter"), + ("win_lineinfile", "insertbefore"), + ("win_lineinfile", "regexp"), + ("win_lineinfile", "line"), ("ansible.builtin.lineinfile", "insertafter"), ("ansible.builtin.lineinfile", "insertbefore"), ("ansible.builtin.lineinfile", "regexp"), @@ -35,6 +41,10 @@ class NoTabsRule(AnsibleLintRule): ("ansible.legacy.lineinfile", "insertbefore"), ("ansible.legacy.lineinfile", "regexp"), ("ansible.legacy.lineinfile", "line"), + ("community.windows.win_lineinfile", "insertafter"), + ("community.windows.win_lineinfile", "insertbefore"), + ("community.windows.win_lineinfile", "regexp"), + ("community.windows.win_lineinfile", "line"), ] def matchtask( @@ -44,17 +54,22 @@ class NoTabsRule(AnsibleLintRule): ) -> bool | str: action = task["action"]["__ansible_module__"] for k, v, _ in nested_items_path(task): - if isinstance(k, str) and "\t" in k: + if isinstance(k, str) and "\t" in k and not has_jinja(k): return True - if isinstance(v, str) and "\t" in v and (action, k) not in self.allow_list: + if ( + isinstance(v, str) + and "\t" in v + and (action, k) not in self.allow_list + and not has_jinja(v) + ): return True return False # testing code to be loaded only with pytest or when executed the rule file if "pytest" in sys.modules: - from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports - from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + from ansiblelint.rules import RulesCollection + from ansiblelint.runner import Runner def test_no_tabs_rule(default_rules_collection: RulesCollection) -> None: """Test rule matches.""" @@ -62,6 +77,12 @@ if "pytest" in sys.modules: "examples/playbooks/rule-no-tabs.yml", rules=default_rules_collection, ).run() - assert results[0].lineno == 10 - assert results[0].message == NoTabsRule().shortdesc - assert len(results) == 2 + expected_results = [ + (10, NoTabsRule().shortdesc), + (13, NoTabsRule().shortdesc), + ] + for i, expected in enumerate(expected_results): + assert len(results) >= i + 1 + assert results[i].lineno == expected[0] + assert results[i].message == expected[1] + assert len(results) == len(expected), results diff --git a/src/ansiblelint/rules/only_builtins.py b/src/ansiblelint/rules/only_builtins.py index 78ad93a..3757af8 100644 --- a/src/ansiblelint/rules/only_builtins.py +++ b/src/ansiblelint/rules/only_builtins.py @@ -1,11 +1,11 @@ """Rule definition for usage of builtin actions only.""" + from __future__ import annotations import os import sys from typing import TYPE_CHECKING -from ansiblelint.config import options from ansiblelint.rules import AnsibleLintRule from ansiblelint.rules.fqcn import builtins from ansiblelint.skip_utils import is_nested_task @@ -33,9 +33,11 @@ class OnlyBuiltinsRule(AnsibleLintRule): allowed_collections = [ "ansible.builtin", "ansible.legacy", - *options.only_builtins_allow_collections, ] - allowed_modules = builtins + options.only_builtins_allow_modules + allowed_modules = builtins + if self.options: + allowed_collections += self.options.only_builtins_allow_collections + allowed_modules += self.options.only_builtins_allow_modules is_allowed = ( any(module.startswith(f"{prefix}.") for prefix in allowed_collections) diff --git a/src/ansiblelint/rules/package_latest.md b/src/ansiblelint/rules/package_latest.md index c7e0d82..c965548 100644 --- a/src/ansiblelint/rules/package_latest.md +++ b/src/ansiblelint/rules/package_latest.md @@ -7,7 +7,7 @@ In production environments, you should set `state` to `present` and specify a ta Setting `state` to `latest` not only installs software, it performs an update and installs additional packages. This can result in performance degradation or loss of service. -If you do want to update packages to the latest version, you should also set the `update_only` parameter to `true` to avoid installing additional packages. +If you do want to update packages to the latest version, you should also set the `update_only` or `only_upgrade` parameter to `true` based on package manager to avoid installing additional packages. ## Problematic Code @@ -32,11 +32,17 @@ If you do want to update packages to the latest version, you should also set the name: some-package state: latest # <- Installs the latest package. - - name: Install Ansible with update_only to false + - name: Install sudo with update_only to false ansible.builtin.yum: name: sudo state: latest update_only: false # <- Updates and installs packages. + + - name: Install sudo with only_upgrade to false + ansible.builtin.apt: + name: sudo + state: latest + only_upgrade: false # <- Upgrades and installs packages ``` ## Correct Code @@ -63,9 +69,15 @@ If you do want to update packages to the latest version, you should also set the name: some-package state: present # <- Ensures the package is installed. - - name: Update Ansible with update_only to true + - name: Update sudo with update_only to true ansible.builtin.yum: name: sudo state: latest update_only: true # <- Updates but does not install additional packages. + + - name: Install sudo with only_upgrade to true + ansible.builtin.apt: + name: sudo + state: latest + only_upgrade: true # <- Upgrades but does not install additional packages. ``` diff --git a/src/ansiblelint/rules/package_latest.py b/src/ansiblelint/rules/package_latest.py index a00a540..9c8ce3c 100644 --- a/src/ansiblelint/rules/package_latest.py +++ b/src/ansiblelint/rules/package_latest.py @@ -1,4 +1,5 @@ """Implementations of the package-latest rule.""" + # Copyright (c) 2016 Will Thames <will@thames.id.au> # # Permission is hereby granted, free of charge, to any person obtaining a copy @@ -79,5 +80,6 @@ class PackageIsNotLatestRule(AnsibleLintRule): task["action"]["__ansible_module__"] in self._package_managers and not task["action"].get("version") and not task["action"].get("update_only") + and not task["action"].get("only_upgrade") and task["action"].get("state") == "latest" ) diff --git a/src/ansiblelint/rules/partial_become.md b/src/ansiblelint/rules/partial_become.md index 01f9dae..672ef96 100644 --- a/src/ansiblelint/rules/partial_become.md +++ b/src/ansiblelint/rules/partial_become.md @@ -5,6 +5,13 @@ This rule checks that privilege escalation is activated when changing users. To perform an action as a different user with the `become_user` directive, you must set `become: true`. +This rule can produce the following messages: + +- `partial-become[play]`: become_user requires become to work as expected, at + play level. +- `partial-become[task]`: become_user requires become to work as expected, at + task level. + !!! warning While Ansible inherits have of `become` and `become_user` from upper levels, @@ -19,12 +26,13 @@ must set `become: true`. --- - name: Example playbook hosts: localhost + become: true # <- Activates privilege escalation. tasks: - name: Start the httpd service as the apache user ansible.builtin.service: name: httpd state: started - become_user: apache # <- Does not change the user because "become: true" is not set. + become_user: apache # <- Does not change the user because "become: true" is not set. ``` ## Correct Code @@ -37,6 +45,82 @@ must set `become: true`. ansible.builtin.service: name: httpd state: started - become: true # <- Activates privilege escalation. - become_user: apache # <- Changes the user with the desired privileges. + become: true # <- Activates privilege escalation. + become_user: apache # <- Changes the user with the desired privileges. + +# Stand alone playbook alternative, applies to all tasks + +- name: Example playbook + hosts: localhost + become: true # <- Activates privilege escalation. + become_user: apache # <- Changes the user with the desired privileges. + tasks: + - name: Start the httpd service as the apache user + ansible.builtin.service: + name: httpd + state: started +``` + +## Problematic Code + +```yaml +--- +- name: Example playbook 1 + hosts: localhost + become: true # <- Activates privilege escalation. + tasks: + - name: Include a task file + ansible.builtin.include_tasks: tasks.yml ``` + +```yaml +--- +- name: Example playbook 2 + hosts: localhost + tasks: + - name: Include a task file + ansible.builtin.include_tasks: tasks.yml +``` + +```yaml +# tasks.yml +- name: Start the httpd service as the apache user + ansible.builtin.service: + name: httpd + state: started + become_user: apache # <- Does not change the user because "become: true" is not set. +``` + +## Correct Code + +```yaml +--- +- name: Example playbook 1 + hosts: localhost + tasks: + - name: Include a task file + ansible.builtin.include_tasks: tasks.yml +``` + +```yaml +--- +- name: Example playbook 2 + hosts: localhost + tasks: + - name: Include a task file + ansible.builtin.include_tasks: tasks.yml +``` + +```yaml +# tasks.yml +- name: Start the httpd service as the apache user + ansible.builtin.service: + name: httpd + state: started + become: true # <- Activates privilege escalation. + become_user: apache # <- Does not change the user because "become: true" is not set. +``` + +!!! note + + This rule can be automatically fixed using [`--fix`](../autofix.md) option. diff --git a/src/ansiblelint/rules/partial_become.py b/src/ansiblelint/rules/partial_become.py index d14c06f..879b186 100644 --- a/src/ansiblelint/rules/partial_become.py +++ b/src/ansiblelint/rules/partial_become.py @@ -1,4 +1,5 @@ """Implementation of partial-become rule.""" + # Copyright (c) 2016 Will Thames <will@thames.id.au> # # Permission is hereby granted, free of charge, to any person obtaining a copy @@ -21,115 +22,231 @@ from __future__ import annotations import sys -from functools import reduce from typing import TYPE_CHECKING, Any +from ruamel.yaml.comments import CommentedMap, CommentedSeq + from ansiblelint.constants import LINE_NUMBER_KEY -from ansiblelint.rules import AnsibleLintRule +from ansiblelint.rules import AnsibleLintRule, TransformMixin if TYPE_CHECKING: + from collections.abc import Iterator + from ansiblelint.errors import MatchError from ansiblelint.file_utils import Lintable + from ansiblelint.utils import Task -def _get_subtasks(data: dict[str, Any]) -> list[Any]: - result: list[Any] = [] - block_names = [ - "tasks", - "pre_tasks", - "post_tasks", - "handlers", - "block", - "always", - "rescue", - ] - for name in block_names: - if data and name in data: - result += data[name] or [] - return result - - -def _nested_search(term: str, data: dict[str, Any]) -> Any: - if data and term in data: - return True - return reduce( - (lambda x, y: x or _nested_search(term, y)), - _get_subtasks(data), - False, - ) - - -def _become_user_without_become(becomeuserabove: bool, data: dict[str, Any]) -> Any: - if "become" in data: - # If become is in lineage of tree then correct - return False - if "become_user" in data and _nested_search("become", data): - # If 'become_user' on tree and become somewhere below - # we must check for a case of a second 'become_user' without a - # 'become' in its lineage - subtasks = _get_subtasks(data) - return reduce( - (lambda x, y: x or _become_user_without_become(False, y)), - subtasks, - False, - ) - if _nested_search("become_user", data): - # Keep searching down if 'become_user' exists in the tree below current task - subtasks = _get_subtasks(data) - return len(subtasks) == 0 or reduce( - ( - lambda x, y: x - or _become_user_without_become( - becomeuserabove or "become_user" in data, - y, - ) - ), - subtasks, - False, - ) - # If at bottom of tree, flag up if 'become_user' existed in the lineage of the tree and - # 'become' was not. This is an error if any lineage has a 'become_user' but no become - return becomeuserabove - - -class BecomeUserWithoutBecomeRule(AnsibleLintRule): - """become_user requires become to work as expected.""" +class BecomeUserWithoutBecomeRule(AnsibleLintRule, TransformMixin): + """``become_user`` should have a corresponding ``become`` at the play or task level.""" id = "partial-become" - description = "``become_user`` without ``become`` will not actually change user" + description = "``become_user`` should have a corresponding ``become`` at the play or task level." severity = "VERY_HIGH" tags = ["unpredictability"] version_added = "historic" - def matchplay(self, file: Lintable, data: dict[str, Any]) -> list[MatchError]: - if file.kind == "playbook": - result = _become_user_without_become(False, data) - if result: - return [ - self.create_matcherror( - message=self.shortdesc, - filename=file, - lineno=data[LINE_NUMBER_KEY], - ), - ] - return [] + def matchplay( + self: BecomeUserWithoutBecomeRule, + file: Lintable, + data: dict[str, Any], + ) -> list[MatchError]: + """Match become_user without become in play. + + :param file: The file to lint. + :param data: The data to lint (play) + :returns: A list of errors. + """ + if file.kind != "playbook": + return [] + errors = [] + partial = "become_user" in data and "become" not in data + if partial: + error = self.create_matcherror( + message=self.shortdesc, + filename=file, + tag=f"{self.id}[play]", + lineno=data[LINE_NUMBER_KEY], + ) + errors.append(error) + return errors + + def matchtask( + self: BecomeUserWithoutBecomeRule, + task: Task, + file: Lintable | None = None, + ) -> list[MatchError]: + """Match become_user without become in task. + + :param task: The task to lint. + :param file: The file to lint. + :returns: A list of errors. + """ + data = task.normalized_task + errors = [] + partial = "become_user" in data and "become" not in data + if partial: + error = self.create_matcherror( + message=self.shortdesc, + filename=file, + tag=f"{self.id}[task]", + lineno=task[LINE_NUMBER_KEY], + ) + errors.append(error) + return errors + + def _dive(self: BecomeUserWithoutBecomeRule, data: CommentedSeq) -> Iterator[Any]: + """Dive into the data and yield each item. + + :param data: The data to dive into. + :yield: Each item in the data. + """ + for item in data: + for nested in ("block", "rescue", "always"): + if nested in item: + yield from self._dive(item[nested]) + yield item + + def transform( + self: BecomeUserWithoutBecomeRule, + match: MatchError, + lintable: Lintable, + data: CommentedMap | CommentedSeq | str, + ) -> None: + """Transform the data. + + :param match: The match to transform. + :param lintable: The file to transform. + :param data: The data to transform. + """ + if not isinstance(data, CommentedSeq): + return + + obj = self.seek(match.yaml_path, data) + if "become" in obj and "become_user" in obj: + match.fixed = True + return + if "become" not in obj and "become_user" not in obj: + match.fixed = True + return + + self._transform_plays(plays=data) + + if "become" in obj and "become_user" in obj: + match.fixed = True + return + if "become" not in obj and "become_user" not in obj: + match.fixed = True + return + + def is_ineligible_for_transform( + self: BecomeUserWithoutBecomeRule, + data: CommentedMap, + ) -> bool: + """Check if the data is eligible for transformation. + + :param data: The data to check. + :returns: True if ineligible, False otherwise. + """ + if any("include" in key for key in data): + return True + if "notify" in data: + return True + return False + + def _transform_plays(self, plays: CommentedSeq) -> None: + """Transform the plays. + + :param plays: The plays to transform. + """ + for play in plays: + self._transform_play(play=play) + + def _transform_play(self, play: CommentedMap) -> None: + """Transform the play. + + :param play: The play to transform. + """ + # Ensure we have no includes in this play + task_groups = ("tasks", "pre_tasks", "post_tasks", "handlers") + for task_group in task_groups: + tasks = self._dive(play.get(task_group, [])) + for task in tasks: + if self.is_ineligible_for_transform(task): + return + remove_play_become_user = False + for task_group in task_groups: + tasks = self._dive(play.get(task_group, [])) + for task in tasks: + b_in_t = "become" in task + bu_in_t = "become_user" in task + b_in_p = "become" in play + bu_in_p = "become_user" in play + if b_in_t and not bu_in_t and bu_in_p: + # Preserve the end comment if become is the last key + comment = None + if list(task.keys())[-1] == "become" and "become" in task.ca.items: + comment = task.ca.items.pop("become") + become_index = list(task.keys()).index("become") + task.insert(become_index + 1, "become_user", play["become_user"]) + if comment: + self._attach_comment_end(task, comment) + remove_play_become_user = True + if bu_in_t and not b_in_t and b_in_p: + become_user_index = list(task.keys()).index("become_user") + task.insert(become_user_index, "become", play["become"]) + if bu_in_t and not b_in_t and not b_in_p: + # Preserve the end comment if become_user is the last key + comment = None + if ( + list(task.keys())[-1] == "become_user" + and "become_user" in task.ca.items + ): + comment = task.ca.items.pop("become_user") + task.pop("become_user") + if comment: + self._attach_comment_end(task, comment) + if remove_play_become_user: + del play["become_user"] + + def _attach_comment_end( + self, + obj: CommentedMap | CommentedSeq, + comment: Any, + ) -> None: + """Attach a comment to the end of the object. + + :param obj: The object to attach the comment to. + :param comment: The comment to attach. + """ + if isinstance(obj, CommentedMap): + last = list(obj.keys())[-1] + if not isinstance(obj[last], CommentedSeq | CommentedMap): + obj.ca.items[last] = comment + return + self._attach_comment_end(obj[last], comment) + elif isinstance(obj, CommentedSeq): + if not isinstance(obj[-1], CommentedSeq | CommentedMap): + obj.ca.items[len(obj)] = comment + return + self._attach_comment_end(obj[-1], comment) # testing code to be loaded only with pytest or when executed the rule file if "pytest" in sys.modules: - from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports - from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + from ansiblelint.rules import RulesCollection + from ansiblelint.runner import Runner - def test_partial_become_positive() -> None: - """Positive test for partial-become.""" + def test_partial_become_pass() -> None: + """No errors found for partial-become.""" collection = RulesCollection() collection.register(BecomeUserWithoutBecomeRule()) success = "examples/playbooks/rule-partial-become-without-become-pass.yml" good_runner = Runner(success, rules=collection) assert [] == good_runner.run() - def test_partial_become_negative() -> None: - """Negative test for partial-become.""" + def test_partial_become_fail() -> None: + """Errors found for partial-become.""" collection = RulesCollection() collection.register(BecomeUserWithoutBecomeRule()) failure = "examples/playbooks/rule-partial-become-without-become-fail.yml" diff --git a/src/ansiblelint/rules/playbook_extension.py b/src/ansiblelint/rules/playbook_extension.py index b4ca41c..a08c984 100644 --- a/src/ansiblelint/rules/playbook_extension.py +++ b/src/ansiblelint/rules/playbook_extension.py @@ -1,4 +1,5 @@ """Implementation of playbook-extension rule.""" + # Copyright (c) 2016, Tsukinowa Inc. <info@tsukinowa.jp> # Copyright (c) 2018, Ansible Project from __future__ import annotations @@ -39,7 +40,8 @@ class PlaybookExtensionRule(AnsibleLintRule): if "pytest" in sys.modules: import pytest - from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports + # pylint: disable=ungrouped-imports + from ansiblelint.rules import RulesCollection @pytest.mark.parametrize( ("file", "expected"), diff --git a/src/ansiblelint/rules/risky_file_permissions.md b/src/ansiblelint/rules/risky_file_permissions.md index 2a62a6d..ad46871 100644 --- a/src/ansiblelint/rules/risky_file_permissions.md +++ b/src/ansiblelint/rules/risky_file_permissions.md @@ -50,7 +50,7 @@ Modules that are checked: - name: Safe example of using ini_file (2nd solution) community.general.ini_file: path: foo - mode: 0600 # explicitly sets the desired permissions, to make the results predictable + mode: "0600" # explicitly sets the desired permissions, to make the results predictable - name: Safe example of using copy (3rd solution) ansible.builtin.copy: diff --git a/src/ansiblelint/rules/risky_file_permissions.py b/src/ansiblelint/rules/risky_file_permissions.py index f4494eb..7fe3870 100644 --- a/src/ansiblelint/rules/risky_file_permissions.py +++ b/src/ansiblelint/rules/risky_file_permissions.py @@ -137,8 +137,9 @@ class MissingFilePermissionsRule(AnsibleLintRule): if "pytest" in sys.modules: import pytest - from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports - from ansiblelint.testing import RunFromText # pylint: disable=ungrouped-imports + # pylint: disable=ungrouped-imports + from ansiblelint.rules import RulesCollection + from ansiblelint.testing import RunFromText @pytest.mark.parametrize( ("file", "expected"), diff --git a/src/ansiblelint/rules/risky_octal.py b/src/ansiblelint/rules/risky_octal.py index e3651ea..e3dad38 100644 --- a/src/ansiblelint/rules/risky_octal.py +++ b/src/ansiblelint/rules/risky_octal.py @@ -1,4 +1,5 @@ """Implementation of risky-octal rule.""" + # Copyright (c) 2013-2014 Will Thames <will@thames.id.au> # # Permission is hereby granted, free of charge, to any person obtaining a copy diff --git a/src/ansiblelint/rules/risky_shell_pipe.md b/src/ansiblelint/rules/risky_shell_pipe.md index 302d0d9..dfede8e 100644 --- a/src/ansiblelint/rules/risky_shell_pipe.md +++ b/src/ansiblelint/rules/risky_shell_pipe.md @@ -7,7 +7,7 @@ The return status of a pipeline is the exit status of the command. The `pipefail` option ensures that tasks fail as expected if the first command fails. -As this requirement does apply to PowerShell, for shell commands that have +As this requirement does not apply to PowerShell, for shell commands that have `pwsh` inside `executable` attribute, this rule will not trigger. ## Problematic Code @@ -30,10 +30,14 @@ As this requirement does apply to PowerShell, for shell commands that have become: false tasks: - name: Pipeline with pipefail - ansible.builtin.shell: set -o pipefail && false | cat + ansible.builtin.shell: + cmd: set -o pipefail && false | cat + executable: /bin/bash - name: Pipeline with pipefail, multi-line - ansible.builtin.shell: | - set -o pipefail # <-- adding this will prevent surprises - false | cat + ansible.builtin.shell: + cmd: | + set -o pipefail # <-- adding this will prevent surprises + false | cat + executable: /bin/bash ``` diff --git a/src/ansiblelint/rules/risky_shell_pipe.py b/src/ansiblelint/rules/risky_shell_pipe.py index 58a6f5f..b0c6063 100644 --- a/src/ansiblelint/rules/risky_shell_pipe.py +++ b/src/ansiblelint/rules/risky_shell_pipe.py @@ -1,4 +1,5 @@ """Implementation of risky-shell-pipe rule.""" + from __future__ import annotations import re @@ -62,8 +63,9 @@ class ShellWithoutPipefail(AnsibleLintRule): if "pytest" in sys.modules: import pytest - from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports - from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + # pylint: disable=ungrouped-imports + from ansiblelint.rules import RulesCollection + from ansiblelint.runner import Runner @pytest.mark.parametrize( ("file", "expected"), diff --git a/src/ansiblelint/rules/role_name.py b/src/ansiblelint/rules/role_name.py index 499c086..ebe0b1a 100644 --- a/src/ansiblelint/rules/role_name.py +++ b/src/ansiblelint/rules/role_name.py @@ -1,4 +1,5 @@ """Implementation of role-name rule.""" + # Copyright (c) 2020 Gael Chamoulaud <gchamoul@redhat.com> # Copyright (c) 2020 Sorin Sbarnea <ssbarnea@redhat.com> # @@ -94,6 +95,26 @@ class RoleNames(AnsibleLintRule): if file.kind not in ("meta", "role", "playbook"): return result + if file.kind == "meta": + for role in file.data.get("dependencies", []): + if isinstance(role, dict): + role_name = role["role"] + elif isinstance(role, str): + role_name = role + else: + msg = "Role dependency has unexpected type." + raise TypeError(msg) + if "/" in role_name: + result.append( + self.create_matcherror( + f"Avoid using paths when importing roles. ({role_name})", + filename=file, + lineno=role_name.ansible_pos[1], + tag=f"{self.id}[path]", + ), + ) + return result + if file.kind == "playbook": for play in file.data: if "roles" in play: @@ -143,7 +164,7 @@ class RoleNames(AnsibleLintRule): if meta_data: try: return str(meta_data["galaxy_info"]["role_name"]) - except KeyError: + except (KeyError, TypeError): pass return default @@ -151,8 +172,9 @@ class RoleNames(AnsibleLintRule): if "pytest" in sys.modules: import pytest - from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports - from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + # pylint: disable=ungrouped-imports + from ansiblelint.rules import RulesCollection + from ansiblelint.runner import Runner @pytest.mark.parametrize( ("test_file", "failure"), @@ -168,3 +190,44 @@ if "pytest" in sys.modules: for result in results: assert result.tag == "role-name[path]" assert len(results) == failure + + @pytest.mark.parametrize( + ("test_file", "failure"), + (pytest.param("examples/roles/role_with_deps_paths", 3, id="fail"),), + ) + def test_role_deps_path_names( + default_rules_collection: RulesCollection, + test_file: str, + failure: int, + ) -> None: + """Test rule matches.""" + results = Runner( + test_file, + rules=default_rules_collection, + ).run() + expected_errors = ( + ("role-name[path]", 3), + ("role-name[path]", 9), + ("role-name[path]", 10), + ) + assert len(expected_errors) == failure + for idx, result in enumerate(results): + assert result.tag == expected_errors[idx][0] + assert result.lineno == expected_errors[idx][1] + assert len(results) == failure + + @pytest.mark.parametrize( + ("test_file", "failure"), + (pytest.param("examples/roles/test-no-deps-role", 0, id="no_deps"),), + ) + def test_role_no_deps( + default_rules_collection: RulesCollection, + test_file: str, + failure: int, + ) -> None: + """Test role if no dependencies are present in meta/main.yml.""" + results = Runner( + test_file, + rules=default_rules_collection, + ).run() + assert len(results) == failure diff --git a/src/ansiblelint/rules/run_once.py b/src/ansiblelint/rules/run_once.py index 78968b6..d656711 100644 --- a/src/ansiblelint/rules/run_once.py +++ b/src/ansiblelint/rules/run_once.py @@ -1,4 +1,5 @@ """Optional Ansible-lint rule to warn use of run_once with strategy free.""" + from __future__ import annotations import sys @@ -34,7 +35,7 @@ class RunOnce(AnsibleLintRule): if not file or file.kind != "playbook" or not data: return [] - strategy = data.get("strategy", None) + strategy = data.get("strategy") run_once = data.get("run_once", False) if (not strategy and not run_once) or strategy != "free": return [] @@ -43,7 +44,6 @@ class RunOnce(AnsibleLintRule): message="Play uses strategy: free", filename=file, tag=f"{self.id}[play]", - # pylint: disable=protected-access lineno=strategy._line_number, # noqa: SLF001 ), ] @@ -74,8 +74,9 @@ class RunOnce(AnsibleLintRule): if "pytest" in sys.modules: import pytest - from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports - from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + # pylint: disable=ungrouped-imports + from ansiblelint.rules import RulesCollection + from ansiblelint.runner import Runner @pytest.mark.parametrize( ("test_file", "failure"), diff --git a/src/ansiblelint/rules/sanity.md b/src/ansiblelint/rules/sanity.md index 5b4f3a4..f17cdaf 100644 --- a/src/ansiblelint/rules/sanity.md +++ b/src/ansiblelint/rules/sanity.md @@ -1,10 +1,10 @@ # sanity This rule checks the `tests/sanity/ignore-x.x.txt` file for disallowed ignores. -This rule is extremely opinionated and enforced by Partner Engineering. The +This rule is extremely opinionated and enforced by Partner Engineering as a requirement for Red Hat Certification. The currently allowed ruleset is subject to change, but is starting at a minimal number of allowed ignores for maximum test enforcement. Any commented-out ignore -entries are not evaluated. +entries are not evaluated, and ignore files for unsupported versions of ansible-core are not evaluated. This rule can produce messages like: @@ -29,10 +29,9 @@ Currently allowed ignores for all Ansible versions are: - `compile-2.7!skip` - `compile-3.5` - `compile-3.5!skip` - -Additionally allowed ignores for Ansible 2.9 are: -- `validate-modules:deprecation-mismatch` -- `validate-modules:invalid-documentation` +- `shellcheck` +- `shebang` +- `pylint:used-before-assignment` ## Problematic code diff --git a/src/ansiblelint/rules/sanity.py b/src/ansiblelint/rules/sanity.py index 09fe7cc..921e712 100644 --- a/src/ansiblelint/rules/sanity.py +++ b/src/ansiblelint/rules/sanity.py @@ -1,6 +1,8 @@ """Implementation of sanity rule.""" + from __future__ import annotations +import re import sys from typing import TYPE_CHECKING @@ -27,12 +29,7 @@ class CheckSanityIgnoreFiles(AnsibleLintRule): # Partner Engineering defines this list. Please contact PE for changes. - allowed_ignores_v2_9 = [ - "validate-modules:deprecation-mismatch", # Note: 2.9 expects a deprecated key in the METADATA. It was removed in later versions. - "validate-modules:invalid-documentation", # Note: The removed_at_date key in the deprecated section is invalid for 2.9. - ] - - allowed_ignores_all = [ + allowed_ignores = [ "validate-modules:missing-gplv3-license", "action-plugin-docs", # Added for Networking Collections "import-2.6", @@ -47,7 +44,18 @@ class CheckSanityIgnoreFiles(AnsibleLintRule): "compile-2.7!skip", "compile-3.5", "compile-3.5!skip", + "shebang", # Unreliable test + "shellcheck", # Unreliable test + "pylint:used-before-assignment", # Unreliable test + ] + + no_check_ignore_files = [ + "ignore-2.9", + "ignore-2.10", + "ignore-2.11", + "ignore-2.12", ] + _ids = { "sanity[cannot-ignore]": "Ignore file contains ... at line ..., which is not a permitted ignore.", "sanity[bad-ignore]": "Ignore file entry at ... is formatted incorrectly. Please review.", @@ -62,44 +70,55 @@ class CheckSanityIgnoreFiles(AnsibleLintRule): results: list[MatchError] = [] test = "" + check_dirs = { + "plugins", + "roles", + } + if file.kind != "sanity-ignore-file": return [] with file.path.open(encoding="utf-8") as ignore_file: entries = ignore_file.read().splitlines() - ignores = self.allowed_ignores_all - - # If there is a ignore-2.9.txt file, add the v2_9 list of allowed ignores - if "ignore-2.9.txt" in str(file.abspath): - ignores = self.allowed_ignores_all + self.allowed_ignores_v2_9 + if any(name in str(file.abspath) for name in self.no_check_ignore_files): + return [] for line_num, entry in enumerate(entries, 1): - if entry and entry[0] != "#": - try: - if "#" in entry: - entry, _ = entry.split("#") - (_, test) = entry.split() - if test not in ignores: + base_ignore_dir = "" + + if entry: + # match up to the first "/" + regex = re.match("[^/]*", entry) + + if regex: + base_ignore_dir = regex.group(0) + + if base_ignore_dir in check_dirs: + try: + if "#" in entry: + entry, _ = entry.split("#") + (_, test) = entry.split() + if test not in self.allowed_ignores: + results.append( + self.create_matcherror( + message=f"Ignore file contains {test} at line {line_num}, which is not a permitted ignore.", + tag="sanity[cannot-ignore]", + lineno=line_num, + filename=file, + ), + ) + + except ValueError: results.append( self.create_matcherror( - message=f"Ignore file contains {test} at line {line_num}, which is not a permitted ignore.", - tag="sanity[cannot-ignore]", + message=f"Ignore file entry at {line_num} is formatted incorrectly. Please review.", + tag="sanity[bad-ignore]", lineno=line_num, filename=file, ), ) - except ValueError: - results.append( - self.create_matcherror( - message=f"Ignore file entry at {line_num} is formatted incorrectly. Please review.", - tag="sanity[bad-ignore]", - lineno=line_num, - filename=file, - ), - ) - return results @@ -107,8 +126,9 @@ class CheckSanityIgnoreFiles(AnsibleLintRule): if "pytest" in sys.modules: import pytest - from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports - from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + # pylint: disable=ungrouped-imports + from ansiblelint.rules import RulesCollection + from ansiblelint.runner import Runner @pytest.mark.parametrize( ("test_file", "failures", "tags"), diff --git a/src/ansiblelint/rules/schema.py b/src/ansiblelint/rules/schema.py index 32ff2eb..6997acd 100644 --- a/src/ansiblelint/rules/schema.py +++ b/src/ansiblelint/rules/schema.py @@ -1,7 +1,9 @@ """Rule definition for JSON Schema Validations.""" + from __future__ import annotations import logging +import re import sys from typing import TYPE_CHECKING, Any @@ -13,6 +15,7 @@ from ansiblelint.schemas.main import validate_file_schema from ansiblelint.text import has_jinja if TYPE_CHECKING: + from ansiblelint.config import Options from ansiblelint.utils import Task @@ -94,7 +97,11 @@ class ValidateSchemaRule(AnsibleLintRule): def matchplay(self, file: Lintable, data: dict[str, Any]) -> list[MatchError]: """Return matches found for a specific playbook.""" results: list[MatchError] = [] - if not data or file.kind not in ("tasks", "handlers", "playbook"): + if ( + not data + or file.kind not in ("tasks", "handlers", "playbook") + or file.failed() + ): return results # check at play level results.extend(self._get_field_matches(file=file, data=data)) @@ -117,7 +124,7 @@ class ValidateSchemaRule(AnsibleLintRule): message=msg, lineno=data.get("__line__", 1), lintable=file, - rule=ValidateSchemaRule(), + rule=self, details=ValidateSchemaRule.description, tag=f"schema[{file.kind}]", ), @@ -129,9 +136,13 @@ class ValidateSchemaRule(AnsibleLintRule): task: Task, file: Lintable | None = None, ) -> bool | str | MatchError | list[MatchError]: - results = [] + results: list[MatchError] = [] if not file: file = Lintable("", kind="tasks") + + if file.failed(): + return results + results.extend(self._get_field_matches(file=file, data=task.raw_task)) for key in pre_checks["task"]: if key in task.raw_task: @@ -141,7 +152,7 @@ class ValidateSchemaRule(AnsibleLintRule): MatchError( message=msg, lintable=file, - rule=ValidateSchemaRule(), + rule=self, details=ValidateSchemaRule.description, tag=f"schema[{tag}]", ), @@ -151,12 +162,15 @@ class ValidateSchemaRule(AnsibleLintRule): def matchyaml(self, file: Lintable) -> list[MatchError]: """Return JSON validation errors found as a list of MatchError(s).""" result: list[MatchError] = [] + + if file.failed(): + return result + if file.kind not in JSON_SCHEMAS: return result - errors = validate_file_schema(file) - if errors: - if errors[0].startswith("Failed to load YAML file"): + for error in validate_file_schema(file): + if error.startswith("Failed to load YAML file"): _logger.debug( "Ignored failure to load %s for schema validation, as !vault may cause it.", file, @@ -165,13 +179,14 @@ class ValidateSchemaRule(AnsibleLintRule): result.append( MatchError( - message=errors[0], + message=error, lintable=file, - rule=ValidateSchemaRule(), + rule=self, details=ValidateSchemaRule.description, tag=f"schema[{file.kind}]", ), ) + break if not result: result = super().matchyaml(file) @@ -183,7 +198,6 @@ if "pytest" in sys.modules: import pytest # pylint: disable=ungrouped-imports - from ansiblelint.config import options from ansiblelint.rules import RulesCollection from ansiblelint.runner import Runner @@ -191,27 +205,30 @@ if "pytest" in sys.modules: ("file", "expected_kind", "expected"), ( pytest.param( - "examples/collection/galaxy.yml", + "examples/.collection/galaxy.yml", "galaxy", - ["'GPL' is not one of"], + [r".*'GPL' is not one of.*https://"], id="galaxy", ), pytest.param( "examples/roles/invalid_requirements_schema/meta/requirements.yml", "requirements", - ["{'foo': 'bar'} is not valid under any of the given schemas"], + [ + # r".*{'foo': 'bar'} is not valid under any of the given schemas.*https://", + r".*{'foo': 'bar'} is not of type 'array'.*https://", + ], id="requirements", ), pytest.param( "examples/roles/invalid_meta_schema/meta/main.yml", "meta", - ["False is not of type 'string'"], + [r".*False is not of type 'string'.*https://"], id="meta", ), pytest.param( "examples/playbooks/vars/invalid_vars_schema.yml", "vars", - ["'123' does not match any of the regexes"], + [r".* '123' does not match any of the regexes.*https://"], id="vars", ), pytest.param( @@ -223,14 +240,23 @@ if "pytest" in sys.modules: pytest.param( "examples/ee_broken/execution-environment.yml", "execution-environment", - ["{'foo': 'bar'} is not valid under any of the given schemas"], + [ + r".*Additional properties are not allowed \('foo' was unexpected\).*https://", + ], id="execution-environment-broken", ), - ("examples/meta/runtime.yml", "meta-runtime", []), + pytest.param( + "examples/meta/runtime.yml", + "meta-runtime", + [], + id="meta-runtime", + ), pytest.param( "examples/broken_collection_meta_runtime/meta/runtime.yml", "meta-runtime", - ["Additional properties are not allowed ('foo' was unexpected)"], + [ + r".*Additional properties are not allowed \('foo' was unexpected\).*https://", + ], id="meta-runtime-broken", ), pytest.param( @@ -242,7 +268,9 @@ if "pytest" in sys.modules: pytest.param( "examples/inventory/broken_dev_inventory.yml", "inventory", - ["Additional properties are not allowed ('foo' was unexpected)"], + [ + r".*Additional properties are not allowed \('foo' was unexpected\).*https://", + ], id="inventory-broken", ), pytest.param( @@ -260,7 +288,17 @@ if "pytest" in sys.modules: pytest.param( "examples/broken/.ansible-lint", "ansible-lint-config", - ["Additional properties are not allowed ('foo' was unexpected)"], + [ + r".*Additional properties are not allowed \('foo' was unexpected\).*https://", + ], + id="ansible-lint-config-broken", + ), + pytest.param( + "examples/broken_supported_ansible_also/.ansible-lint", + "ansible-lint-config", + [ + r".*supported_ansible_also True is not of type 'array'.*https://", + ], id="ansible-lint-config-broken", ), pytest.param( @@ -272,7 +310,9 @@ if "pytest" in sys.modules: pytest.param( "examples/broken/ansible-navigator.yml", "ansible-navigator-config", - ["Additional properties are not allowed ('ansible' was unexpected)"], + [ + r".*Additional properties are not allowed \('ansible' was unexpected\).*https://", + ], id="ansible-navigator-config-broken", ), pytest.param( @@ -284,20 +324,25 @@ if "pytest" in sys.modules: pytest.param( "examples/roles/broken_argument_specs/meta/argument_specs.yml", "role-arg-spec", - ["Additional properties are not allowed ('foo' was unexpected)"], + [ + r".*Additional properties are not allowed \('foo' was unexpected\).*https://", + ], id="role-arg-spec-broken", ), pytest.param( "examples/changelogs/changelog.yaml", "changelog", - ["Additional properties are not allowed ('foo' was unexpected)"], + [ + r".*Additional properties are not allowed \('foo' was unexpected\).*https://", + ], id="changelog", ), pytest.param( "examples/rulebooks/rulebook-fail.yml", "rulebook", [ - "Additional properties are not allowed ('that_should_not_be_here' was unexpected)", + # r".*Additional properties are not allowed \('that_should_not_be_here' was unexpected\).*https://", + r".*'sss' is not of type 'object'.*https://", ], id="rulebook", ), @@ -324,19 +369,24 @@ if "pytest" in sys.modules: ), ), ) - def test_schema(file: str, expected_kind: str, expected: list[str]) -> None: + def test_schema( + file: str, + expected_kind: str, + expected: list[str], + config_options: Options, + ) -> None: """Validate parsing of ansible output.""" lintable = Lintable(file) assert lintable.kind == expected_kind - rules = RulesCollection(options=options) + rules = RulesCollection(options=config_options) rules.register(ValidateSchemaRule()) results = Runner(lintable, rules=rules).run() assert len(results) == len(expected), results for idx, result in enumerate(results): assert result.filename.endswith(file) - assert expected[idx] in result.message + assert re.match(expected[idx], result.message) assert result.tag == f"schema[{expected_kind}]" @pytest.mark.parametrize( @@ -356,12 +406,13 @@ if "pytest" in sys.modules: expected_kind: str, expected_tag: str, count: int, + config_options: Options, ) -> None: """Validate ability to detect schema[moves].""" lintable = Lintable(file) assert lintable.kind == expected_kind - rules = RulesCollection(options=options) + rules = RulesCollection(options=config_options) rules.register(ValidateSchemaRule()) results = Runner(lintable, rules=rules).run() diff --git a/src/ansiblelint/rules/syntax_check.md b/src/ansiblelint/rules/syntax_check.md index e8197a5..566fa33 100644 --- a/src/ansiblelint/rules/syntax_check.md +++ b/src/ansiblelint/rules/syntax_check.md @@ -9,7 +9,7 @@ You can exclude these files from linting, but it is better to make sure they can be loaded by Ansible. This is often achieved by editing the inventory file and/or `ansible.cfg` so ansible can load required variables. -If undefined variables cause the failure, you can use the jinja `default()` +If undefined variables cause the failure, you can use the Jinja `default()` filter to provide fallback values, like in the example below. This rule is among the few `unskippable` rules that cannot be added to @@ -20,9 +20,32 @@ fixtures that are invalid on purpose. One of the most common sources of errors is a failure to assert the presence of various variables at the beginning of the playbook. -This rule can produce messages like below: +This rule can produce messages like: -- `syntax-check[empty-playbook]` is raised when a playbook file has no content. +- `syntax-check[empty-playbook]`: Empty playbook, nothing to do +- `syntax-check[malformed]`: A malformed block was encountered while loading a block +- `syntax-check[missing-file]`: Unable to retrieve file contents ... Could not find or access ... +- `syntax-check[unknown-module]`: couldn't resolve module/action +- `syntax-check[specific]`: for other errors not mentioned above. + +## syntax-check[unknown-module] + +The linter relies on ansible-core code to load the ansible code and it will +produce a syntax error if the code refers to ansible content that is not +installed. You must ensure that all collections and roles used inside your +repository are listed inside a [`requirements.yml`](https://docs.ansible.com/ansible/latest/galaxy/user_guide.html#installing-roles-and-collections-from-the-same-requirements-yml-file) file, so the linter can +install them when they are missing. + +Valid location for `requirements.yml` are: + +- `requirements.yml` +- `roles/requirements.yml` +- `collections/requirements.yml` +- `tests/requirements.yml` +- `tests/integration/requirements.yml` +- `tests/unit/requirements.yml` + +Note: If requirements are test related then they should be inside `tests/`. ## Problematic code diff --git a/src/ansiblelint/rules/syntax_check.py b/src/ansiblelint/rules/syntax_check.py index c6a4c5e..9b072f6 100644 --- a/src/ansiblelint/rules/syntax_check.py +++ b/src/ansiblelint/rules/syntax_check.py @@ -1,4 +1,5 @@ """Rule definition for ansible syntax check.""" + from __future__ import annotations import re @@ -15,6 +16,8 @@ class KnownError: regex: re.Pattern[str] +# Order matters, we only report the first matching pattern, the one at the end +# is used to match generic or less specific patterns. OUTPUT_PATTERNS = ( KnownError( tag="missing-file", @@ -25,9 +28,9 @@ OUTPUT_PATTERNS = ( ), ), KnownError( - tag="specific", + tag="no-file", regex=re.compile( - r"^ERROR! (?P<title>[^\n]*)\n\nThe error appears to be in '(?P<filename>[\w\/\.\-]+)': line (?P<line>\d+), column (?P<column>\d+)", + r"^ERROR! (?P<title>No file specified for [^\n]*)", re.MULTILINE | re.S | re.DOTALL, ), ), @@ -45,6 +48,28 @@ OUTPUT_PATTERNS = ( re.MULTILINE | re.S | re.DOTALL, ), ), + KnownError( + tag="unknown-module", + regex=re.compile( + r"^ERROR! (?P<title>couldn't resolve module/action [^\n]*)\n\nThe error appears to be in '(?P<filename>[\w\/\.\-]+)': line (?P<line>\d+), column (?P<column>\d+)", + re.MULTILINE | re.S | re.DOTALL, + ), + ), + KnownError( + tag="specific", + regex=re.compile( + r"^ERROR! (?P<title>[^\n]*)\n\nThe error appears to be in '(?P<filename>[\w\/\.\-]+)': line (?P<line>\d+), column (?P<column>\d+)", + re.MULTILINE | re.S | re.DOTALL, + ), + ), + # "ERROR! the role 'this_role_is_missing' was not found in ROLE_INCLUDE_PATHS\n\nThe error appears to be in 'FILE_PATH': line 5, column 7, but may\nbe elsewhere in the file depending on the exact syntax problem.\n\nThe offending line appears to be:\n\n roles:\n - this_role_is_missing\n ^ here\n" + KnownError( + tag="specific", + regex=re.compile( + r"^ERROR! (?P<title>the role '.*' was not found in[^\n]*)'(?P<filename>[\w\/\.\-]+)': line (?P<line>\d+), column (?P<column>\d+)", + re.MULTILINE | re.S | re.DOTALL, + ), + ), ) diff --git a/src/ansiblelint/rules/var_naming.md b/src/ansiblelint/rules/var_naming.md index 3386a0c..e4034f0 100644 --- a/src/ansiblelint/rules/var_naming.md +++ b/src/ansiblelint/rules/var_naming.md @@ -22,7 +22,7 @@ Possible errors messages: - `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. + `role_name_` as a prefix. Underlines are accepted before the prefix. - `var-naming[no-reserved]`: Variables names must not be Ansible reserved names. - `var-naming[read-only]`: This special variable is read-only. diff --git a/src/ansiblelint/rules/var_naming.py b/src/ansiblelint/rules/var_naming.py index 389530d..14a4c40 100644 --- a/src/ansiblelint/rules/var_naming.py +++ b/src/ansiblelint/rules/var_naming.py @@ -1,4 +1,5 @@ """Implementation of var-naming rule.""" + from __future__ import annotations import keyword @@ -9,13 +10,19 @@ from typing import TYPE_CHECKING, Any from ansible.parsing.yaml.objects import AnsibleUnicode from ansible.vars.reserved import get_reserved_names -from ansiblelint.config import options -from ansiblelint.constants import ANNOTATION_KEYS, LINE_NUMBER_KEY, RC +from ansiblelint.config import Options, options +from ansiblelint.constants import ( + ANNOTATION_KEYS, + LINE_NUMBER_KEY, + PLAYBOOK_ROLE_KEYWORDS, + 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.text import has_jinja, is_fqcn_or_name from ansiblelint.utils import parse_yaml_from_file if TYPE_CHECKING: @@ -160,10 +167,15 @@ class VariableNamingRule(AnsibleLintRule): rule=self, ) - if prefix and not ident.startswith(f"{prefix}_"): + if ( + prefix + and not ident.lstrip("_").startswith(f"{prefix}_") + and not has_jinja(prefix) + and is_fqcn_or_name(prefix) + ): return MatchError( tag="var-naming[no-role-prefix]", - message="Variables names from within roles should use role_name_ as a prefix.", + message=f"Variables names from within roles should use {prefix}_ as a prefix.", rule=self, ) return None @@ -187,6 +199,37 @@ class VariableNamingRule(AnsibleLintRule): else our_vars[LINE_NUMBER_KEY] ) raw_results.append(match_error) + roles = data.get("roles", []) + for role in roles: + if isinstance(role, AnsibleUnicode): + continue + role_fqcn = role.get("role", role.get("name")) + prefix = role_fqcn.split("/" if "/" in role_fqcn else ".")[-1] + for key in list(role.keys()): + if key not in PLAYBOOK_ROLE_KEYWORDS: + match_error = self.get_var_naming_matcherror(key, prefix=prefix) + if match_error: + match_error.filename = str(file.path) + match_error.message += f" (vars: {key})" + match_error.lineno = ( + key.ansible_pos[1] + if isinstance(key, AnsibleUnicode) + else role[LINE_NUMBER_KEY] + ) + raw_results.append(match_error) + + our_vars = role.get("vars", {}) + for key in our_vars: + match_error = self.get_var_naming_matcherror(key, prefix=prefix) + if match_error: + match_error.filename = str(file.path) + match_error.message += f" (vars: {key})" + 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: @@ -266,7 +309,8 @@ class VariableNamingRule(AnsibleLintRule): if str(file.kind) == "vars" and file.data: meta_data = parse_yaml_from_file(str(file.path)) for key in meta_data: - match_error = self.get_var_naming_matcherror(key) + prefix = file.role if file.role else "" + match_error = self.get_var_naming_matcherror(key, prefix=prefix) if match_error: match_error.filename = filename match_error.lineno = key.ansible_pos[1] @@ -298,13 +342,21 @@ if "pytest" in sys.modules: @pytest.mark.parametrize( ("file", "expected"), ( - pytest.param("examples/playbooks/rule-var-naming-fail.yml", 7, id="0"), + pytest.param( + "examples/playbooks/var-naming/rule-var-naming-fail.yml", + 7, + id="0", + ), pytest.param("examples/Taskfile.yml", 0, id="1"), ), ) - def test_invalid_var_name_playbook(file: str, expected: int) -> None: + def test_invalid_var_name_playbook( + file: str, + expected: int, + config_options: Options, + ) -> None: """Test rule matches.""" - rules = RulesCollection(options=options) + rules = RulesCollection(options=config_options) rules.register(VariableNamingRule()) results = Runner(Lintable(file), rules=rules).run() assert len(results) == expected @@ -337,6 +389,40 @@ if "pytest" in sys.modules: assert result.tag == expected_errors[idx][0] assert result.lineno == expected_errors[idx][1] + def test_var_naming_with_role_prefix( + default_rules_collection: RulesCollection, + ) -> None: + """Test rule matches.""" + results = Runner( + Lintable("examples/roles/role_vars_prefix_detection"), + rules=default_rules_collection, + ).run() + assert len(results) == 2 + for result in results: + assert result.tag == "var-naming[no-role-prefix]" + + def test_var_naming_with_role_prefix_plays( + default_rules_collection: RulesCollection, + ) -> None: + """Test rule matches.""" + results = Runner( + Lintable("examples/playbooks/role_vars_prefix_detection.yml"), + rules=default_rules_collection, + exclude_paths=["examples/roles/role_vars_prefix_detection"], + ).run() + expected_errors = ( + ("var-naming[no-role-prefix]", 9), + ("var-naming[no-role-prefix]", 12), + ("var-naming[no-role-prefix]", 15), + ("var-naming[no-role-prefix]", 25), + ("var-naming[no-role-prefix]", 32), + ("var-naming[no-role-prefix]", 45), + ) + 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.""" role_path = "examples/roles/var_naming_pattern/tasks/main.yml" @@ -364,7 +450,7 @@ if "pytest" in sys.modules: def test_var_naming_with_include_role_import_role() -> None: """Test with include role and import role.""" - role_path = "examples/test_collection/roles/my_role/tasks/main.yml" + role_path = "examples/.test_collection/roles/my_role/tasks/main.yml" result = run_ansible_lint(role_path) assert result.returncode == RC.SUCCESS assert "var-naming" not in result.stdout diff --git a/src/ansiblelint/rules/yaml.md b/src/ansiblelint/rules/yaml.md index 8dc56eb..654f80e 100644 --- a/src/ansiblelint/rules/yaml.md +++ b/src/ansiblelint/rules/yaml.md @@ -1,6 +1,8 @@ # yaml -This rule checks YAML syntax and is an implementation of `yamllint`. +This rule checks YAML syntax by using [yamllint] library but with a +[specific default configuration](#yamllint-configuration), one that is +compatible with both, our internal reformatter (`--fix`) and also [prettier]. You can disable YAML syntax violations by adding `yaml` to the `skip_list` in your Ansible-lint configuration as follows: @@ -53,6 +55,7 @@ Some of the detailed error codes that you might see are: - `yaml[empty-lines]` - _too many blank lines (...> ...)_ - `yaml[indentation]` - _Wrong indentation: expected ... but found ..._ - `yaml[key-duplicates]` - _Duplication of key "..." in mapping_ +- `yaml[line-length]` - _Line too long (... > ... characters)_ - `yaml[new-line-at-end-of-file]` - _No new line character at the end of file_ - `yaml[octal-values]`: forbidden implicit or explicit [octal](#octals) value - `yaml[syntax]` - YAML syntax is broken @@ -72,6 +75,13 @@ for it does check these. If for some reason, you do not want to follow our defaults, you can create a `.yamllint` file in your project and this will take precedence over our defaults. +## Additional Information for Multiline Strings + +Adhering to yaml[line-length] rule, for writing multiline strings we recommend +using Block Style Indicator: literal style indicated by a pipe (|) or folded +style indicated by a right angle bracket (>), instead of escaping the newlines +with backslashes. Reference [guide] for writing multiple line strings in yaml. + ## Problematic code ```yaml @@ -91,7 +101,53 @@ foo2: "0o777" # <-- Explicitly quoting octal is less risky. bar: ... # Correct comment indentation. ``` +## Yamllint configuration + +If you decide to add a custom yamllint config to your project, ansible-lint +might refuse to run if it detects that some of your options are incompatible and +ask you to correct them. When this happens, you will see a message like the one +below: + +``` +CRITICAL Found incompatible custom yamllint configuration (.yamllint), please either remove the file or edit it to comply with: + - comments.min-spaces-from-content must be 1 + - braces.min-spaces-inside must be 0 + - braces.max-spaces-inside must be 1 + - octal-values.forbid-implicit-octal must be true + - octal-values.forbid-explicit-octal must be true + +Read https://ansible.readthedocs.io/projects/lint/rules/yaml/ for more details regarding why we have these requirements. +``` + +!!! warning + + [Auto-fix](../autofix.md) functionality will change **inline comment indentation to one + character instead of two**, which is the default of [yamllint]. The reason + for this decision was to keep reformatting compatibility + with [prettier], which is the most popular reformatter. + + ```yaml title=".yamllint" + rules: + comments: + min-spaces-from-content: 1 # prettier compatibility + ``` + + There is no need to create this yamllint config file, but if you also + run yamllint yourself, you might want to create it to make it behave + the same way as ansible-lint. + +Below you can find the default yamllint configuration that our linter will use +when there is no custom file present. + +```yaml +{!../src/ansiblelint/data/.yamllint!} +``` + [1.1]: https://yaml.org/spec/1.1/ [1.2.0]: https://yaml.org/spec/1.2.0/ [1.2.2]: https://yaml.org/spec/1.2.2/ [yaml specification]: https://yaml.org/ +[guide]: + https://docs.ansible.com/ansible/latest/reference_appendices/YAMLSyntax.html#yaml-basics +[prettier]: https://prettier.io/ +[yamllint]: https://yamllint.readthedocs.io/en/stable/ diff --git a/src/ansiblelint/rules/yaml_rule.py b/src/ansiblelint/rules/yaml_rule.py index 4da4d41..3ec5b59 100644 --- a/src/ansiblelint/rules/yaml_rule.py +++ b/src/ansiblelint/rules/yaml_rule.py @@ -1,27 +1,29 @@ """Implementation of yaml linting rule (yamllint integration).""" + from __future__ import annotations import logging import sys -from collections.abc import Iterable +from collections.abc import Iterable, MutableMapping, MutableSequence from typing import TYPE_CHECKING from yamllint.linter import run as run_yamllint from ansiblelint.constants import LINE_NUMBER_KEY, SKIPPED_RULES_KEY from ansiblelint.file_utils import Lintable -from ansiblelint.rules import AnsibleLintRule +from ansiblelint.rules import AnsibleLintRule, TransformMixin from ansiblelint.yaml_utils import load_yamllint_config if TYPE_CHECKING: from typing import Any + from ansiblelint.config import Options from ansiblelint.errors import MatchError _logger = logging.getLogger(__name__) -class YamllintRule(AnsibleLintRule): +class YamllintRule(AnsibleLintRule, TransformMixin): """Violations reported by yamllint.""" id = "yaml" @@ -73,6 +75,12 @@ class YamllintRule(AnsibleLintRule): self.severity = "VERY_LOW" if problem.level == "error": self.severity = "MEDIUM" + # Ignore truthy violation with github workflows ("on:" keys) + if problem.rule == "truthy" and file.path.parent.parts[-2:] == ( + ".github", + "workflows", + ): + continue matches.append( self.create_matcherror( # yamllint does return lower-case sentences @@ -85,6 +93,22 @@ class YamllintRule(AnsibleLintRule): ) return matches + def transform( + self: YamllintRule, + match: MatchError, + lintable: Lintable, + data: MutableMapping[str, Any] | MutableSequence[Any] | str, + ) -> None: + """Transform yaml. + + :param match: MatchError instance + :param lintable: Lintable instance + :param data: data to transform + """ + # This method does nothing because the YAML reformatting is implemented + # in data dumper. Still presence of this method helps us with + # documentation generation. + def _combine_skip_rules(data: Any) -> set[str]: """Return a consolidated list of skipped rules.""" @@ -107,7 +131,7 @@ def _fetch_skips(data: Any, collector: dict[int, set[str]]) -> dict[int, set[str collector[data.get(LINE_NUMBER_KEY)].update(rules) if isinstance(data, Iterable) and not isinstance(data, str): if isinstance(data, dict): - for _entry, value in data.items(): + for value in data.values(): _fetch_skips(value, collector) else: # must be some kind of list for entry in data: @@ -128,7 +152,6 @@ if "pytest" in sys.modules: import pytest # pylint: disable=ungrouped-imports - from ansiblelint.config import options from ansiblelint.rules import RulesCollection from ansiblelint.runner import Runner @@ -180,15 +203,26 @@ if "pytest" in sys.modules: [], id="rule-yaml-pass", ), + pytest.param( + "examples/yamllint/.github/workflows/ci.yml", + "yaml", + [], + id="rule-yaml-github-workflow", + ), ), ) @pytest.mark.filterwarnings("ignore::ansible_compat.runtime.AnsibleWarning") - def test_yamllint(file: str, expected_kind: str, expected: list[str]) -> None: + def test_yamllint( + file: str, + expected_kind: str, + expected: list[str], + config_options: Options, + ) -> None: """Validate parsing of ansible output.""" lintable = Lintable(file) assert lintable.kind == expected_kind - rules = RulesCollection(options=options) + rules = RulesCollection(options=config_options) rules.register(YamllintRule()) results = Runner(lintable, rules=rules).run() |