feat: add new rule CheckKeyOrder (#765)

ansiblelater/candidate.py

@@ -23,7 +23,8 @@ class Candidate:
         self.path = filename
         self.binary = False
         self.vault = False
-        self.filetype = type(self).__name__.lower()
+        self.filemeta = type(self).__name__.lower()
+        self.kind = type(self).__name__.lower()
         self.faulty = False
         self.config = settings.config
         self.settings = settings
@@ -54,7 +55,7 @@ class Candidate:
         self.rules = SingleRules(self.config["rules"]["dir"]).rules
 
         for rule in self._filter_rules():
-            if type(self).__name__.lower() not in rule.types:
+            if self.kind not in rule.types:
                 continue
 
             result = rule.check(self, self.config)
@@ -145,7 +146,7 @@ class Candidate:
         return rule_id
 
     def __repr__(self):
-        return f"{type(self).__name__.lower()} ({self.path})"
+        return f"{self.kind} ({self.path})"
 
     def __getitem__(self, item):
         return self.__dict__.get(item)
@@ -177,7 +178,7 @@ class Task(RoleFile):
 
     def __init__(self, filename, settings={}, rules=[]):  # noqa
         super().__init__(filename, settings, rules)
-        self.filetype = "tasks"
+        self.filemeta = "tasks"
 
 
 class Handler(RoleFile):
@@ -185,7 +186,7 @@ class Handler(RoleFile):
 
     def __init__(self, filename, settings={}, rules=[]):  # noqa
         super().__init__(filename, settings, rules)
-        self.filetype = "handlers"
+        self.filemeta = "handlers"
 
 
 class Vars(Candidate):

ansiblelater/rules/CheckKeyOrder.py

@@ -0,0 +1,89 @@
+# Original code written by the authors of ansible-lint
+
+import functools
+
+from ansiblelater.rule import RuleBase
+
+SORTER_TASKS = (
+    "name",
+    # "__module__",
+    # "action",
+    # "args",
+    None,  # <-- None include all modules that not using action and *
+    # "when",
+    # "notify",
+    # "tags",
+    "block",
+    "rescue",
+    "always",
+)
+
+
+class CheckKeyOrder(RuleBase):
+    rid = "ANS129"
+    description = "Check optimized key order"
+    helptext = "{type} key order can be improved to `{sorted_keys}`"
+    types = ["playbook", "task", "handler"]
+
+    def check(self, candidate, settings):
+        errors = []
+        tasks, err = self.get_normalized_tasks(candidate, settings)
+
+        if err:
+            return self.Result(candidate.path, err)
+
+        for task in tasks:
+            is_sorted, keys = self._sort_keys(task.get("__raw_task__"))
+            if not is_sorted:
+                errors.append(
+                    self.Error(
+                        task["__line__"],
+                        self.helptext.format(type="task", sorted_keys=", ".join(keys)),
+                    )
+                )
+
+        if candidate.kind == "playbook":
+            tasks, err = self.get_tasks(candidate, settings)
+
+            if err:
+                return self.Result(candidate.path, err)
+
+            for task in tasks:
+                is_sorted, keys = self._sort_keys(task)
+                if not is_sorted:
+                    errors.append(
+                        self.Error(
+                            task["__line__"],
+                            self.helptext.format(type="play", sorted_keys=", ".join(keys)),
+                        )
+                    )
+
+        return self.Result(candidate.path, errors)
+
+    @staticmethod
+    def _sort_keys(task):
+        if not task:
+            return True, []
+
+        keys = [str(key) for key in task if not key.startswith("_")]
+        sorted_keys = sorted(keys, key=functools.cmp_to_key(_task_property_sorter))
+
+        return (keys == sorted_keys), sorted_keys
+
+
+def _task_property_sorter(property1, property2):
+    """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)
+
+
+def _get_property_sort_index(name):
+    """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

ansiblelater/rules/CheckWhenFormat.py

@@ -5,8 +5,9 @@ class CheckWhenFormat(RuleBase):
     rid = "ANS122"
     description = "Don't use Jinja2 in when"
     helptext = (
-        "`when` is a raw Jinja2 expression, redundant {{ }} " "should be removed from variable(s)"
+        "`when` is a raw Jinja2 expression, redundant `{{ }}` should be removed from variable(s)"
     )
+
     types = ["playbook", "task", "handler"]
 
     def check(self, candidate, settings):

ansiblelater/utils/yamlhelper.py

@@ -21,6 +21,7 @@
 # THE SOFTWARE.
 
 import codecs
+import copy
 import os
 from contextlib import suppress
 
@@ -436,10 +437,10 @@ def normalize_task(task, filename, custom_modules=None):
     return normalized
 
 
-def action_tasks(yaml, file):
+def action_tasks(yaml, candidate):
     tasks = []
-    if file["filetype"] in ["tasks", "handlers"]:
-        tasks = add_action_type(yaml, file["filetype"])
+    if candidate.filemeta in ["tasks", "handlers"]:
+        tasks = add_action_type(yaml, candidate.filemeta)
     else:
         tasks.extend(extract_from_list(yaml, ["tasks", "handlers", "pre_tasks", "post_tasks"]))
 
@@ -480,7 +481,12 @@ def extract_from_list(blocks, candidates):
                     meta_data = dict(block)
                     for key in delete_meta_keys:
                         meta_data.pop(key, None)
-                    results.extend(add_action_type(block[candidate], candidate, meta_data))
+
+                    actions = add_action_type(block[candidate], candidate, meta_data)
+                    for action in actions:
+                        action["__raw_task__"] = copy.copy(block)
+
+                    results.extend(actions)
                 elif block[candidate] is not None:
                     raise RuntimeError(
                         f"Key '{candidate}' defined, but bad value: '{block[candidate]!s}'"

docs/content/included_rules/_index.md

@@ -44,4 +44,5 @@ Reviews are useless without some rules to check against. `ansible-later` comes w
 | CheckChangedInWhen            | ANS126 | Use handlers instead of `when: changed`.                          |                                                                            |
 | CheckChangedInWhen            | ANS127 | Deprecated bare variables in loops must not be used.              |                                                                            |
 | CheckFQCNBuiltin              | ANS128 | Module actions should use full qualified collection names.        |                                                                            |
+| CheckFQCNBuiltin              | ANS129 | Check optimized playbook/tasks key order.                         |                                                                            |
 | CheckDeprecated               | ANS999 | Deprecated features of `ansible-later` should not be used.        |                                                                            |