From 921aa83979873a059bf9eb257c58bde710397159 Mon Sep 17 00:00:00 2001 From: Sorin Sbarnea Date: Tue, 7 May 2024 10:03:00 +0100 Subject: [PATCH] Prevent execution with incompatible yamllint configuration Fixes: #3890 Fixes: #4118 --- .github/workflows/tox.yml | 2 +- .vscode/settings.json | 6 +- .yamllint | 9 ++ .../yamllint/incompatible-config/.yamllint | 14 +++ src/ansiblelint/data/.yamllint | 25 +++++ src/ansiblelint/rules/yaml.md | 73 ++++++++++----- src/ansiblelint/yaml_utils.py | 91 +++++++++++++------ test/test_yaml_utils.py | 13 ++- 8 files changed, 178 insertions(+), 55 deletions(-) create mode 100644 examples/yamllint/incompatible-config/.yamllint create mode 100644 src/ansiblelint/data/.yamllint diff --git a/.github/workflows/tox.yml b/.github/workflows/tox.yml index 38bf1f4b55..e9c0f79b23 100644 --- a/.github/workflows/tox.yml +++ b/.github/workflows/tox.yml @@ -73,7 +73,7 @@ jobs: env: # Number of expected test passes, safety measure for accidental skip of # tests. Update value if you add/remove tests. - PYTEST_REQPASS: 857 + PYTEST_REQPASS: 858 steps: - uses: actions/checkout@v4 with: diff --git a/.vscode/settings.json b/.vscode/settings.json index efe4f21238..bbfb53d30f 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -25,7 +25,7 @@ "python.testing.unittestEnabled": false, "mypy-type-checker.severity": { "error": "Warning", - }, + }, "sortLines.filterBlankLines": true, "yaml.completion": true, "yaml.customTags": [ @@ -39,6 +39,8 @@ "editor.codeActionsOnSave": { "source.organizeImports": "explicit", "source.fixAll": "explicit" - } + }, + "editor.defaultFormatter": "ms-python.black-formatter", + "editor.formatOnSave": true } } diff --git a/.yamllint b/.yamllint index 59c2fa58ab..472bb681e4 100644 --- a/.yamllint +++ b/.yamllint @@ -1,16 +1,25 @@ --- rules: + braces: + min-spaces-inside: 0 + max-spaces-inside: 1 comments: # prettier compatibility min-spaces-from-content: 1 + comments-indentation: false document-start: present: true + key-duplicates: + forbid-duplicated-merge-keys: true indentation: level: error indent-sequences: consistent octal-values: forbid-implicit-octal: true forbid-explicit-octal: true + # quoted-strings: + # quote-type: double + # required: only-when-needed ignore: | .tox examples/playbooks/example.yml diff --git a/examples/yamllint/incompatible-config/.yamllint b/examples/yamllint/incompatible-config/.yamllint new file mode 100644 index 0000000000..7639dd55c9 --- /dev/null +++ b/examples/yamllint/incompatible-config/.yamllint @@ -0,0 +1,14 @@ +# This config file is full of yamllint configuration settings that are +# incompatible with ansible-lint. It used for testing their detection. +rules: + comments: + min-spaces-from-content: 2 + comments-indentation: false + braces: + min-spaces-inside: 1 + max-spaces-inside: 2 + key-duplicates: + forbid-duplicated-merge-keys: false + octal-values: + forbid-implicit-octal: false + forbid-explicit-octal: false diff --git a/src/ansiblelint/data/.yamllint b/src/ansiblelint/data/.yamllint new file mode 100644 index 0000000000..6ff09f0917 --- /dev/null +++ b/src/ansiblelint/data/.yamllint @@ -0,0 +1,25 @@ +extends: default +rules: + comments: + # https://github.com/prettier/prettier/issues/6780 + min-spaces-from-content: 1 + # https://github.com/adrienverge/yamllint/issues/384 + comments-indentation: false + document-start: disable + # 160 chars was the default used by old E204 rule, but + # you can easily change it or disable in your .yamllint file. + line-length: + max: 160 + # We are adding an extra space inside braces as that's how prettier does it + # and we are trying not to fight other linters. + braces: + min-spaces-inside: 0 # yamllint defaults to 0 + max-spaces-inside: 1 # yamllint defaults to 0 + # key-duplicates: + # forbid-duplicated-merge-keys: true # not enabled by default + octal-values: + forbid-implicit-octal: true # yamllint defaults to false + forbid-explicit-octal: true # yamllint defaults to false + # quoted-strings: + # quote-type: double + # required: only-when-needed diff --git a/src/ansiblelint/rules/yaml.md b/src/ansiblelint/rules/yaml.md index 11d131ead6..654f80e647 100644 --- a/src/ansiblelint/rules/yaml.md +++ b/src/ansiblelint/rules/yaml.md @@ -1,24 +1,8 @@ # yaml -This rule checks YAML syntax by using [yamllint] but with few minor default -configuration changes. - -!!! warning - - [Auto-fix](../autofix.md) functionality will change **inline comment indentation to one - character instead of two**, which is the default of [yamllint]. The reason - for this decision is for keeping reformatting compatibility - with [prettier], which is the most popular reformatter. - - ```yaml title=".yamllint" - rules: - comments: - min-spaces-from-content: 1 # prettier compatibility - ``` - - There is no need to create this yamllint config file, but if you also - run yamllint yourself, you might want to create it to make it behave - the same way as ansible-lint. +This rule checks YAML syntax by using [yamllint] library but with a +[specific default configuration](#yamllint-configuration), one that is +compatible with both, our internal reformatter (`--fix`) and also [prettier]. You can disable YAML syntax violations by adding `yaml` to the `skip_list` in your Ansible-lint configuration as follows: @@ -93,8 +77,10 @@ precedence over our defaults. ## Additional Information for Multiline Strings -Adhering to yaml[line-length] rule, for writing multiline strings we recommend using Block Style Indicator: literal style indicated by a pipe (|) or folded style indicated by a right angle bracket (>), instead of escaping the newlines with backslashes. -Reference [guide] for writing multiple line strings in yaml. +Adhering to yaml[line-length] rule, for writing multiline strings we recommend +using Block Style Indicator: literal style indicated by a pipe (|) or folded +style indicated by a right angle bracket (>), instead of escaping the newlines +with backslashes. Reference [guide] for writing multiple line strings in yaml. ## Problematic code @@ -115,10 +101,53 @@ foo2: "0o777" # <-- Explicitly quoting octal is less risky. bar: ... # Correct comment indentation. ``` +## Yamllint configuration + +If you decide to add a custom yamllint config to your project, ansible-lint +might refuse to run if it detects that some of your options are incompatible and +ask you to correct them. When this happens, you will see a message like the one +below: + +``` +CRITICAL Found incompatible custom yamllint configuration (.yamllint), please either remove the file or edit it to comply with: + - comments.min-spaces-from-content must be 1 + - braces.min-spaces-inside must be 0 + - braces.max-spaces-inside must be 1 + - octal-values.forbid-implicit-octal must be true + - octal-values.forbid-explicit-octal must be true + +Read https://ansible.readthedocs.io/projects/lint/rules/yaml/ for more details regarding why we have these requirements. +``` + +!!! warning + + [Auto-fix](../autofix.md) functionality will change **inline comment indentation to one + character instead of two**, which is the default of [yamllint]. The reason + for this decision was to keep reformatting compatibility + with [prettier], which is the most popular reformatter. + + ```yaml title=".yamllint" + rules: + comments: + min-spaces-from-content: 1 # prettier compatibility + ``` + + There is no need to create this yamllint config file, but if you also + run yamllint yourself, you might want to create it to make it behave + the same way as ansible-lint. + +Below you can find the default yamllint configuration that our linter will use +when there is no custom file present. + +```yaml +{!../src/ansiblelint/data/.yamllint!} +``` + [1.1]: https://yaml.org/spec/1.1/ [1.2.0]: https://yaml.org/spec/1.2.0/ [1.2.2]: https://yaml.org/spec/1.2.2/ [yaml specification]: https://yaml.org/ -[guide]: https://docs.ansible.com/ansible/latest/reference_appendices/YAMLSyntax.html#yaml-basics +[guide]: + https://docs.ansible.com/ansible/latest/reference_appendices/YAMLSyntax.html#yaml-basics [prettier]: https://prettier.io/ [yamllint]: https://yamllint.readthedocs.io/en/stable/ diff --git a/src/ansiblelint/yaml_utils.py b/src/ansiblelint/yaml_utils.py index aa001b89ea..876c1e614a 100644 --- a/src/ansiblelint/yaml_utils.py +++ b/src/ansiblelint/yaml_utils.py @@ -7,6 +7,7 @@ import logging import os import re +import sys from collections.abc import Callable, Iterator, Sequence from io import StringIO from pathlib import Path @@ -30,6 +31,7 @@ ANNOTATION_KEYS, NESTED_TASK_KEYS, PLAYBOOK_TASK_KEYWORDS, + RC, ) from ansiblelint.utils import Task @@ -46,30 +48,6 @@ _logger = logging.getLogger(__name__) -YAMLLINT_CONFIG = """ -extends: default -rules: - comments: - # https://github.com/prettier/prettier/issues/6780 - min-spaces-from-content: 1 - # https://github.com/adrienverge/yamllint/issues/384 - comments-indentation: false - document-start: disable - # 160 chars was the default used by old E204 rule, but - # you can easily change it or disable in your .yamllint file. - line-length: - max: 160 - # We are adding an extra space inside braces as that's how prettier does it - # and we are trying not to fight other linters. - braces: - min-spaces-inside: 0 # yamllint defaults to 0 - max-spaces-inside: 1 # yamllint defaults to 0 - octal-values: - forbid-implicit-octal: true # yamllint defaults to false - forbid-explicit-octal: true # yamllint defaults to false -""" - - def deannotate(data: Any) -> Any: """Remove our annotations like __file__ and __line__ and return a JSON serializable object.""" if isinstance(data, dict): @@ -85,10 +63,9 @@ def deannotate(data: Any) -> Any: return data -@functools.lru_cache(maxsize=1) def load_yamllint_config() -> YamlLintConfig: """Load our default yamllint config and any customized override file.""" - config = YamlLintConfig(content=YAMLLINT_CONFIG) + config = YamlLintConfig(file=Path(__file__).parent / "data" / ".yamllint") # if we detect local yamllint config we use it but raise a warning # as this is likely to get out of sync with our internal config. for path in [ @@ -105,10 +82,66 @@ def load_yamllint_config() -> YamlLintConfig: "internal yamllint config.", file, ) - config_override = YamlLintConfig(file=str(file)) - config_override.extend(config) - config = config_override + custom_config = YamlLintConfig(file=str(file)) + custom_config.extend(config) + config = custom_config break + + # Look for settings incompatible with our reformatting + checks: list[tuple[str, str | int | bool]] = [ + ( + "comments.min-spaces-from-content", + 1, + ), + ( + "comments-indentation", + False, + ), + ( + "braces.min-spaces-inside", + 0, + ), + ( + "braces.max-spaces-inside", + 1, + ), + ( + "octal-values.forbid-implicit-octal", + True, + ), + ( + "octal-values.forbid-explicit-octal", + True, + ), + # ( + # "key-duplicates.forbid-duplicated-merge-keys", # v1.34.0+ + # True, + # ), + # ( + # "quoted-strings.quote-type", "double", + # ), + # ( + # "quoted-strings.required", "only-when-needed", + # ), + ] + errors = [] + for setting, expected_value in checks: + v = config.rules + for key in setting.split("."): + if not isinstance(v, dict): # pragma: no cover + break + if key not in v: # pragma: no cover + break + v = v[key] + if v != expected_value: + msg = f"{setting} must be {str(expected_value).lower()}" + errors.append(msg) + if errors: + nl = "\n" + msg = f"Found incompatible custom yamllint configuration ({file}), please either remove the file or edit it to comply with:{nl} - {nl + ' - '.join(errors)}.{nl}{nl}Read https://ansible.readthedocs.io/projects/lint/rules/yaml/ for more details regarding why we have these requirements." + logging.fatal(msg) + sys.exit(RC.INVALID_CONFIG) + _logger.debug("Effective yamllint rules used: %s", config.rules) return config diff --git a/test/test_yaml_utils.py b/test/test_yaml_utils.py index 08d9edfbbe..3513ed47d8 100644 --- a/test/test_yaml_utils.py +++ b/test/test_yaml_utils.py @@ -1,5 +1,6 @@ """Tests for yaml-related utility functions.""" +# pylint: disable=too-many-lines from __future__ import annotations from io import StringIO @@ -11,7 +12,8 @@ from yamllint.linter import run as run_yamllint import ansiblelint.yaml_utils -from ansiblelint.file_utils import Lintable +from ansiblelint.constants import RC +from ansiblelint.file_utils import Lintable, cwd from ansiblelint.utils import task_in_list if TYPE_CHECKING: @@ -989,3 +991,12 @@ def test_deannotate( ) -> None: """Ensure deannotate works as intended.""" assert ansiblelint.yaml_utils.deannotate(before) == after + + +def test_yamllint_incompatible_config() -> None: + """Ensure we can detect incompatible yamllint settings.""" + with ( + cwd(Path("examples/yamllint/incompatible-config")), + pytest.raises(SystemExit, match=f"^{RC.INVALID_CONFIG}$"), + ): + ansiblelint.yaml_utils.load_yamllint_config()