From b9d96dff73cc5aee5e99f25298774eed17a9296b Mon Sep 17 00:00:00 2001 From: Alison Hart Date: Thu, 2 Mar 2023 05:59:30 -0500 Subject: [PATCH] Add sanity rule with check for bad and disallowed ignores (#3102) --- .config/dictionary.txt | 5 +- .config/requirements.txt | 2 +- .github/workflows/tox.yml | 2 +- docs/profiles.md | 2 +- docs/rules/index.md | 1 + docs/rules/sanity.md | 1 + .../tests/sanity/ignore-2.13.txt | 1 + .../tests/sanity/ignore-2.14.txt | 2 + .../tests/sanity/ignore-2.15.txt | 2 + src/ansiblelint/config.py | 1 + src/ansiblelint/constants.py | 1 + src/ansiblelint/data/profiles.yml | 2 +- src/ansiblelint/rules/sanity.md | 49 +++++++ src/ansiblelint/rules/sanity.py | 133 ++++++++++++++++++ src/ansiblelint/schemas/__store__.json | 4 +- test/schemas/negative_test/.ansible-lint.md | 75 +++++++++- test/test_file_utils.py | 5 + test/test_rules_collection.py | 2 +- 18 files changed, 280 insertions(+), 10 deletions(-) create mode 120000 docs/rules/sanity.md create mode 100644 examples/sanity_ignores/tests/sanity/ignore-2.13.txt create mode 100644 examples/sanity_ignores/tests/sanity/ignore-2.14.txt create mode 100644 examples/sanity_ignores/tests/sanity/ignore-2.15.txt create mode 100644 src/ansiblelint/rules/sanity.md create mode 100644 src/ansiblelint/rules/sanity.py diff --git a/.config/dictionary.txt b/.config/dictionary.txt index d20b5c9fd0..6a7640d6ea 100644 --- 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 index 62f22bcfdc..f89c394499 100644 --- 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 index 2c35b1f84f..5c25b58c3c 100644 --- 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 index 815c62a61c..95e4598f4b 100644 --- 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 index e204ea722b..dd4d057c3d 100644 --- 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 new file mode 120000 index 0000000000..71346b4a3c --- /dev/null +++ b/docs/rules/sanity.md @@ -0,0 +1 @@ +../../src/ansiblelint/rules/sanity.md \ No newline at end of file diff --git a/examples/sanity_ignores/tests/sanity/ignore-2.13.txt b/examples/sanity_ignores/tests/sanity/ignore-2.13.txt new file mode 100644 index 0000000000..81fb3a5d52 --- /dev/null +++ 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 new file mode 100644 index 0000000000..d86f85256a --- /dev/null +++ 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 new file mode 100644 index 0000000000..069ef15fef --- /dev/null +++ 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 index c9262c942b..f784beb2d1 100644 --- 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 index 0fec8fdd6a..3b4a0ebc42 100644 --- 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 index 8de92fdd28..103795e797 100644 --- 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 new file mode 100644 index 0000000000..6eb2988e14 --- /dev/null +++ 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 new file mode 100644 index 0000000000..47e25f5701 --- /dev/null +++ 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 index 65f064dd3d..371962a772 100644 --- 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 index 1bc97bfb8e..22538e0e4a 100644 --- 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", diff --git a/test/test_file_utils.py b/test/test_file_utils.py index b427fa833a..adc9d527b0 100644 --- a/test/test_file_utils.py +++ b/test/test_file_utils.py @@ -229,6 +229,11 @@ def test_discover_lintables_umlaut(monkeypatch: MonkeyPatch) -> None: pytest.param( "examples/playbooks/vars/subfolder/settings.yaml", "vars", id="40" ), + pytest.param( + "examples/sanity_ignores/tests/sanity/ignore-2.14.txt", + "sanity-ignore-file", + id="41", + ), ), ) def test_kinds(monkeypatch: MonkeyPatch, path: str, kind: FileType) -> None: diff --git a/test/test_rules_collection.py b/test/test_rules_collection.py index 60a2b83cc0..1939d253b0 100644 --- a/test/test_rules_collection.py +++ b/test/test_rules_collection.py @@ -166,5 +166,5 @@ def test_rules_id_format() -> None: rule.help != "" or rule.description or rule.__doc__ ), f"Rule {rule.id} must have at least one of: .help, .description, .__doc__" assert "yaml" in keys, "yaml rule is missing" - assert len(rules) == 50 # update this number when adding new rules! + assert len(rules) == 51 # update this number when adding new rules! assert len(keys) == len(rules), "Duplicate rule ids?"