From 821fb6d056255ceb2cdd338366589f4dab6667b7 Mon Sep 17 00:00:00 2001 From: Felix Fontein Date: Tue, 10 Sep 2024 09:03:08 +0200 Subject: [PATCH] Add collection-meta.yaml linter (#617) * Add collection-meta.yaml linter. * Allow reason=other. * Use StrPath; fix collection missing error reporting. * Replace some manual validation with model validators. * Forgot to reformat. * Fix message. Co-authored-by: Maxwell G * Improve typing and validator naming, fix lint condition. * Remove nested if. * Improve PyPI version validation. Co-authored-by: Maxwell G * Quote value. Co-authored-by: Maxwell G * Add major_ prefix for major version field names. * Re-do PypiVer parsing. * Apparently I undid more things... * Add new reason 'guidelines-violation'. --------- Co-authored-by: Maxwell G --- .github/workflows/antsibull-build.yml | 4 +- .../fragments/617-collection-meta-linter.yml | 4 + pyproject.toml | 5 +- src/antsibull/changelog.py | 2 +- src/antsibull/cli/antsibull_build.py | 38 ++++- src/antsibull/collection_meta.py | 145 ++++++++++++++--- src/antsibull/collection_meta_lint.py | 149 ++++++++++++++++++ src/antsibull/tagging.py | 4 +- 8 files changed, 314 insertions(+), 37 deletions(-) create mode 100644 changelogs/fragments/617-collection-meta-linter.yml create mode 100644 src/antsibull/collection_meta_lint.py diff --git a/.github/workflows/antsibull-build.yml b/.github/workflows/antsibull-build.yml index 32364217..dbb99b89 100644 --- a/.github/workflows/antsibull-build.yml +++ b/.github/workflows/antsibull-build.yml @@ -29,14 +29,14 @@ jobs: options: '-e antsibull_ansible_version=8.99.0' python: '3.9' antsibull_changelog_ref: 0.24.0 - antsibull_core_ref: stable-2 + antsibull_core_ref: main antsibull_docutils_ref: 1.0.0 # isn't used by antsibull-changelog 0.24.0 antsibull_fileutils_ref: main - name: Ansible 9 options: '-e antsibull_ansible_version=9.99.0' python: '3.11' antsibull_changelog_ref: main - antsibull_core_ref: stable-2 + antsibull_core_ref: main antsibull_docutils_ref: main antsibull_fileutils_ref: main - name: Ansible 10 diff --git a/changelogs/fragments/617-collection-meta-linter.yml b/changelogs/fragments/617-collection-meta-linter.yml new file mode 100644 index 00000000..f53ba84d --- /dev/null +++ b/changelogs/fragments/617-collection-meta-linter.yml @@ -0,0 +1,4 @@ +minor_changes: + - "Add subcommand ``lint-collection-meta`` for linting ``collection-meta.yaml`` files in ``ansible-build-data`` (https://github.com/ansible-community/antsibull/pull/617)." +breaking_changes: + - "Antsibull now depends on antsibull-core >= 3.0.0 and pydantic >= 2.0.0 (https://github.com/ansible-community/antsibull/pull/617)." diff --git a/pyproject.toml b/pyproject.toml index 30a84e18..e011728c 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -27,7 +27,7 @@ classifiers = [ requires-python = ">=3.9" dependencies = [ "antsibull-changelog >= 0.24.0", - "antsibull-core >= 2.0.0, < 4.0.0", + "antsibull-core >= 3.0.0, < 4.0.0", "antsibull-fileutils >= 1.0.0, < 2.0.0", "asyncio-pool", "build", @@ -38,8 +38,7 @@ dependencies = [ "aiohttp >= 3.0.0", "twiggy", "pyyaml", - # We rely on deprecated features to maintain compat btw. pydantic v1 and v2 - "pydantic < 3", + "pydantic >= 2, < 3", # pydantic already pulls it in, but we use it for TypedDict "typing_extensions", ] diff --git a/src/antsibull/changelog.py b/src/antsibull/changelog.py index ce1debc3..b78f6270 100644 --- a/src/antsibull/changelog.py +++ b/src/antsibull/changelog.py @@ -506,7 +506,7 @@ def get_changelog( PypiVer(ansible_ancestor_version_str) if ansible_ancestor_version_str else None ) - collection_metadata = CollectionsMetadata(deps_dir) + collection_metadata = CollectionsMetadata.load_from(deps_dir) if deps_dir is not None: for path in glob.glob(os.path.join(deps_dir, "*.deps"), recursive=False): diff --git a/src/antsibull/cli/antsibull_build.py b/src/antsibull/cli/antsibull_build.py index b9323868..dd4047fe 100644 --- a/src/antsibull/cli/antsibull_build.py +++ b/src/antsibull/cli/antsibull_build.py @@ -54,6 +54,7 @@ rebuild_single_command, ) from ..build_changelog import build_changelog # noqa: E402 +from ..collection_meta_lint import lint_collection_meta # noqa: E402 from ..constants import MINIMUM_ANSIBLE_VERSION, SANITY_TESTS_DEFAULT # noqa: E402 from ..dep_closure import validate_dependencies_command # noqa: E402 from ..from_source import verify_upstream_command # noqa: E402 @@ -84,6 +85,7 @@ "sanity-tests": sanity_tests_command, "announcements": announcements_command, "send-announcements": send_announcements_command, + "lint-collection-meta": lint_collection_meta, } DISABLE_VERIFY_UPSTREAMS_IGNORES_SENTINEL = "NONE" DEFAULT_ANNOUNCEMENTS_DIR = Path("build/announce") @@ -107,7 +109,11 @@ def _normalize_build_options(args: argparse.Namespace) -> None: ): return - if args.ansible_version < MINIMUM_ANSIBLE_VERSION: + if ( + (args.ansible_version < MINIMUM_ANSIBLE_VERSION) + if args.command != "lint-collection-meta" + else (args.ansible_major_version < MINIMUM_ANSIBLE_VERSION.major) + ): raise InvalidArgumentError( f"Ansible < {MINIMUM_ANSIBLE_VERSION} is not supported" " by this antsibull version." @@ -136,8 +142,8 @@ def _normalize_build_write_data_options(args: argparse.Namespace) -> None: ) -def _normalize_new_release_options(args: argparse.Namespace) -> None: - if args.command != "new-ansible": +def _normalize_pieces_file_options(args: argparse.Namespace) -> None: + if args.command not in ("new-ansible", "lint-collection-meta"): return if args.pieces_file is None: @@ -151,6 +157,11 @@ def _normalize_new_release_options(args: argparse.Namespace) -> None: " per line" ) + +def _normalize_new_release_options(args: argparse.Namespace) -> None: + if args.command != "new-ansible": + return + compat_version_part = f"{args.ansible_version.major}" if args.build_file is None: @@ -769,6 +780,26 @@ def parse_args(program_name: str, args: list[str]) -> argparse.Namespace: dest="send_actions", ) + lint_collection_meta_parser = subparsers.add_parser( + "lint-collection-meta", + description="Lint the collection-meta.yaml file.", + ) + lint_collection_meta_parser.add_argument( + "ansible_major_version", + type=int, + help="The X major version of Ansible that this will be for", + ) + lint_collection_meta_parser.add_argument( + "--data-dir", default=".", help="Directory to read .build and .deps files from" + ) + lint_collection_meta_parser.add_argument( + "--pieces-file", + default=None, + help="File containing a list of collections to include. This is" + " considered to be relative to --data-dir. The default is" + f" {DEFAULT_PIECES_FILE}", + ) + # This must come after all parser setup if HAS_ARGCOMPLETE: argcomplete.autocomplete(parser) @@ -780,6 +811,7 @@ def parse_args(program_name: str, args: list[str]) -> argparse.Namespace: _normalize_commands(parsed_args) _normalize_build_options(parsed_args) _normalize_build_write_data_options(parsed_args) + _normalize_pieces_file_options(parsed_args) _normalize_new_release_options(parsed_args) _normalize_release_build_options(parsed_args) _normalize_validate_tags_options(parsed_args) diff --git a/src/antsibull/collection_meta.py b/src/antsibull/collection_meta.py index 72a5bdca..a56b6d6d 100644 --- a/src/antsibull/collection_meta.py +++ b/src/antsibull/collection_meta.py @@ -13,50 +13,143 @@ import os import typing as t -from collections.abc import Mapping +import pydantic as p from antsibull_fileutils.yaml import load_yaml_file +from packaging.version import Version as PypiVer +from pydantic.functional_validators import BeforeValidator +from typing_extensions import Annotated, Self +if t.TYPE_CHECKING: + from _typeshed import StrPath -class CollectionMetadata: + +def _convert_pypi_version(v: t.Any) -> t.Any: + if isinstance(v, str): + if not v: + raise ValueError(f"must be a non-trivial string, got {v!r}") + version = PypiVer(v) + elif isinstance(v, PypiVer): + version = v + else: + raise ValueError(f"must be a string or PypiVer object, got {v!r}") + + if len(version.release) != 3: + raise ValueError( + f"must be a version with three release numbers (e.g. 1.2.3, 2.3.4a1), got {v!r}" + ) + return version + + +PydanticPypiVersion = Annotated[PypiVer, BeforeValidator(_convert_pypi_version)] + + +class RemovalInformation(p.BaseModel): + """ + Stores metadata on when and why a collection will get removed. + """ + + model_config = p.ConfigDict(extra="ignore", arbitrary_types_allowed=True) + + major_version: t.Union[int, t.Literal["TBD"]] + reason: t.Literal[ + "deprecated", + "considered-unmaintained", + "renamed", + "guidelines-violation", + "other", + ] + reason_text: t.Optional[str] = None + announce_version: t.Optional[PydanticPypiVersion] = None + new_name: t.Optional[str] = None + discussion: t.Optional[p.HttpUrl] = None + redirect_replacement_major_version: t.Optional[int] = None + + @p.model_validator(mode="after") + def _check_reason_text(self) -> Self: + reasons_with_text = ("other", "guidelines-violation") + if self.reason in reasons_with_text: + if self.reason_text is None: + reasons = ", ".join(f"'{reason}'" for reason in reasons_with_text) + raise ValueError(f"reason_text must be provided if reason is {reasons}") + else: + if self.reason_text is not None: + reasons = ", ".join(f"'{reason}'" for reason in reasons_with_text) + raise ValueError( + f"reason_text must not be provided if reason is not {reasons}" + ) + return self + + @p.model_validator(mode="after") + def _check_reason_is_renamed(self) -> Self: + if self.reason != "renamed": + return self + if self.new_name is None: + raise ValueError("new_name must be provided if reason is 'renamed'") + if ( + self.redirect_replacement_major_version is not None + and self.major_version != "TBD" + and self.redirect_replacement_major_version >= self.major_version + ): + raise ValueError( + "redirect_replacement_major_version must be smaller than major_version" + ) + return self + + @p.model_validator(mode="after") + def _check_reason_is_not_renamed(self) -> Self: + if self.reason == "renamed": + return self + if self.new_name is not None: + raise ValueError("new_name must not be provided if reason is not 'renamed'") + if self.redirect_replacement_major_version is not None: + raise ValueError( + "redirect_replacement_major_version must not be provided if reason is not 'renamed'" + ) + if self.major_version == "TBD": + raise ValueError("major_version must not be TBD if reason is not 'renamed'") + return self + + +class CollectionMetadata(p.BaseModel): """ Stores metadata about one collection. """ - changelog_url: str | None - collection_directory: str | None - repository: str | None - tag_version_regex: str | None + model_config = p.ConfigDict(extra="ignore") - def __init__(self, source: Mapping[str, t.Any] | None = None): - if source is None: - source = {} - self.changelog_url = source.get("changelog-url") - self.collection_directory = source.get("collection-directory") - self.repository = source.get("repository") - self.tag_version_regex = source.get("tag_version_regex") + changelog_url: t.Optional[str] = p.Field(alias="changelog-url", default=None) + collection_directory: t.Optional[str] = p.Field( + alias="collection-directory", default=None + ) + repository: t.Optional[str] = None + tag_version_regex: t.Optional[str] = None + maintainers: list[str] = [] + removal: t.Optional[RemovalInformation] = None -class CollectionsMetadata: +class CollectionsMetadata(p.BaseModel): """ Stores metadata about a set of collections. """ - data: dict[str, CollectionMetadata] + model_config = p.ConfigDict(extra="ignore") + + collections: dict[str, CollectionMetadata] - def __init__(self, deps_dir: str | None): - self.data = {} - if deps_dir is not None: - collection_meta_path = os.path.join(deps_dir, "collection-meta.yaml") - if os.path.exists(collection_meta_path): - data = load_yaml_file(collection_meta_path) - if data and "collections" in data: - for collection_name, collection_data in data["collections"].items(): - self.data[collection_name] = CollectionMetadata(collection_data) + @staticmethod + def load_from(deps_dir: StrPath | None) -> CollectionsMetadata: + if deps_dir is None: + return CollectionsMetadata(collections={}) + collection_meta_path = os.path.join(deps_dir, "collection-meta.yaml") + if not os.path.exists(collection_meta_path): + return CollectionsMetadata(collections={}) + data = load_yaml_file(collection_meta_path) + return CollectionsMetadata.parse_obj(data) def get_meta(self, collection_name: str) -> CollectionMetadata: - result = self.data.get(collection_name) + result = self.collections.get(collection_name) if result is None: result = CollectionMetadata() - self.data[collection_name] = result + self.collections[collection_name] = result return result diff --git a/src/antsibull/collection_meta_lint.py b/src/antsibull/collection_meta_lint.py new file mode 100644 index 00000000..10163d1f --- /dev/null +++ b/src/antsibull/collection_meta_lint.py @@ -0,0 +1,149 @@ +# Author: Felix Fontein +# GNU General Public License v3.0+ (see LICENSES/GPL-3.0-or-later.txt or +# https://www.gnu.org/licenses/gpl-3.0.txt) +# SPDX-License-Identifier: GPL-3.0-or-later +# SPDX-FileCopyrightText: Ansible Project, 2024 + +""" +Classes to lint collection-meta.yaml +""" + +from __future__ import annotations + +import os + +import pydantic as p +from antsibull_core import app_context +from antsibull_core.dependency_files import parse_pieces_file +from antsibull_core.yaml import load_yaml_file + +from .collection_meta import CollectionMetadata, CollectionsMetadata, RemovalInformation + + +class _Validator: + def __init__(self, *, all_collections: list[str], major_release: int): + self.errors: list[str] = [] + self.all_collections = all_collections + self.major_release = major_release + + def _validate_removal( + self, collection: str, removal: RemovalInformation, prefix: str + ) -> None: + if ( + removal.major_version != "TBD" + and removal.major_version <= self.major_release + ): + self.errors.append( + f"{prefix} major_version: Removal major version {removal.major_version} must" + f" be larger than current major version {self.major_release}" + ) + + if ( + removal.announce_version is not None + and removal.announce_version.major > self.major_release + ): + self.errors.append( + f"{prefix} announce_version: Major version of {removal.announce_version}" + f" must not be larger than the current major version {self.major_release}" + ) + + if removal.redirect_replacement_major_version is not None: + if removal.redirect_replacement_major_version <= self.major_release: + self.errors.append( + f"{prefix} redirect_replacement_major_version: Redirect removal version" + f" {removal.redirect_replacement_major_version} must be larger than" + f" current major version {self.major_release}" + ) + if ( + removal.major_version != "TBD" + and removal.redirect_replacement_major_version >= removal.major_version + ): + self.errors.append( + f"{prefix} redirect_replacement_major_version: Redirect removal major version" + f" {removal.redirect_replacement_major_version} must be smaller than" + f" the removal major version {removal.major_version}" + ) + + if removal.reason == "renamed" and removal.new_name == collection: + self.errors.append(f"{prefix} new_name: Must not be the collection's name") + + def _validate_collection( + self, collection: str, meta: CollectionMetadata, prefix: str + ) -> None: + if meta.repository is None: + self.errors.append(f"{prefix} repository: Required field not provided") + + if meta.removal: + self._validate_removal(collection, meta.removal, f"{prefix} removal ->") + + def validate(self, data: CollectionsMetadata) -> None: + # Check order + sorted_list = sorted(data.collections) + raw_list = list(data.collections) + if raw_list != sorted_list: + for raw_entry, sorted_entry in zip(raw_list, sorted_list): + if raw_entry != sorted_entry: + self.errors.append( + "The collection list must be sorted; " + f"{sorted_entry!r} must come before {raw_entry}" + ) + break + + # Validate collection data + remaining_collections = set(self.all_collections) + for collection, meta in data.collections.items(): + if collection not in remaining_collections: + self.errors.append( + f"collections -> {collection}: Collection not in ansible.in" + ) + else: + remaining_collections.remove(collection) + self._validate_collection( + collection, meta, f"collections -> {collection} ->" + ) + + # Complain about remaining collections + for collection in sorted(remaining_collections): + self.errors.append(f"collections: No metadata present for {collection}") + + +def lint_collection_meta() -> int: + """Lint collection-meta.yaml.""" + app_ctx = app_context.app_ctx.get() + + major_release: int = app_ctx.extra["ansible_major_version"] + data_dir: str = app_ctx.extra["data_dir"] + pieces_file: str = app_ctx.extra["pieces_file"] + + validator = _Validator( + all_collections=parse_pieces_file(os.path.join(data_dir, pieces_file)), + major_release=major_release, + ) + + for cls in ( + # NOTE: The order is important here! The most deeply nested classes must come first, + # otherwise extra=forbid might not be used for something deeper in the hierarchy. + RemovalInformation, + CollectionMetadata, + CollectionsMetadata, + ): + cls.model_config["extra"] = "forbid" + cls.model_rebuild(force=True) + + collection_meta_path = os.path.join(data_dir, "collection-meta.yaml") + if not os.path.exists(collection_meta_path): + validator.errors.append(f"Cannot find {collection_meta_path}") + else: + data = load_yaml_file(collection_meta_path) + try: + parsed_data = CollectionsMetadata.parse_obj(data) + validator.validate(parsed_data) + except p.ValidationError as exc: + for error in exc.errors(): + location = " -> ".join(str(loc) for loc in error["loc"]) + validator.errors.append(f'{location}: {error["msg"]}') + + for message in validator.errors: + print(message) + + return 3 if validator.errors else 0 diff --git a/src/antsibull/tagging.py b/src/antsibull/tagging.py index f7b3de66..e3bb07a8 100644 --- a/src/antsibull/tagging.py +++ b/src/antsibull/tagging.py @@ -160,11 +160,11 @@ async def get_collections_tags( deps_filename = os.path.join(data_dir, deps_filename) deps_data = DepsFile(deps_filename).parse() - meta_data = CollectionsMetadata(data_dir) + meta_data = CollectionsMetadata.load_from(data_dir) async with asyncio_pool.AioPool(size=lib_ctx.thread_max) as pool: collection_tags = {} - for name, data in meta_data.data.items(): + for name, data in meta_data.collections.items(): collection_tags[name] = pool.spawn_n( _get_collection_tags(deps_data.deps[name], data, name) )