From 350a026ba7c0a46e09f5457b65bdbd7194105170 Mon Sep 17 00:00:00 2001 From: Sorin Sbarnea Date: Thu, 27 Oct 2022 20:15:09 +0100 Subject: [PATCH] Add experimental rule for run_once (#2626) Co-authored-by: Ajinkya --- .github/workflows/tox.yml | 2 +- examples/playbooks/run-once-fail.yml | 10 ++++ examples/playbooks/run-once-pass.yml | 8 +++ src/ansiblelint/rules/no_free_form.py | 6 +- src/ansiblelint/rules/run_once.md | 41 +++++++++++++ src/ansiblelint/rules/run_once.py | 84 +++++++++++++++++++++++++++ test/test_rules_collection.py | 2 +- 7 files changed, 148 insertions(+), 5 deletions(-) create mode 100644 examples/playbooks/run-once-fail.yml create mode 100644 examples/playbooks/run-once-pass.yml create mode 100644 src/ansiblelint/rules/run_once.md create mode 100644 src/ansiblelint/rules/run_once.py diff --git a/.github/workflows/tox.yml b/.github/workflows/tox.yml index e0d95b73f7..8fef0843a3 100644 --- a/.github/workflows/tox.yml +++ b/.github/workflows/tox.yml @@ -166,7 +166,7 @@ jobs: WSLENV: FORCE_COLOR:PYTEST_REQPASS:TOXENV:TOX_PARALLEL_NO_SPINNER # Number of expected test passes, safety measure for accidental skip of # tests. Update value if you add/remove tests. - PYTEST_REQPASS: 708 + PYTEST_REQPASS: 710 steps: - name: Activate WSL1 diff --git a/examples/playbooks/run-once-fail.yml b/examples/playbooks/run-once-fail.yml new file mode 100644 index 0000000000..cb7621812d --- /dev/null +++ b/examples/playbooks/run-once-fail.yml @@ -0,0 +1,10 @@ +--- +- name: "Example with run_once" + hosts: all + strategy: free # <-- avoid use of strategy as free + gather_facts: false + tasks: + - name: Task with run_once + ansible.builtin.debug: + msg: "Test" + run_once: true # <-- avoid use of strategy as free at play level when using run_once at task level diff --git a/examples/playbooks/run-once-pass.yml b/examples/playbooks/run-once-pass.yml new file mode 100644 index 0000000000..030a2a0513 --- /dev/null +++ b/examples/playbooks/run-once-pass.yml @@ -0,0 +1,8 @@ +--- +- name: "Example without run_once" + hosts: all + gather_facts: false + tasks: + - name: Task without run_once + ansible.builtin.debug: + msg: "Test" diff --git a/src/ansiblelint/rules/no_free_form.py b/src/ansiblelint/rules/no_free_form.py index eaa1ad5fac..1e121f11f4 100644 --- a/src/ansiblelint/rules/no_free_form.py +++ b/src/ansiblelint/rules/no_free_form.py @@ -1,4 +1,4 @@ -"""Implementation of NoFreeFormRune.""" +"""Implementation of NoFreeFormRule.""" from __future__ import annotations import re @@ -14,7 +14,7 @@ from ansiblelint.file_utils import Lintable -class NoFreeFormRune(AnsibleLintRule): +class NoFreeFormRule(AnsibleLintRule): """Rule for detecting discouraged free-form syntax for action modules.""" id = "no-free-form" @@ -106,5 +106,5 @@ def test_rule_no_free_form( results = Runner(file, rules=default_rules_collection).run() for result in results: - assert result.rule.id == NoFreeFormRune.id, result + assert result.rule.id == NoFreeFormRule.id, result assert len(results) == expected diff --git a/src/ansiblelint/rules/run_once.md b/src/ansiblelint/rules/run_once.md new file mode 100644 index 0000000000..2e47ac26b7 --- /dev/null +++ b/src/ansiblelint/rules/run_once.md @@ -0,0 +1,41 @@ +# run-once + +This rule warns against the use of `run_once` when `strategy` is set to `free`. + +This rule can produce the following messages: + +- `run_once[play]`: Play uses `strategy: free`. +- `run_once[task]`: Using `run_once` may behave differently if `strategy` is set to `free`. + +For more information see the following topics in Ansible documentation: + +- [free strategy](https://docs.ansible.com/ansible/latest/collections/ansible/builtin/free_strategy.html#free-strategy) +- [selecting a strategy](https://docs.ansible.com/ansible/latest/user_guide/playbooks_strategies.html#selecting-a-strategy) +- [run_once(playbook keyword) more info](https://docs.ansible.com/ansible/latest/reference_appendices/playbooks_keywords.html) + +## Problematic Code + +```yaml +--- +- name: "Example with run_once" + hosts: all + strategy: free # <-- avoid use of strategy as free + gather_facts: false + tasks: + - name: Task with run_once + ansible.builtin.debug: + msg: "Test" + run_once: true # <-- avoid use of strategy as free at play level when using run_once at task level +``` + +## Correct Code + +```yaml +- name: "Example without run_once" + hosts: all + gather_facts: false + tasks: + - name: Task without run_once + ansible.builtin.debug: + msg: "Test" +``` diff --git a/src/ansiblelint/rules/run_once.py b/src/ansiblelint/rules/run_once.py new file mode 100644 index 0000000000..ea49654bee --- /dev/null +++ b/src/ansiblelint/rules/run_once.py @@ -0,0 +1,84 @@ +"""Optional Ansible-lint rule to warn use of run_once with strategy free.""" +from __future__ import annotations + +import sys +from typing import TYPE_CHECKING, Any + +from ansiblelint.errors import MatchError +from ansiblelint.rules import AnsibleLintRule + +if TYPE_CHECKING: + from ansiblelint.file_utils import Lintable + + +class RunOnce(AnsibleLintRule): + """Run once should use strategy other than free.""" + + id = "run-once" + link = "https://docs.ansible.com/ansible/latest/reference_appendices/playbooks_keywords.html" + description = "When using run_once, we should avoid using strategy as free." + + tags = ["idiom", "experimental"] + severity = "MEDIUM" + + def matchplay(self, file: Lintable, data: dict[str, Any]) -> list[MatchError]: + """Return matches found for a specific playbook.""" + # If the Play uses the 'strategy' and it's value is set to free + + if not file or file.kind != "playbook" or not data: + return [] + + strategy = data.get("strategy", None) + run_once = data.get("run_once", False) + if (not strategy and not run_once) or strategy != "free": + return [] + return [ + self.create_matcherror( + message="Play uses strategy: free", + filename=file, + tag="run_once[play]", + ) + ] + + def matchtask( + self, task: dict[str, Any], file: Lintable | None = None + ) -> list[MatchError]: + """Return matches for a task.""" + if not file or file.kind != "playbook": + return [] + + run_once = task.get("run_once", False) + if not run_once: + return [] + return [ + self.create_matcherror( + message="Using run_once may behave differently if strategy is set to free.", + filename=file, + tag="run_once[task]", + ) + ] + + +# 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 + + @pytest.mark.parametrize( + ("test_file", "failure"), + ( + pytest.param("examples/playbooks/run-once-pass.yml", 0, id="pass"), + pytest.param("examples/playbooks/run-once-fail.yml", 2, id="fail"), + ), + ) + def test_run_once( + default_rules_collection: RulesCollection, test_file: str, failure: int + ) -> None: + """Test rule matches.""" + results = Runner(test_file, rules=default_rules_collection).run() + for result in results: + assert result.rule.id == RunOnce().id + assert len(results) == failure diff --git a/test/test_rules_collection.py b/test/test_rules_collection.py index 85f3aa5b92..e548cd5f64 100644 --- a/test/test_rules_collection.py +++ b/test/test_rules_collection.py @@ -166,5 +166,5 @@ def test_rules_id_format() -> None: rule.help != "" or rule.description or rule.__doc__ ), f"Rule {rule.id} must have at least one of: .help, .description, .__doc__" assert "yaml" in keys, "yaml rule is missing" - assert len(rules) == 47 # update this number when adding new rules! + assert len(rules) == 48 # update this number when adding new rules! assert len(keys) == len(rules), "Duplicate rule ids?"