From 2d0ffa14ffe6a9fb81b91d43b83f86a8e6fe9ee3 Mon Sep 17 00:00:00 2001 From: Felix Fontein Date: Sat, 19 Oct 2024 21:07:32 +0200 Subject: [PATCH] Add --strict parameter to lint-changelog-yaml subcommand. --- changelogs/fragments/182-strict-linting.yml | 2 + docs/changelog.yaml-format.md | 2 + noxfile.py | 15 +++++- src/antsibull_changelog/cli.py | 12 ++++- src/antsibull_changelog/lint.py | 54 +++++++++++++++++++ tests/functional/bad/ansible-base-1.json | 3 ++ .../bad/felixfontein.hosttech_dns-1.json | 4 ++ tests/functional/bad/random-1.json | 26 +++++++++ tests/functional/bad/random-2.json | 3 ++ tests/functional/bad/random-3.json | 3 ++ tests/functional/bad/random-4.json | 15 ++++++ tests/functional/bad/random-4.yaml | 5 ++ .../functional/test_changelog_yaml_linter.py | 29 +++++++++- 13 files changed, 168 insertions(+), 5 deletions(-) create mode 100644 changelogs/fragments/182-strict-linting.yml diff --git a/changelogs/fragments/182-strict-linting.yml b/changelogs/fragments/182-strict-linting.yml new file mode 100644 index 00000000..dd4c15ff --- /dev/null +++ b/changelogs/fragments/182-strict-linting.yml @@ -0,0 +1,2 @@ +minor_changes: + - "Add ``--strict`` parameter to the ``lint-changelog-yaml`` subcommand to also check for extra fields that should not be there (https://github.com/ansible-community/antsibull-changelog/pull/182)." diff --git a/docs/changelog.yaml-format.md b/docs/changelog.yaml-format.md index 2b20ae85..bd3a9fd6 100644 --- a/docs/changelog.yaml-format.md +++ b/docs/changelog.yaml-format.md @@ -21,6 +21,8 @@ You can use the `antsibull-changelog lint-changelog-yaml` tool included in the [ The tool does not output anything and exits with exit code 0 in case the file is OK, and outputs errors and exits with exit code 3 in case an error was found. Other exit codes indicate problems with the command line or during the execution of the linter. +To avoid extra data in `changelog.yaml` that should not be in there, add the `--strict` option. This can be useful to avoid typos, for example if you wrote `change:` instead of `changes:`, or forgot `changes:` alltogether. + ## changelog.yaml diff --git a/noxfile.py b/noxfile.py index 206ace5f..c3d43b3e 100644 --- a/noxfile.py +++ b/noxfile.py @@ -125,6 +125,14 @@ def integration(session: nox.Session): "changelogs/changelog.yaml", ) + # Lint own changelogs/changelog.yaml in strict mode + cov_run( + "lint-changelog-yaml", + "--no-semantic-versioning", + "--strict", + "changelogs/changelog.yaml", + ) + # Lint community.general's changelogs/changelog.yaml cg_destination = tmp / "community.general" if not cg_destination.exists(): @@ -133,7 +141,7 @@ def integration(session: nox.Session): "git", "clone", "https://github.com/ansible-collections/community.general", - "--branch=stable-4", + "--branch=stable-9", "--depth=1", external=True, ) @@ -141,6 +149,11 @@ def integration(session: nox.Session): "lint-changelog-yaml", str(cg_destination / "changelogs" / "changelog.yaml"), ) + cov_run( + "lint-changelog-yaml", + str(cg_destination / "changelogs" / "changelog.yaml"), + "--strict", + ) combined = map(str, tmp.glob(".coverage.*")) session.run("coverage", "combine", *combined, env=env) diff --git a/src/antsibull_changelog/cli.py b/src/antsibull_changelog/cli.py index 62b04e05..c4653463 100644 --- a/src/antsibull_changelog/cli.py +++ b/src/antsibull_changelog/cli.py @@ -180,7 +180,7 @@ def create_argparser(program_name: str) -> argparse.ArgumentParser: lint_changelog_yaml_parser = subparsers.add_parser( "lint-changelog-yaml", parents=[common], - help="check syntax of" " changelogs/changelog.yaml file", + help="check syntax of changelogs/changelog.yaml file", ) lint_changelog_yaml_parser.set_defaults(func=command_lint_changelog_yaml) lint_changelog_yaml_parser.add_argument( @@ -188,6 +188,12 @@ def create_argparser(program_name: str) -> argparse.ArgumentParser: metavar="/path/to/changelog.yaml", help="path to changelogs/changelog.yaml", ) + lint_changelog_yaml_parser.add_argument( + "--strict", + type=parse_boolean_arg, + help="do more strict checking, for example complain about extra" + " entries that are not mentioned in the changelog.yaml specification", + ) lint_changelog_yaml_parser.add_argument( "--no-semantic-versioning", @@ -834,7 +840,9 @@ def command_lint_changelog_yaml(args: Any) -> int: :arg args: Parsed arguments """ errors = lint_changelog_yaml( - args.changelog_yaml_path, no_semantic_versioning=args.no_semantic_versioning + args.changelog_yaml_path, + no_semantic_versioning=args.no_semantic_versioning, + strict=args.strict, ) messages = sorted( diff --git a/src/antsibull_changelog/lint.py b/src/antsibull_changelog/lint.py index 56c64f8c..9a93f4ba 100644 --- a/src/antsibull_changelog/lint.py +++ b/src/antsibull_changelog/lint.py @@ -33,6 +33,7 @@ class ChangelogYamlLinter: errors: list[tuple[str, int, int, str]] path: str + strict: bool def __init__( self, @@ -40,6 +41,7 @@ def __init__( *, no_semantic_versioning: bool = False, preprocess_data: Callable[[dict], None] | None = None, + strict: bool = False, ): self.errors = [] self.path = path @@ -47,6 +49,7 @@ def __init__( self.valid_plugin_types.update(OTHER_PLUGIN_TYPES) self.no_semantic_versioning = no_semantic_versioning self.preprocess_data = preprocess_data + self.strict = strict def check_version(self, version: Any, message: str) -> Any | None: """ @@ -75,6 +78,27 @@ def check_version(self, version: Any, message: str) -> Any | None: ) return None + def check_extra_entries( + self, data: dict, allowed_keys: set[str], yaml_path: list[Any] + ) -> None: + """ + Verify that no extra keys than the ones from ``allowed_keys`` appear in ``data``. + """ + if not self.strict: + return + for key in data: + if key not in allowed_keys: + self.errors.append( + ( + self.path, + 0, + 0, + "{0}: extra key not allowed".format( + self._format_yaml_path(yaml_path + [key]), + ), + ) + ) + @staticmethod def _format_yaml_path(yaml_path: list[Any]) -> str: """ @@ -181,6 +205,16 @@ def verify_plugin( ), ) ) + self.check_extra_entries( + plugin, + { + "release_date", + "name", + "description", + "namespace", + }, + yaml_path, + ) def lint_plugins(self, version_str: str, plugins_dict: dict): """ @@ -368,6 +402,20 @@ def lint_releases_entry( fragment, (str,), ["releases", version_str, "fragments", idx] ) + self.check_extra_entries( + entry, + { + "release_date", + "codename", + "changes", + "modules", + "plugins", + "objects", + "fragments", + }, + ["releases", version_str], + ) + def lint_content(self, changelog_yaml: dict) -> None: """ Lint the contents of a changelog.yaml file, provided it is a global mapping. @@ -402,6 +450,8 @@ def lint_content(self, changelog_yaml: dict) -> None: if self.verify_type(entry, (dict,), ["releases", version_str]): self.lint_releases_entry(fragment_linter, version_str, entry) + self.check_extra_entries(changelog_yaml, {"releases", "ancestor"}, []) + def lint(self) -> list[tuple[str, int, int, str]]: """ Load and lint the changelog.yaml file. @@ -442,6 +492,7 @@ def lint_changelog_yaml( *, no_semantic_versioning: bool = False, preprocess_data: Callable[[dict], None] | None = None, + strict: bool = False, ) -> list[tuple[str, int, int, str]]: """ Lint a changelogs/changelog.yaml file. @@ -450,9 +501,12 @@ def lint_changelog_yaml( semantic versioning, but Python version numbers. :kwarg preprocess_data: If provided, will be called on the data loaded before it is checked. This can be used to remove extra data before validation. + :kwarg strict: Set to ``True`` to enable more strict validation + (complain about extra fields). """ return ChangelogYamlLinter( path, no_semantic_versioning=no_semantic_versioning, preprocess_data=preprocess_data, + strict=strict, ).lint() diff --git a/tests/functional/bad/ansible-base-1.json b/tests/functional/bad/ansible-base-1.json index 0897adff..054e45bb 100644 --- a/tests/functional/bad/ansible-base-1.json +++ b/tests/functional/bad/ansible-base-1.json @@ -1,5 +1,8 @@ { "errors": [ [0, 0, "Invalid ancestor version: error while parse version '2.9.0b1': Invalid version string: '2.9.0b1'"] + ], + "errors_strict": [ + [0, 0, "Invalid ancestor version: error while parse version '2.9.0b1': Invalid version string: '2.9.0b1'"] ] } diff --git a/tests/functional/bad/felixfontein.hosttech_dns-1.json b/tests/functional/bad/felixfontein.hosttech_dns-1.json index 96880d22..5c7d0e29 100644 --- a/tests/functional/bad/felixfontein.hosttech_dns-1.json +++ b/tests/functional/bad/felixfontein.hosttech_dns-1.json @@ -2,5 +2,9 @@ "errors": [ [0, 0, "'releases' -> '1.0.0' -> 'modules' -> 0 -> 'name' must not be a FQCN"], [0, 0, "'releases' -> '1.0.0' -> 'modules' -> 1 -> 'name' must not be a FQCN"] + ], + "errors_strict": [ + [0, 0, "'releases' -> '1.0.0' -> 'modules' -> 0 -> 'name' must not be a FQCN"], + [0, 0, "'releases' -> '1.0.0' -> 'modules' -> 1 -> 'name' must not be a FQCN"] ] } diff --git a/tests/functional/bad/random-1.json b/tests/functional/bad/random-1.json index e55594d2..b3a74f19 100644 --- a/tests/functional/bad/random-1.json +++ b/tests/functional/bad/random-1.json @@ -24,5 +24,31 @@ [0, 0, "Invalid release version: error while parse version 2.0: Expecting string"], [0, 0, "'releases' -> 2.0 is expected to be , but got \\['lists', 'are not allowed'\\]"], [0, 0, "'releases' -> '3.0.0' is expected to be , but got 42"] + ], + "errors_strict": [ + [0, 0, "Invalid ancestor version: error while parse version \\['test'\\]: Expecting string"], + [0, 0, "'releases' -> '1.0.0' -> 'release_date' must be a ISO date \\(YYYY-MM-DD\\)"], + [0, 0, "'releases' -> '1.0.0' -> 'changes': invalid section: invalid_category"], + [0, 0, "'releases' -> '1.0.0' -> 'changes': section \"invalid_category\" list items must be type str not int"], + [0, 0, "'releases' -> '1.0.0' -> 'changes': section \"minor_changes\" must be type list not dict"], + [0, 0, "'releases' -> '1.0.0' -> 'changes': section \"major_changes\" must be type list not int"], + [0, 0, "'releases' -> '1.0.0' -> 'modules' -> 0 -> 'name' must not be a FQCN"], + [0, 0, "'releases' -> '1.0.0' -> 'modules' -> 1 -> 'description' is expected to be , but got None"], + [0, 0, "'releases' -> '1.0.0' -> 'modules' -> 1 -> 'namespace' must not contain spaces or slashes"], + [0, 0, "'releases' -> '1.0.0' -> 'modules' -> 2 -> 'namespace' must not contain spaces or slashes"], + [0, 0, "'releases' -> '1.0.0' -> 'modules' -> 3 -> 'namespace' must not contain spaces or slashes"], + [0, 0, "'releases' -> '1.0.0' -> 'modules' -> 4 -> 'namespace' is expected to be , but got None"], + [0, 0, "'releases' -> '1.0.0' -> 'modules' -> 5 -> 'namespace' is expected to be , but got None"], + [0, 0, "'releases' -> '1.0.0' -> 'plugins' -> 'lookup' -> 0 -> 'description' is expected to be , but got 123"], + [0, 0, "'releases' -> '1.0.0' -> 'plugins' -> 'lookup' -> 0 -> 'namespace' must be null"], + [0, 0, "Unknown plugin type 'woo' in 'releases' -> '1.0.0' -> 'plugins'"], + [0, 0, "Unknown plugin type 'screwed' in 'releases' -> '1.0.0' -> 'plugins'"], + [0, 0, "'releases' -> '1.0.0' -> 'plugins' -> 'screwed' is expected to be , but got 321"], + [0, 0, "'releases' -> '1.0.0' -> 'fragments' is expected to be null or , but got {'files': 123}"], + [0, 0, "'releases' -> '1.1.0' -> 'release_date' is expected to be , but got 12345"], + [0, 0, "'releases' -> '1.1.0' -> 'changes': section \"release_summary\" must be type str not int"], + [0, 0, "Invalid release version: error while parse version 2.0: Expecting string"], + [0, 0, "'releases' -> 2.0 is expected to be , but got \\['lists', 'are not allowed'\\]"], + [0, 0, "'releases' -> '3.0.0' is expected to be , but got 42"] ] } diff --git a/tests/functional/bad/random-2.json b/tests/functional/bad/random-2.json index eef08e8e..37fbacf3 100644 --- a/tests/functional/bad/random-2.json +++ b/tests/functional/bad/random-2.json @@ -1,5 +1,8 @@ { "errors": [ [0, 0, "error while parsing YAML: .*in \"input.yaml\", line 7, column 1"] + ], + "errors_strict": [ + [0, 0, "error while parsing YAML: .*in \"input.yaml\", line 7, column 1"] ] } diff --git a/tests/functional/bad/random-3.json b/tests/functional/bad/random-3.json index 2e7e917d..ade4506b 100644 --- a/tests/functional/bad/random-3.json +++ b/tests/functional/bad/random-3.json @@ -1,5 +1,8 @@ { "errors": [ [0, 0, "'releases' is expected to be , but got \\[\\]"] + ], + "errors_strict": [ + [0, 0, "'releases' is expected to be , but got \\[\\]"] ] } diff --git a/tests/functional/bad/random-4.json b/tests/functional/bad/random-4.json index d1a0b6cd..8529df77 100644 --- a/tests/functional/bad/random-4.json +++ b/tests/functional/bad/random-4.json @@ -8,5 +8,20 @@ [0, 0, "'releases' -> '1.0.0' -> 'plugins' -> 'callback' -> 1 -> 'description' is expected to be , but got 234"], [0, 0, "'releases' -> '1.0.0' -> 'plugins' -> 'callback' -> 1 -> 'namespace' must be null"], [0, 0, "Unknown object type 'chair' in 'releases' -> '1.0.0' -> 'objects'"] + ], + "errors_strict": [ + [0, 0, "release version '1.0.0' must come after ancestor version '1.0.0'"], + [0, 0, "'releases' -> '1.0.0' -> 'plugins' is expected to be , but got 23"], + [0, 0, "Unknown plugin type 'foo' in 'releases' -> '1.0.0' -> 'plugins'"], + [0, 0, "'releases' -> '1.0.0' -> 'plugins' -> 'callback' -> 0 is expected to be , but got 42"], + [0, 0, "'releases' -> '1.0.0' -> 'plugins' -> 'callback' -> 1 -> 'name' is expected to be , but got 123"], + [0, 0, "'releases' -> '1.0.0' -> 'plugins' -> 'callback' -> 1 -> 'description' is expected to be , but got 234"], + [0, 0, "'releases' -> '1.0.0' -> 'plugins' -> 'callback' -> 1 -> 'namespace' must be null"], + [0, 0, "'releases' -> '1.0.0' -> 'plugins' -> 'callback' -> 2 -> 'extra_callback': extra key not allowed"], + [0, 0, "Unknown object type 'chair' in 'releases' -> '1.0.0' -> 'objects'"], + [0, 0, "'releases' -> '1.0.0' -> 'objects' -> 'chair' -> 0 -> 'extra_chair': extra key not allowed"], + [0, 0, "'releases' -> '1.0.0' -> 'objects' -> 'role' -> 0 -> 'extra_role': extra key not allowed"], + [0, 0, "'releases' -> '1.0.0' -> 'extra_release': extra key not allowed"], + [0, 0, "'extra_global': extra key not allowed"] ] } diff --git a/tests/functional/bad/random-4.yaml b/tests/functional/bad/random-4.yaml index 6f75f825..e501f3cc 100644 --- a/tests/functional/bad/random-4.yaml +++ b/tests/functional/bad/random-4.yaml @@ -7,6 +7,7 @@ ancestor: 1.0.0 releases: 1.0.0: release_date: '2020-01-01' + extra_release: shouldn't be there plugins: 23: [] foo: [] @@ -17,12 +18,16 @@ releases: namespace: 345 - name: foo description: bar + extra_callback: shouldn't be there objects: chair: - name: hello description: world namespace: null + extra_chair: shouldn't be there role: - name: my_role description: This is my role namespace: null + extra_role: shouldn't be there +extra_global: shouldn't be there \ No newline at end of file diff --git a/tests/functional/test_changelog_yaml_linter.py b/tests/functional/test_changelog_yaml_linter.py index d02dc33a..100e4c84 100644 --- a/tests/functional/test_changelog_yaml_linter.py +++ b/tests/functional/test_changelog_yaml_linter.py @@ -46,9 +46,12 @@ def load_tests(): class Args: - def __init__(self, changelog_yaml_path=None, no_semantic_versioning=False): + def __init__( + self, changelog_yaml_path=None, no_semantic_versioning=False, strict=False + ): self.changelog_yaml_path = changelog_yaml_path self.no_semantic_versioning = no_semantic_versioning + self.strict = strict # Test good files @@ -67,18 +70,32 @@ def test_good_changelog_yaml_files(yaml_filename): assert stdout_lines == [] assert rc == C.RC_SUCCESS + # Run test against CLI (strict checking) + args = Args(changelog_yaml_path=yaml_filename, strict=True) + stdout = io.StringIO() + with redirect_stdout(stdout): + rc = command_lint_changelog_yaml(args) + stdout_lines = stdout.getvalue().splitlines() + assert stdout_lines == [] + assert rc == C.RC_SUCCESS + @pytest.mark.parametrize("yaml_filename, json_filename", BAD_TESTS) def test_bad_changelog_yaml_files(yaml_filename, json_filename): # Run test directly against implementation errors = lint_changelog_yaml(yaml_filename) - assert len(errors) > 0 + errors_strict = lint_changelog_yaml(yaml_filename, strict=True) + assert len(errors_strict) > 0 # Cut off path errors = [ [error[1], error[2], error[3].replace(yaml_filename, "input.yaml")] for error in errors ] + errors_strict = [ + [error[1], error[2], error[3].replace(yaml_filename, "input.yaml")] + for error in errors_strict + ] # Load expected errors with open(json_filename, "r") as json_f: @@ -87,11 +104,19 @@ def test_bad_changelog_yaml_files(yaml_filename, json_filename): if errors != data["errors"]: print(json.dumps(errors, indent=2)) + if errors_strict != data["errors_strict"]: + print(json.dumps(errors_strict, indent=2)) + assert len(errors) == len(data["errors"]) for error1, error2 in zip(errors, data["errors"]): assert error1[0:2] == error2[0:2] assert re.match(error2[2], error1[2], flags=re.DOTALL) is not None + assert len(errors_strict) == len(data["errors_strict"]) + for error1, error2 in zip(errors_strict, data["errors_strict"]): + assert error1[0:2] == error2[0:2] + assert re.match(error2[2], error1[2], flags=re.DOTALL) is not None + # Run test against CLI args = Args(changelog_yaml_path=yaml_filename) stdout = io.StringIO()