diff --git a/.github/workflows/tox.yml b/.github/workflows/tox.yml index 8b1e2c2499..eddfe2e6f3 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: 702 + PYTEST_REQPASS: 714 steps: - name: Activate WSL1 diff --git a/examples/playbooks/rule-key-order-fail.yml b/examples/playbooks/rule-key-order-fail.yml new file mode 100644 index 0000000000..a83514a9e4 --- /dev/null +++ b/examples/playbooks/rule-key-order-fail.yml @@ -0,0 +1,30 @@ +--- +- name: Fixture + hosts: localhost + tasks: + - no_log: true + ansible.builtin.command: echo hello + name: Task with no_log on top + changed_when: false + - when: true + name: Task with when on top + ansible.builtin.command: echo hello + changed_when: false + - delegate_to: localhost + name: Delegate_to on top + ansible.builtin.command: echo hello + changed_when: false + - loop: + - 1 + - 2 + name: Loopy + ansible.builtin.command: echo {{ item }} + changed_when: false + - become: true + name: Become first + ansible.builtin.command: echo hello + changed_when: false + - register: test + ansible.builtin.command: echo hello + name: Register first + changed_when: false diff --git a/examples/playbooks/test_skip_inside_yaml.yml b/examples/playbooks/test_skip_inside_yaml.yml index 27978cb1eb..e19ee1c9e9 100644 --- a/examples/playbooks/test_skip_inside_yaml.yml +++ b/examples/playbooks/test_skip_inside_yaml.yml @@ -10,11 +10,11 @@ action: ansible.builtin.hg - name: Test latest[git] and partial-become - become_user: alice action: ansible.builtin.git - - name: Test latest[git] and partial-become (skipped) # noqa latest[git] partial-become become_user: alice + - name: Test latest[git] and partial-become (skipped) # noqa latest[git] partial-become action: ansible.builtin.git + become_user: alice - name: Test YAML # <-- 1 jinja[spacing] ansible.builtin.get_url: diff --git a/src/ansiblelint/rules/key_order.md b/src/ansiblelint/rules/key_order.md index 0014b3f470..ee3b07bf7e 100644 --- a/src/ansiblelint/rules/key_order.md +++ b/src/ansiblelint/rules/key_order.md @@ -1,13 +1,14 @@ # key-order -This rule recommends reordering key names in ansible content in order to make -code easier to maintain and avoid mistakes. +This rule recommends reordering key names in ansible content to make +code easier to maintain and less prone to errors. -Here are some examples of common ordering checks done: +Here are some examples of common ordering checks done for tasks and handlers: - `name` must always be the first key for plays, tasks and handlers -- when present, the `block` key must be the last, avoid accidental indentation - bugs moving keys between block and the last task within the block. +- on tasks, the `block`, `rescue` and `always` keys must be the last keys, + as this would avoid accidental miss-indentation errors between the last task + and the parent level. ## Problematic code diff --git a/src/ansiblelint/rules/key_order.py b/src/ansiblelint/rules/key_order.py index dddbe15def..1ddd732f94 100644 --- a/src/ansiblelint/rules/key_order.py +++ b/src/ansiblelint/rules/key_order.py @@ -1,13 +1,51 @@ """All tasks should be have name come first.""" from __future__ import annotations +import functools import sys -from typing import Any +from typing import TYPE_CHECKING, Any from ansiblelint.file_utils import Lintable from ansiblelint.rules import AnsibleLintRule from ansiblelint.testing import RunFromText +if TYPE_CHECKING: + from ansiblelint.errors import MatchError + + +SORTER_TASKS = ( + "name", + # "__module__", + # "action", + # "args", + None, # <-- None include all modules that not using action and * + # "when", + # "(loop|loop_|with_).*", + # "notify", + # "tags", + "block", + "rescue", + "always", +) + + +def get_property_sort_index(name: str) -> int: + """Return the index of the property in the sorter.""" + a_index = -1 + for i, v in enumerate(SORTER_TASKS): + if v == name: + return i + if v is None: + a_index = i + return a_index + + +def task_property_sorter(property1: str, property2: str) -> int: + """Sort task properties based on SORTER.""" + v_1 = get_property_sort_index(property1) + v_2 = get_property_sort_index(property2) + return (v_1 > v_2) - (v_1 < v_2) + class KeyOrderRule(AnsibleLintRule): """Ensure specific order of keys in mappings.""" @@ -16,18 +54,25 @@ class KeyOrderRule(AnsibleLintRule): shortdesc = __doc__ severity = "LOW" tags = ["formatting", "experimental"] - version_added = "v6.2.0" + version_added = "v6.6.2" needs_raw_task = True def matchtask( self, task: dict[str, Any], file: Lintable | None = None - ) -> bool | str: + ) -> list[MatchError]: + result = [] raw_task = task["__raw_task__"] - if "name" in raw_task: - attribute_list = [*raw_task] - if bool(attribute_list[0] != "name"): - return "'name' key is not first" - return False + keys = [key for key in raw_task.keys() if not key.startswith("_")] + 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 task key order to: {', '.join(sorted_keys)}", + filename=file, + tag="key-order[task]", + ) + ) + return result # testing code to be loaded only with pytest or when executed the rule file @@ -35,31 +80,6 @@ def matchtask( import pytest - PLAY_FAIL = """--- -- hosts: localhost - tasks: - - no_log: true - shell: echo hello - name: Task with no_log on top - - when: true - name: Task with when on top - shell: echo hello - - delegate_to: localhost - name: Delegate_to on top - shell: echo hello - - loop: - - 1 - - 2 - name: Loopy - command: echo {{ item }} - - become: true - name: Become first - shell: echo hello - - register: test - shell: echo hello - name: Register first -""" - PLAY_SUCCESS = """--- - hosts: localhost tasks: @@ -76,13 +96,61 @@ def matchtask( """ @pytest.mark.parametrize("rule_runner", (KeyOrderRule,), indirect=["rule_runner"]) - def test_task_name_has_name_first_rule_pass(rule_runner: RunFromText) -> None: + def test_key_order_task_name_has_name_first_rule_pass( + rule_runner: RunFromText, + ) -> None: """Test rule matches.""" results = rule_runner.run_playbook(PLAY_SUCCESS) assert len(results) == 0 @pytest.mark.parametrize("rule_runner", (KeyOrderRule,), indirect=["rule_runner"]) - def test_task_name_has_name_first_rule_fail(rule_runner: RunFromText) -> None: + def test_key_order_task_name_has_name_first_rule_fail( + rule_runner: RunFromText, + ) -> None: """Test rule matches.""" - results = rule_runner.run_playbook(PLAY_FAIL) + results = rule_runner.run("examples/playbooks/rule-key-order-fail.yml") assert len(results) == 6 + + @pytest.mark.parametrize( + ("properties", "expected"), + ( + pytest.param([], []), + pytest.param(["block", "name"], ["name", "block"]), + pytest.param( + ["block", "name", "action", "..."], ["name", "action", "...", "block"] + ), + ), + ) + def test_key_order_property_sorter( + properties: list[str], expected: list[str] + ) -> None: + """Test the task property sorter.""" + result = sorted(properties, key=functools.cmp_to_key(task_property_sorter)) + assert expected == result + + @pytest.mark.parametrize( + ("key", "order"), + ( + pytest.param("name", 0), + pytest.param("action", 1), + pytest.param("foobar", SORTER_TASKS.index(None)), + pytest.param("block", len(SORTER_TASKS) - 3), + pytest.param("rescue", len(SORTER_TASKS) - 2), + pytest.param("always", len(SORTER_TASKS) - 1), + ), + ) + def test_key_order_property_sort_index(key: str, order: int) -> None: + """Test sorting index.""" + assert get_property_sort_index(key) == order + + @pytest.mark.parametrize( + ("prop1", "prop2", "result"), + ( + pytest.param("name", "block", -1), + pytest.param("block", "name", 1), + pytest.param("block", "block", 0), + ), + ) + def test_key_order_property_sortfunc(prop1: str, prop2: str, result: int) -> None: + """Test sorting function.""" + assert task_property_sorter(prop1, prop2) == result diff --git a/src/ansiblelint/testing/__init__.py b/src/ansiblelint/testing/__init__.py index a23398de4b..dd7c88ec9a 100644 --- a/src/ansiblelint/testing/__init__.py +++ b/src/ansiblelint/testing/__init__.py @@ -38,6 +38,10 @@ def _call_runner(self, path: str) -> list[MatchError]: runner = Runner(path, rules=self.collection) return runner.run() + def run(self, filename: str) -> list[MatchError]: + """Lints received filename.""" + return self._call_runner(filename) + def run_playbook(self, playbook_text: str) -> list[MatchError]: """Lints received text as a playbook.""" with tempfile.NamedTemporaryFile(