Add --strict parameter to lint-changelog-yaml subcommand. (#182)

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)."

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
 

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,14 +141,19 @@ def integration(session: nox.Session):
                 "git",
                 "clone",
                 "https://github.com/ansible-collections/community.general",
-                "--branch=stable-4",
+                "--branch=stable-9",
                 "--depth=1",
                 external=True,
             )
     cov_run(
         "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)

src/antsibull_changelog/cli.py

@@ -180,14 +180,20 @@ 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(
         "changelog_yaml_path",
         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(

src/antsibull_changelog/lint.py

@@ -33,20 +33,23 @@ class ChangelogYamlLinter:
 
     errors: list[tuple[str, int, int, str]]
     path: str
+    strict: bool
 
     def __init__(
         self,
         path: str,
         *,
         no_semantic_versioning: bool = False,
         preprocess_data: Callable[[dict], None] | None = None,
+        strict: bool = False,
     ):
         self.errors = []
         self.path = path
         self.valid_plugin_types = set(get_documentable_plugins())
         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()

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'"]
     ]
 }

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"]
     ]
 }

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 <class 'dict'>, but got \\['lists', 'are not allowed'\\]"],
         [0, 0, "'releases' -> '3.0.0' is expected to be <class 'dict'>, 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 <class 'str'>, 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 <class 'str'>, but got None"],
+        [0, 0, "'releases' -> '1.0.0' -> 'modules' -> 5 -> 'namespace' is expected to be <class 'str'>, but got None"],
+        [0, 0, "'releases' -> '1.0.0' -> 'plugins' -> 'lookup' -> 0 -> 'description' is expected to be <class 'str'>, 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 <class 'list'>, but got 321"],
+        [0, 0, "'releases' -> '1.0.0' -> 'fragments' is expected to be null or <class 'list'>, but got {'files': 123}"],
+        [0, 0, "'releases' -> '1.1.0' -> 'release_date' is expected to be <class 'str'>, 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 <class 'dict'>, but got \\['lists', 'are not allowed'\\]"],
+        [0, 0, "'releases' -> '3.0.0' is expected to be <class 'dict'>, but got 42"]
     ]
 }

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"]
     ]
 }

tests/functional/bad/random-3.json

@@ -1,5 +1,8 @@
 {
     "errors": [
         [0, 0, "'releases' is expected to be <class 'dict'>, but got \\[\\]"]
+    ],
+    "errors_strict": [
+        [0, 0, "'releases' is expected to be <class 'dict'>, but got \\[\\]"]
     ]
 }

tests/functional/bad/random-4.json

@@ -8,5 +8,20 @@
         [0, 0, "'releases' -> '1.0.0' -> 'plugins' -> 'callback' -> 1 -> 'description' is expected to be <class 'str'>, 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 <class 'str'>, 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 <class 'dict'>, but got 42"],
+        [0, 0, "'releases' -> '1.0.0' -> 'plugins' -> 'callback' -> 1 -> 'name' is expected to be <class 'str'>, but got 123"],
+        [0, 0, "'releases' -> '1.0.0' -> 'plugins' -> 'callback' -> 1 -> 'description' is expected to be <class 'str'>, 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"]
     ]
 }

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

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()