Add sanity rule with check for bad and disallowed ignores (#3102)

ansible · Mar 2, 2023 · b9d96df · b9d96df
1 parent 5b070f0
commit b9d96df
Show file tree

Hide file tree

Showing 18 changed files with 280 additions and 10 deletions.
diff --git a/.config/dictionary.txt b/.config/dictionary.txt
@@ -143,6 +143,7 @@ geerlingguy
 getmatches
 globbing
 globmatch
+gplv3
 groupname
 hostkey
 hostnames
@@ -172,6 +173,7 @@ keyserver
 konstruktoid
 kubernetes
 kubevirt
+lalo
 languageservice
 letsencrypt
 levelname
@@ -347,6 +349,7 @@ toidentifier
 tomli
 toolset
 tripleo
+tuco
 typehint
 typehints
 ulimits
@@ -388,5 +391,3 @@ xfail
 xunit
 yatesr
 zuul
-tuco
-lalo
diff --git a/.config/requirements.txt b/.config/requirements.txt
@@ -49,7 +49,7 @@ mkdocs==1.4.2
 mkdocs-autorefs==0.4.1
 mkdocs-gen-files==0.4.0
 mkdocs-htmlproofer-plugin==0.10.3
-mkdocs-material==9.0.15
+mkdocs-material==9.1.0
 mkdocs-material-extensions==1.1.1
 mkdocstrings==0.20.0
 mkdocstrings-python==0.8.3

diff --git a/.github/workflows/tox.yml b/.github/workflows/tox.yml
@@ -70,7 +70,7 @@ jobs:
       WSLENV: FORCE_COLOR:PYTEST_REQPASS:TOXENV:GITHUB_STEP_SUMMARY
       # Number of expected test passes, safety measure for accidental skip of
       # tests. Update value if you add/remove tests.
-      PYTEST_REQPASS: 802
+      PYTEST_REQPASS: 806
     steps:
       - name: Activate WSL1
         if: "contains(matrix.shell, 'wsl')"

diff --git a/docs/profiles.md b/docs/profiles.md
@@ -104,7 +104,7 @@ in
 as validated or certified content. It extends [shared](#shared) profile.
 
 - [avoid-dot-notation](https://github.com/ansible/ansible-lint/issues/2174)
-- [disallowed-ignore](https://github.com/ansible/ansible-lint/issues/2121)
+- [sanity](https://github.com/ansible/ansible-lint/issues/2121)
 - [fqcn](rules/fqcn.md)
 - [import-task-no-when](https://github.com/ansible/ansible-lint/issues/2219)
 - [meta-no-dependencies](https://github.com/ansible/ansible-lint/issues/2159)

diff --git a/docs/rules/index.md b/docs/rules/index.md
@@ -45,6 +45,7 @@
 - [risky-shell-pipe][]
 - [role-name][]
 - [run-once][]
+- [sanity][]
 - [schema][]
 - [syntax-check][]
 - [var-naming][]

diff --git a/docs/rules/sanity.md b/docs/rules/sanity.md
@@ -0,0 +1 @@
+../../src/ansiblelint/rules/sanity.md
diff --git a/examples/sanity_ignores/tests/sanity/ignore-2.13.txt b/examples/sanity_ignores/tests/sanity/ignore-2.13.txt
@@ -0,0 +1 @@
+plugins/module_utils/ansible_example_module.py import-3.6!skip          # comment
diff --git a/examples/sanity_ignores/tests/sanity/ignore-2.14.txt b/examples/sanity_ignores/tests/sanity/ignore-2.14.txt
@@ -0,0 +1,2 @@
+plugins/module_utils/ansible_example_module.py compile-2.6!skip
+plugins/module_utils/ansible_example_module.py import-2.6!skip
diff --git a/examples/sanity_ignores/tests/sanity/ignore-2.15.txt b/examples/sanity_ignores/tests/sanity/ignore-2.15.txt
@@ -0,0 +1,2 @@
+plugins/module_utils/ansible_example_module.incorrect-3.6!skip
+#plugins/module_utils/ansible_example_module.py import-3.6!skip
diff --git a/src/ansiblelint/config.py b/src/ansiblelint/config.py
@@ -68,6 +68,7 @@
     {"changelog": "**/changelogs/changelog.yaml"},
     {"yaml": "**/*.{yaml,yml}"},
     {"yaml": "**/.*.{yaml,yml}"},
+    {"sanity-ignore-file": "**/tests/sanity/ignore-*.txt"},
 ]
 
 BASE_KINDS = [

diff --git a/src/ansiblelint/constants.py b/src/ansiblelint/constants.py
@@ -63,6 +63,7 @@ def main():
     "role",  # that is a folder!
     "yaml",  # generic yaml file, previously reported as unknown file type
     "ansible-lint-config",
+    "sanity-ignore-file",  # tests/sanity/ignore file
     "",  # unknown file type
 ]
 

diff --git a/src/ansiblelint/data/profiles.yml b/src/ansiblelint/data/profiles.yml
@@ -108,7 +108,7 @@ production:
   rules:
     avoid-dot-notation:
       url: https://github.com/ansible/ansible-lint/issues/2174
-    disallowed-ignore: # [sanity]
+    sanity:
       url: https://github.com/ansible/ansible-lint/issues/2121
     fqcn:
     import-task-no-when:

diff --git a/src/ansiblelint/rules/sanity.md b/src/ansiblelint/rules/sanity.md
@@ -0,0 +1,49 @@
+# sanity
+
+This rule checks the `tests/sanity/ignore-x.x.txt` file for disallowed ignores.
+This rule is extremely opinionated and enforced by Partner Engineering. The
+currently allowed ruleset is subject to change, but is starting at a minimal
+number of allowed ignores for maximum test enforcement. Any commented-out ignore
+entries are not evaluated.
+
+This rule can produce messages like:
+
+- `sanity[cannot-ignore]` - Ignore file contains {test} at line {line_num},
+  which is not a permitted ignore.
+- `sanity[bad-ignore]` - Ignore file entry at {line_num} is formatted
+  incorrectly. Please review.
+
+Currently allowed ignores are:
+
+- `validate-modules:missing-gplv3-license`
+- `import-2.6`
+- `import-2.6!skip`
+- `import-2.7`
+- `import-2.7!skip`
+- `import-3.5`
+- `import-3.5!skip`
+- `compile-2.6`
+- `compile-2.6!skip`
+- `compile-2.7`
+- `compile-2.7!skip`
+- `compile-3.5`
+- `compile-3.5!skip`
+
+## Problematic code
+
+```
+# tests/sanity/ignore-x.x.txt
+plugins/module_utils/ansible_example_module.py import-3.6!skip
+```
+
+```
+# tests/sanity/ignore-x.x.txt
+plugins/module_utils/ansible_example_module.oops-3.6!skip
+```
+
+## Correct code
+
+```
+# tests/sanity/ignore-x.x.txt
+plugins/module_utils/ansible_example_module.py import-2.7!skip
+```
diff --git a/src/ansiblelint/rules/sanity.py b/src/ansiblelint/rules/sanity.py
@@ -0,0 +1,133 @@
+"""Implementation of sanity rule."""
+from __future__ import annotations
+
+import sys
+from typing import TYPE_CHECKING
+
+from ansiblelint.rules import AnsibleLintRule
+
+# Copyright (c) 2018, Ansible Project
+
+
+if TYPE_CHECKING:
+    from typing import Any
+
+    from ansiblelint.errors import MatchError
+    from ansiblelint.file_utils import Lintable
+
+
+class CheckSanityIgnoreFiles(AnsibleLintRule):
+    """Ignore entries in sanity ignore files must match an allow list."""
+
+    id = "sanity"
+    description = (
+        "Identifies non-allowed entries in the `tests/sanity/ignore*.txt files."
+    )
+    severity = "MEDIUM"
+    tags = ["experimental"]
+    version_added = "v6.14.0"
+
+    # Partner Engineering defines this list. Please contact PE for changes.
+    allowed_ignores = [
+        "validate-modules:missing-gplv3-license",
+        "import-2.6",
+        "import-2.6!skip",
+        "import-2.7",
+        "import-2.7!skip",
+        "import-3.5",
+        "import-3.5!skip",
+        "compile-2.6",
+        "compile-2.6!skip",
+        "compile-2.7",
+        "compile-2.7!skip",
+        "compile-3.5",
+        "compile-3.5!skip",
+    ]
+
+    def matchyaml(self, file: Lintable) -> list[MatchError]:
+        """Evaluate sanity ignore lists for disallowed ignores.
+
+        :param file: Input lintable file that is a match for `sanity-ignore-file`
+        :returns: List of errors matched to the input file
+        """
+        results: list[MatchError] = []
+        test = ""
+
+        if file.kind != "sanity-ignore-file":
+            return []
+
+        with open(file.abspath, encoding="utf-8") as ignore_file:
+            entries = ignore_file.read().splitlines()
+
+            for line_num, entry in enumerate(entries, 1):
+                if entry and entry[0] != "#":
+                    try:
+                        if "#" in entry:
+                            entry, _ = entry.split("#")
+                        (_, test) = entry.split()
+                        if test not in self.allowed_ignores:
+                            results.append(
+                                self.create_matcherror(
+                                    message=f"Ignore file contains {test} at line {line_num}, which is not a permitted ignore.",
+                                    tag="sanity[cannot-ignore]",
+                                    linenumber=line_num,
+                                    filename=file,
+                                )
+                            )
+
+                    except ValueError:
+                        results.append(
+                            self.create_matcherror(
+                                message=f"Ignore file entry at {line_num} is formatted incorrectly. Please review.",
+                                tag="sanity[bad-ignore]",
+                                linenumber=line_num,
+                                filename=file,
+                            )
+                        )
+
+        return results
+
+
+# testing code to be loaded only with pytest or when executed the rule file
+if "pytest" in sys.modules:
+    import pytest
+
+    from ansiblelint.rules import RulesCollection  # pylint: disable=ungrouped-imports
+    from ansiblelint.runner import Runner  # pylint: disable=ungrouped-imports
+
+    @pytest.mark.parametrize(
+        ("test_file", "failures", "tags"),
+        (
+            pytest.param(
+                "examples/sanity_ignores/tests/sanity/ignore-2.14.txt",
+                0,
+                "sanity[cannot-ignore]",
+                id="pass",
+            ),
+            pytest.param(
+                "examples/sanity_ignores/tests/sanity/ignore-2.15.txt",
+                1,
+                "sanity[bad-ignore]",
+                id="fail0",
+            ),
+            pytest.param(
+                "examples/sanity_ignores/tests/sanity/ignore-2.13.txt",
+                1,
+                "sanity[cannot-ignore]",
+                id="fail1",
+            ),
+        ),
+    )
+    def test_sanity_ignore_files(
+        default_rules_collection: RulesCollection,
+        test_file: str,
+        failures: int,
+        tags: str,
+    ) -> None:
+        """Test rule matches."""
+        default_rules_collection.register(CheckSanityIgnoreFiles())
+        results = Runner(test_file, rules=default_rules_collection).run()
+        for result in results:
+            assert result.rule.id == CheckSanityIgnoreFiles().id
+            assert result.tag == tags
+        assert len(results) == failures
diff --git a/src/ansiblelint/schemas/__store__.json b/src/ansiblelint/schemas/__store__.json
@@ -1,6 +1,6 @@
 {
   "ansible-lint-config": {
-    "etag": "45ec120948f291620e297af0d75625ceb79d9295e2ec8b31652948c31ceeb209",
+    "etag": "03cc2882dff22434360b76d333bc58cf88ed1629dc2c4432ae92caa35d6cb61a",
     "url": "https://raw.githubusercontent.com/ansible/ansible-lint/main/src/ansiblelint/schemas/ansible-lint-config.json"
   },
   "ansible-navigator-config": {
@@ -20,7 +20,7 @@
     "url": "https://raw.githubusercontent.com/ansible/ansible-lint/main/src/ansiblelint/schemas/execution-environment.json"
   },
   "galaxy": {
-    "etag": "8a7b0505c5b42a6a0fb065b1e1b6ba864f53d69b54d1cfbeaab1fa4b03b705a7",
+    "etag": "aee96d4b1c497138504cee870723456d2f42465a45a7650928c057836f7289b5",
     "url": "https://raw.githubusercontent.com/ansible/ansible-lint/main/src/ansiblelint/schemas/galaxy.json"
   },
   "inventory": {

diff --git a/test/schemas/negative_test/.ansible-lint.md b/test/schemas/negative_test/.ansible-lint.md
@@ -2,6 +2,69 @@
 
 ```json
 [
+  {
+    "instancePath": "/rules",
+    "keyword": "enum",
+    "message": "must be equal to one of the allowed values",
+    "params": {
+      "allowedValues": [
+        "command-instead-of-module",
+        "command-instead-of-shell",
+        "deprecated-bare-vars",
+        "deprecated-command-syntax",
+        "deprecated-local-action",
+        "deprecated-module",
+        "empty-string-compare",
+        "fqcn",
+        "fqcn[action-core]",
+        "fqcn[action-redirect]",
+        "fqcn[action]",
+        "galaxy",
+        "galaxy[no-changelog]",
+        "galaxy[tags]",
+        "galaxy[version-incorrect]",
+        "galaxy[version-missing]",
+        "ignore-errors",
+        "inline-env-var",
+        "internal-error",
+        "jinja",
+        "key-order",
+        "latest",
+        "literal-compare",
+        "load-failure",
+        "meta-incorrect",
+        "meta-no-info",
+        "meta-no-tags",
+        "meta-runtime",
+        "meta-video-links",
+        "name",
+        "no-changed-when",
+        "no-handler",
+        "no-jinja-when",
+        "no-log-password",
+        "loop-var-prefix",
+        "no-prompting",
+        "no-relative-paths",
+        "no-same-owner",
+        "no-tabs",
+        "only-builtins",
+        "package-latest",
+        "parser-error",
+        "partial-become",
+        "playbook-extension",
+        "risky-file-permissions",
+        "risky-octal",
+        "risky-shell-pipe",
+        "role-name",
+        "schema",
+        "syntax-check",
+        "var-naming",
+        "yaml"
+      ]
+    },
+    "propertyName": "Wrong_Rule_name",
+    "schemaPath": "#/properties/rules/propertyNames/oneOf/0/enum"
+  },
   {
     "instancePath": "/rules",
     "keyword": "pattern",
@@ -10,7 +73,17 @@
       "pattern": "^[a-z0-9-\\[\\]]+$"
     },
     "propertyName": "Wrong_Rule_name",
-    "schemaPath": "#/properties/rules/propertyNames/pattern"
+    "schemaPath": "#/properties/rules/propertyNames/oneOf/1/pattern"
+  },
+  {
+    "instancePath": "/rules",
+    "keyword": "oneOf",
+    "message": "must match exactly one schema in oneOf",
+    "params": {
+      "passingSchemas": null
+    },
+    "propertyName": "Wrong_Rule_name",
+    "schemaPath": "#/properties/rules/propertyNames/oneOf"
   },
   {
     "instancePath": "/rules",