Extend key-order key to sort more keys than the name (#2454)

- refactor key ordering to allow us to sort all keys
- ensure `block`/`rescue`/`always` are sorted `last`

.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

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

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:

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
 

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,50 +54,32 @@ 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
 if "pytest" in sys.modules:
 
     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

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(