Remove support for deprecated include (#3722)

ansible · Nov 28, 2023 · 868ebac · 868ebac
1 parent d7c9bff
commit 868ebac
Show file tree

Hide file tree

Showing 10 changed files with 218 additions and 18 deletions.
diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
@@ -34,7 +34,7 @@ repos:
       - id: check-useless-excludes
   - repo: https://github.com/pre-commit/mirrors-prettier
     # keep it before yamllint
-    rev: v3.0.3
+    rev: v3.1.0
     hooks:
       - id: prettier
         # Temporary excludes so we can gradually normalize the formatting

diff --git a/examples/playbooks/blockincludes.yml b/examples/playbooks/blockincludes.yml
@@ -14,7 +14,7 @@
             - name: Block level 3
               block:
                 - name: Include under block level 3 # noqa: deprecated-module
-                  ansible.builtin.include: "{{ varset }}.yml"
+                  ansible.builtin.include_tasks: "{{ varset }}.yml"
                 - name: Block level 4
                   block:
                     - name: INCLUDE under block level 4

diff --git a/examples/playbooks/removed-include.yml b/examples/playbooks/removed-include.yml
@@ -0,0 +1,6 @@
+---
+- name: Invalid playbook
+  hosts: localhost
+  tasks:
+    - name: Foo
+      include: tasks/simple_task.yml # <-- include was removed in 2.16
diff --git a/src/ansiblelint/schemas/ansible.json b/src/ansiblelint/schemas/ansible.json
@@ -1,5 +1,10 @@
 {
   "$defs": {
+    "removed-include-module": {
+      "markdownDescription": "See [include module](https://docs.ansible.com/ansible/latest/collections/ansible/builtin/include_module.html)",
+      "not": {},
+      "title": "Replace 'include' with either 'ansible.builtin.include_tasks' or 'ansible.builtin.import_tasks'"
+    },
     "ansible.builtin.import_playbook": {
       "additionalProperties": false,
       "oneOf": [
@@ -831,6 +836,15 @@
           "title": "Action",
           "type": "string"
         },
+        "ansible.builtin.include": {
+          "$ref": "#/$defs/removed-include-module"
+        },
+        "include": {
+          "$ref": "#/$defs/removed-include-module"
+        },
+        "ansible.legacy.include": {
+          "$ref": "#/$defs/removed-include-module"
+        },
         "any_errors_fatal": {
           "$ref": "#/$defs/templated-boolean",
           "title": "Any Errors Fatal"

diff --git a/src/ansiblelint/schemas/main.py b/src/ansiblelint/schemas/main.py
@@ -55,19 +55,29 @@ def validate_file_schema(file: Lintable) -> list[str]:
             return []
         if error.context:
             error = find_best_deep_match(error)
-        message = f"{error.json_path} {error.message}"
+        # determine if we want to use our own messages embedded into schemas inside title/markdownDescription fields
+        if "not" in error.schema and len(error.schema["not"]) == 0:
+            message = error.schema["title"]
+            schema = error.schema
+        else:
+            message = f"{error.json_path} {error.message}"
 
         documentation_url = ""
-        for k in ("description", "markdownDescription"):
-            if k in schema:
-                # Find standalone URLs and also markdown urls.
-                match = re.search(
-                    r"\[.*?\]\((https?://[^\s]+)\)|https?://[^\s]+",
-                    schema[k],
-                )
-                if match:
-                    documentation_url = match[0] if match[0] else match[1]
-                    break
+        for json_schema in (error.schema, schema):
+            for k in ("description", "markdownDescription"):
+                if k in json_schema:
+                    # Find standalone URLs and also markdown urls.
+                    match = re.search(
+                        r"\[.*?\]\((?P<url>https?://[^\s]+)\)|(?P<url2>https?://[^\s]+)",
+                        json_schema[k],
+                    )
+                    if match:
+                        documentation_url = next(
+                            x for x in match.groups() if x is not None
+                        )
+                        break
+            if documentation_url:
+                break
         if documentation_url:
             if not message.endswith("."):
                 message += "."
@@ -86,7 +96,7 @@ def validate_file_schema(file: Lintable) -> list[str]:
                     schema[k],
                 )
                 if match:
-                    documentation_url = match[0] if match[0] else match[1]
+                    documentation_url = match.groups()[0]
                     break
         if documentation_url:
             if not message.endswith("."):

diff --git a/src/ansiblelint/schemas/playbook.json b/src/ansiblelint/schemas/playbook.json
@@ -796,6 +796,11 @@
       "title": "play-role",
       "type": "object"
     },
+    "removed-include-module": {
+      "markdownDescription": "See [include module](https://docs.ansible.com/ansible/latest/collections/ansible/builtin/include_module.html)",
+      "not": {},
+      "title": "Replace 'include' with either 'ansible.builtin.include_tasks' or 'ansible.builtin.import_tasks'"
+    },
     "tags": {
       "anyOf": [
         {
@@ -847,6 +852,12 @@
           "title": "Action",
           "type": "string"
         },
+        "ansible.builtin.include": {
+          "$ref": "#/$defs/removed-include-module"
+        },
+        "ansible.legacy.include": {
+          "$ref": "#/$defs/removed-include-module"
+        },
         "any_errors_fatal": {
           "$ref": "#/$defs/templated-boolean",
           "title": "Any Errors Fatal"
@@ -932,6 +943,9 @@
           "title": "Ignore Unreachable",
           "type": "boolean"
         },
+        "include": {
+          "$ref": "#/$defs/removed-include-module"
+        },
         "listen": {
           "anyOf": [
             {

diff --git a/src/ansiblelint/schemas/tasks.json b/src/ansiblelint/schemas/tasks.json
@@ -238,6 +238,11 @@
       "markdownDescription": "Use for protecting sensitive data. See [no_log](https://docs.ansible.com/ansible/latest/reference_appendices/logging.html)",
       "title": "no_log"
     },
+    "removed-include-module": {
+      "markdownDescription": "See [include module](https://docs.ansible.com/ansible/latest/collections/ansible/builtin/include_module.html)",
+      "not": {},
+      "title": "Replace 'include' with either 'ansible.builtin.include_tasks' or 'ansible.builtin.import_tasks'"
+    },
     "tags": {
       "anyOf": [
         {
@@ -289,6 +294,12 @@
           "title": "Action",
           "type": "string"
         },
+        "ansible.builtin.include": {
+          "$ref": "#/$defs/removed-include-module"
+        },
+        "ansible.legacy.include": {
+          "$ref": "#/$defs/removed-include-module"
+        },
         "any_errors_fatal": {
           "$ref": "#/$defs/templated-boolean",
           "title": "Any Errors Fatal"
@@ -374,6 +385,9 @@
           "title": "Ignore Unreachable",
           "type": "boolean"
         },
+        "include": {
+          "$ref": "#/$defs/removed-include-module"
+        },
         "listen": {
           "anyOf": [
             {

diff --git a/test/schemas/negative_test/playbooks/include.yml b/test/schemas/negative_test/playbooks/include.yml
@@ -0,0 +1,3 @@
+- hosts: localhost
+  tasks:
+    - include: foo.yml # <-- removed in Ansible 2.16
diff --git a/test/schemas/negative_test/playbooks/include.yml.md b/test/schemas/negative_test/playbooks/include.yml.md
@@ -0,0 +1,142 @@
+# ajv errors
+
+```json
+[
+  {
+    "instancePath": "/0",
+    "keyword": "required",
+    "message": "must have required property 'ansible.builtin.import_playbook'",
+    "params": {
+      "missingProperty": "ansible.builtin.import_playbook"
+    },
+    "schemaPath": "#/oneOf/0/required"
+  },
+  {
+    "instancePath": "/0",
+    "keyword": "required",
+    "message": "must have required property 'import_playbook'",
+    "params": {
+      "missingProperty": "import_playbook"
+    },
+    "schemaPath": "#/oneOf/1/required"
+  },
+  {
+    "instancePath": "/0",
+    "keyword": "oneOf",
+    "message": "must match exactly one schema in oneOf",
+    "params": {
+      "passingSchemas": null
+    },
+    "schemaPath": "#/oneOf"
+  },
+  {
+    "instancePath": "/0",
+    "keyword": "additionalProperties",
+    "message": "must NOT have additional properties",
+    "params": {
+      "additionalProperty": "hosts"
+    },
+    "schemaPath": "#/additionalProperties"
+  },
+  {
+    "instancePath": "/0",
+    "keyword": "additionalProperties",
+    "message": "must NOT have additional properties",
+    "params": {
+      "additionalProperty": "tasks"
+    },
+    "schemaPath": "#/additionalProperties"
+  },
+  {
+    "instancePath": "/0/tasks/0",
+    "keyword": "required",
+    "message": "must have required property 'block'",
+    "params": {
+      "missingProperty": "block"
+    },
+    "schemaPath": "#/required"
+  },
+  {
+    "instancePath": "/0/tasks/0/include",
+    "keyword": "not",
+    "message": "must NOT be valid",
+    "params": {},
+    "schemaPath": "#/$defs/removed-include-module/not"
+  },
+  {
+    "instancePath": "/0/tasks/0",
+    "keyword": "anyOf",
+    "message": "must match a schema in anyOf",
+    "params": {},
+    "schemaPath": "#/items/anyOf"
+  },
+  {
+    "instancePath": "/0",
+    "keyword": "oneOf",
+    "message": "must match exactly one schema in oneOf",
+    "params": {
+      "passingSchemas": null
+    },
+    "schemaPath": "#/items/oneOf"
+  }
+]
+```
+
+# check-jsonschema
+
+stdout:
+
+```json
+{
+  "status": "fail",
+  "successes": [],
+  "errors": [
+    {
+      "filename": "negative_test/playbooks/include.yml",
+      "path": "$[0]",
+      "message": "{'hosts': 'localhost', 'tasks': [{'include': 'foo.yml'}]} is not valid under any of the given schemas",
+      "has_sub_errors": true,
+      "best_match": {
+        "path": "$[0]",
+        "message": "'hosts', 'tasks' do not match any of the regexes: '^(ansible\\\\.builtin\\\\.)?import_playbook$', 'name', 'tags', 'vars', 'when'"
+      },
+      "best_deep_match": {
+        "path": "$[0].tasks[0].include",
+        "message": "'foo.yml' should not be valid under {}"
+      },
+      "num_sub_errors": 6,
+      "sub_errors": [
+        {
+          "path": "$[0]",
+          "message": "'hosts', 'tasks' do not match any of the regexes: '^(ansible\\\\.builtin\\\\.)?import_playbook$', 'name', 'tags', 'vars', 'when'"
+        },
+        {
+          "path": "$[0]",
+          "message": "{'hosts': 'localhost', 'tasks': [{'include': 'foo.yml'}]} is not valid under any of the given schemas"
+        },
+        {
+          "path": "$[0]",
+          "message": "'ansible.builtin.import_playbook' is a required property"
+        },
+        {
+          "path": "$[0]",
+          "message": "'import_playbook' is a required property"
+        },
+        {
+          "path": "$[0].tasks[0]",
+          "message": "{'include': 'foo.yml'} is not valid under any of the given schemas"
+        },
+        {
+          "path": "$[0].tasks[0]",
+          "message": "'block' is a required property"
+        },
+        {
+          "path": "$[0].tasks[0].include",
+          "message": "'foo.yml' should not be valid under {}"
+        }
+      ]
+    }
+  ],
+  "parse_errors": []
+}
+```
diff --git a/test/test_task_includes.py b/test/test_task_includes.py
@@ -1,5 +1,4 @@
 """Tests related to task inclusions."""
-import sys
 
 import pytest
 
@@ -14,9 +13,7 @@
         pytest.param(
             "examples/playbooks/blockincludes.yml",
             4,
-            3
-            if sys.version_info >= (3, 10, 0)
-            else 4,  # 3 with py310/ansible2.16, or 4 with py39/ansible2.15,
+            3,
             id="blockincludes",
         ),
         pytest.param(