diff --git a/.github/workflows/tox.yml b/.github/workflows/tox.yml index a6b6c84566d..2dd7e6a8499 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: 697 + PYTEST_REQPASS: 709 steps: - name: Activate WSL1 diff --git a/examples/playbooks/test_skip_inside_yaml.yml b/examples/playbooks/test_skip_inside_yaml.yml index 27978cb1eb8..e19ee1c9e93 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 0014b3f4708..d8949314670 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 +- `action` should be the second key, just after `name` - when present, the `block` key must be the last, avoid accidental indentation - bugs moving keys between block and the last task within the block. + bugs moving keys between `block` and the last task within the block. ## Problematic code diff --git a/src/ansiblelint/rules/key_order.py b/src/ansiblelint/rules/key_order.py index dddbe15def5..5177f3d0d66 100644 --- a/src/ansiblelint/rules/key_order.py +++ b/src/ansiblelint/rules/key_order.py @@ -1,6 +1,7 @@ """All tasks should be have name come first.""" from __future__ import annotations +import functools import sys from typing import Any @@ -8,6 +9,35 @@ from ansiblelint.rules import AnsibleLintRule from ansiblelint.testing import RunFromText +SORTER = ( + "name", + "action", + None, # this is where all the unknowns go + "when", + "notify", + "block", + "rescue", + "always", +) + + +def get_property_sort_index(a: str) -> int: + """Return the index of the property in the sorter.""" + a_index = -1 + for i, v in enumerate(SORTER): + if v == a: + return i + elif v is None: + a_index = i + return a_index + + +def task_property_sorter(a: str, b: str) -> int: + """Sort task properties based on SORTER.""" + x = get_property_sort_index(a) + y = get_property_sort_index(b) + return (x > y) - (x < y) + class KeyOrderRule(AnsibleLintRule): """Ensure specific order of keys in mappings.""" @@ -16,17 +46,17 @@ class KeyOrderRule(AnsibleLintRule): shortdesc = __doc__ severity = "LOW" tags = ["formatting", "experimental"] - version_added = "v6.2.0" + version_added = "v6.6.1" needs_raw_task = True def matchtask( self, task: dict[str, Any], file: Lintable | None = None ) -> bool | str: 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" + 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: + return f"You can improve the task key order to: {', '.join(sorted_keys)}" return False @@ -86,3 +116,45 @@ def test_task_name_has_name_first_rule_fail(rule_runner: RunFromText) -> None: """Test rule matches.""" results = rule_runner.run_playbook(PLAY_FAIL) assert len(results) == 6 + + @pytest.mark.parametrize( + "input,expected", + [ + pytest.param([], []), + pytest.param(["block", "name"], ["name", "block"]), + pytest.param( + ["block", "name", "action", "..."], ["name", "action", "...", "block"] + ), + ], + ) + def test_property_sorter(input: list[str], expected: list[str]) -> None: + """Test the task property sorter.""" + result = sorted(input, 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.index(None)), + pytest.param("block", len(SORTER) - 3), + pytest.param("rescue", len(SORTER) - 2), + pytest.param("always", len(SORTER) - 1), + ], + ) + def test_property_sort_index(key: str, order: int) -> None: + """Test sorting index""" + assert get_property_sort_index(key) == order + + @pytest.mark.parametrize( + "a,b,result", + [ + pytest.param("name", "block", -1), + pytest.param("block", "name", 1), + pytest.param("block", "block", 0), + ], + ) + def test_property_sortfunc(a: str, b: str, result: int) -> None: + "Test sorting function" + assert task_property_sorter(a, b) == result