From 9851e01f50b0657f01edad4461c69aadb5cbdc0f Mon Sep 17 00:00:00 2001 From: Felix Fontein Date: Mon, 26 Aug 2024 23:21:47 +0200 Subject: [PATCH 01/14] Add collection-meta.yaml linter. --- .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 | 86 ++++++--- src/antsibull/collection_meta_lint.py | 180 ++++++++++++++++++ src/antsibull/tagging.py | 4 +- 8 files changed, 286 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..e7cae772 100644 --- a/src/antsibull/collection_meta.py +++ b/src/antsibull/collection_meta.py @@ -13,50 +13,84 @@ 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 -class CollectionMetadata: +def _convert_pypi_version(v: t.Any) -> t.Any: + if not isinstance(v, str): + raise ValueError(f"must be a string, got {v!r}") + if not v: + raise ValueError(f"must be a non-trivial string, got {v!r}") + version = PypiVer(v) + 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) + + version: t.Union[int, t.Literal["TBD"]] + reason: t.Literal["deprecated", "considered-unmaintained", "renamed"] + announce_version: t.Optional[PydanticPypiVersion] = None + new_name: t.Optional[str] = None + discussion: t.Optional[p.HttpUrl] = None + redirect_replacement_version: t.Optional[int] = None + + +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: str | None): + 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..c9a898e4 --- /dev/null +++ b/src/antsibull/collection_meta_lint.py @@ -0,0 +1,180 @@ +# 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_reason_renamed( + self, collection: str, removal: RemovalInformation, prefix: str + ) -> None: + if removal.new_name is None: + self.errors.append( + f"{prefix} -> new_name: Must be provided if reason is renamed" + ) + elif removal.new_name == collection: + self.errors.append( + f"{prefix} -> new_name: Must not be the collection's name" + ) + + def _validate_reason_not_renamed( + self, + collection: str, # pylint: disable=unused-argument + removal: RemovalInformation, + prefix: str, + ) -> None: + if removal.new_name is not None: + self.errors.append( + f"{prefix} -> new_name: Must not be provided if reason is not renamed" + ) + if removal.redirect_replacement_version is not None: + self.errors.append( + f"{prefix} -> redirect_replacement_version: Must not be" + " provided if reason is not renamed" + ) + if removal.version == "TBD": + self.errors.append( + f"{prefix} -> version: Must not be TBD if reason is not renamed" + ) + + def _validate_removal( + self, collection: str, removal: RemovalInformation, prefix: str + ) -> None: + if removal.version != "TBD" and removal.version <= self.major_release: + self.errors.append( + f"{prefix} version: Removal version {removal.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 be the current major version {self.major_release}" + ) + + if removal.redirect_replacement_version is not None: + if removal.redirect_replacement_version <= self.major_release: + self.errors.append( + f"{prefix} redirect_replacement_version: Redirect removal version" + f" {removal.redirect_replacement_version} must be larger than" + f" current major version {self.major_release}" + ) + if ( + removal.version != "TBD" + and removal.redirect_replacement_version >= removal.version + ): + self.errors.append( + f"{prefix} redirect_replacement_version: Redirect removal version" + f" {removal.redirect_replacement_version} must be smaller than" + f" the removal major version {removal.version}" + ) + + if removal.reason == "renamed": + self._validate_reason_renamed(collection, removal, prefix) + else: + self._validate_reason_not_renamed(collection, removal, prefix) + + 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): + print(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) ) From a75bb2554465ed68bd1fbf86107c55055bce6aab Mon Sep 17 00:00:00 2001 From: Felix Fontein Date: Tue, 27 Aug 2024 09:46:02 +0200 Subject: [PATCH 02/14] Allow reason=other. --- src/antsibull/collection_meta.py | 3 ++- src/antsibull/collection_meta_lint.py | 27 +++++++++++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/src/antsibull/collection_meta.py b/src/antsibull/collection_meta.py index e7cae772..0bfda999 100644 --- a/src/antsibull/collection_meta.py +++ b/src/antsibull/collection_meta.py @@ -45,7 +45,8 @@ class RemovalInformation(p.BaseModel): model_config = p.ConfigDict(extra="ignore", arbitrary_types_allowed=True) version: t.Union[int, t.Literal["TBD"]] - reason: t.Literal["deprecated", "considered-unmaintained", "renamed"] + reason: t.Literal["deprecated", "considered-unmaintained", "renamed", "other"] + reason_text: str announce_version: t.Optional[PydanticPypiVersion] = None new_name: t.Optional[str] = None discussion: t.Optional[p.HttpUrl] = None diff --git a/src/antsibull/collection_meta_lint.py b/src/antsibull/collection_meta_lint.py index c9a898e4..55bce7bc 100644 --- a/src/antsibull/collection_meta_lint.py +++ b/src/antsibull/collection_meta_lint.py @@ -58,6 +58,28 @@ def _validate_reason_not_renamed( f"{prefix} -> version: Must not be TBD if reason is not renamed" ) + def _validate_reason_other( + self, + collection: str, # pylint: disable=unused-argument + removal: RemovalInformation, + prefix: str, + ) -> None: + if removal.reason_text is None: + self.errors.append( + f"{prefix} -> reason_text: Must be provided if reason is other" + ) + + def _validate_reason_not_other( + self, + collection: str, # pylint: disable=unused-argument + removal: RemovalInformation, + prefix: str, + ) -> None: + if removal.reason_text is not None: + self.errors.append( + f"{prefix} -> reason_text: Must not be provided if reason is not other" + ) + def _validate_removal( self, collection: str, removal: RemovalInformation, prefix: str ) -> None: @@ -98,6 +120,11 @@ def _validate_removal( else: self._validate_reason_not_renamed(collection, removal, prefix) + if removal.reason == "other": + self._validate_reason_other(collection, removal, prefix) + else: + self._validate_reason_not_other(collection, removal, prefix) + def _validate_collection( self, collection: str, meta: CollectionMetadata, prefix: str ) -> None: From d4c5c81f8393456e96c56af08b8d59947527b614 Mon Sep 17 00:00:00 2001 From: Felix Fontein Date: Thu, 29 Aug 2024 07:10:42 +0200 Subject: [PATCH 03/14] Use StrPath; fix collection missing error reporting. --- src/antsibull/collection_meta.py | 5 ++++- src/antsibull/collection_meta_lint.py | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/antsibull/collection_meta.py b/src/antsibull/collection_meta.py index 0bfda999..84b10cd0 100644 --- a/src/antsibull/collection_meta.py +++ b/src/antsibull/collection_meta.py @@ -20,6 +20,9 @@ from pydantic.functional_validators import BeforeValidator from typing_extensions import Annotated +if t.TYPE_CHECKING: + from _typeshed import StrPath + def _convert_pypi_version(v: t.Any) -> t.Any: if not isinstance(v, str): @@ -80,7 +83,7 @@ class CollectionsMetadata(p.BaseModel): collections: dict[str, CollectionMetadata] @staticmethod - def load_from(deps_dir: str | None): + def load_from(deps_dir: StrPath | None): if deps_dir is None: return CollectionsMetadata(collections={}) collection_meta_path = os.path.join(deps_dir, "collection-meta.yaml") diff --git a/src/antsibull/collection_meta_lint.py b/src/antsibull/collection_meta_lint.py index 55bce7bc..9f194456 100644 --- a/src/antsibull/collection_meta_lint.py +++ b/src/antsibull/collection_meta_lint.py @@ -162,7 +162,7 @@ def validate(self, data: CollectionsMetadata) -> None: # Complain about remaining collections for collection in sorted(remaining_collections): - print(f"collections: No metadata present for {collection}") + self.errors.append(f"collections: No metadata present for {collection}") def lint_collection_meta() -> int: From 735575f47e23d53e39c32e93952b473e35409eee Mon Sep 17 00:00:00 2001 From: Felix Fontein Date: Thu, 29 Aug 2024 07:32:38 +0200 Subject: [PATCH 04/14] Replace some manual validation with model validators. --- src/antsibull/collection_meta.py | 39 +++++++++++++++- src/antsibull/collection_meta_lint.py | 67 ++------------------------- 2 files changed, 42 insertions(+), 64 deletions(-) diff --git a/src/antsibull/collection_meta.py b/src/antsibull/collection_meta.py index 84b10cd0..e260665e 100644 --- a/src/antsibull/collection_meta.py +++ b/src/antsibull/collection_meta.py @@ -49,12 +49,49 @@ class RemovalInformation(p.BaseModel): version: t.Union[int, t.Literal["TBD"]] reason: t.Literal["deprecated", "considered-unmaintained", "renamed", "other"] - reason_text: str + 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_version: t.Optional[int] = None + @p.model_validator(mode="after") + def check_reason_text(self) -> t.Self: + if self.reason == "other": + if self.reason_text is None: + raise ValueError("reason_text must be provided if reason is other") + else: + if self.reason_text is not None: + raise ValueError( + "reason_text must not be provided if reason is not other" + ) + return self + + @p.model_validator(mode="after") + def check_reason_is_renamed(self) -> t.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_version is not None and self.version != "TBD": + if self.redirect_replacement_version >= self.version: + raise ValueError("redirect_replacement_version must be smaller than version") + return self + + @p.model_validator(mode="after") + def check_reason_is_not_renamed(self) -> t.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_version is not None: + raise ValueError( + "redirect_replacement_version must not be provided if reason is not renamed" + ) + if self.version == "TBD": + raise ValueError("version must not be TBD if reason is not renamed") + return self + class CollectionMetadata(p.BaseModel): """ diff --git a/src/antsibull/collection_meta_lint.py b/src/antsibull/collection_meta_lint.py index 9f194456..fd5e8e7e 100644 --- a/src/antsibull/collection_meta_lint.py +++ b/src/antsibull/collection_meta_lint.py @@ -26,60 +26,6 @@ def __init__(self, *, all_collections: list[str], major_release: int): self.all_collections = all_collections self.major_release = major_release - def _validate_reason_renamed( - self, collection: str, removal: RemovalInformation, prefix: str - ) -> None: - if removal.new_name is None: - self.errors.append( - f"{prefix} -> new_name: Must be provided if reason is renamed" - ) - elif removal.new_name == collection: - self.errors.append( - f"{prefix} -> new_name: Must not be the collection's name" - ) - - def _validate_reason_not_renamed( - self, - collection: str, # pylint: disable=unused-argument - removal: RemovalInformation, - prefix: str, - ) -> None: - if removal.new_name is not None: - self.errors.append( - f"{prefix} -> new_name: Must not be provided if reason is not renamed" - ) - if removal.redirect_replacement_version is not None: - self.errors.append( - f"{prefix} -> redirect_replacement_version: Must not be" - " provided if reason is not renamed" - ) - if removal.version == "TBD": - self.errors.append( - f"{prefix} -> version: Must not be TBD if reason is not renamed" - ) - - def _validate_reason_other( - self, - collection: str, # pylint: disable=unused-argument - removal: RemovalInformation, - prefix: str, - ) -> None: - if removal.reason_text is None: - self.errors.append( - f"{prefix} -> reason_text: Must be provided if reason is other" - ) - - def _validate_reason_not_other( - self, - collection: str, # pylint: disable=unused-argument - removal: RemovalInformation, - prefix: str, - ) -> None: - if removal.reason_text is not None: - self.errors.append( - f"{prefix} -> reason_text: Must not be provided if reason is not other" - ) - def _validate_removal( self, collection: str, removal: RemovalInformation, prefix: str ) -> None: @@ -115,15 +61,10 @@ def _validate_removal( f" the removal major version {removal.version}" ) - if removal.reason == "renamed": - self._validate_reason_renamed(collection, removal, prefix) - else: - self._validate_reason_not_renamed(collection, removal, prefix) - - if removal.reason == "other": - self._validate_reason_other(collection, removal, prefix) - else: - self._validate_reason_not_other(collection, removal, prefix) + 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 From 1af2e184cfa6e336280eccd619a880cb37481347 Mon Sep 17 00:00:00 2001 From: Felix Fontein Date: Thu, 29 Aug 2024 07:37:00 +0200 Subject: [PATCH 05/14] Forgot to reformat. --- src/antsibull/collection_meta.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/antsibull/collection_meta.py b/src/antsibull/collection_meta.py index e260665e..d12c1fb7 100644 --- a/src/antsibull/collection_meta.py +++ b/src/antsibull/collection_meta.py @@ -75,7 +75,9 @@ def check_reason_is_renamed(self) -> t.Self: raise ValueError("new_name must be provided if reason is renamed") if self.redirect_replacement_version is not None and self.version != "TBD": if self.redirect_replacement_version >= self.version: - raise ValueError("redirect_replacement_version must be smaller than version") + raise ValueError( + "redirect_replacement_version must be smaller than version" + ) return self @p.model_validator(mode="after") From 242c540a4a9c53dc243083ec5549fa5b9fc199d2 Mon Sep 17 00:00:00 2001 From: Felix Fontein Date: Sat, 7 Sep 2024 19:56:11 +0000 Subject: [PATCH 06/14] Fix message. Co-authored-by: Maxwell G --- src/antsibull/collection_meta_lint.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/antsibull/collection_meta_lint.py b/src/antsibull/collection_meta_lint.py index fd5e8e7e..1b840056 100644 --- a/src/antsibull/collection_meta_lint.py +++ b/src/antsibull/collection_meta_lint.py @@ -63,7 +63,7 @@ def _validate_removal( if removal.reason == "renamed" and removal.new_name == collection: self.errors.append( - f"{prefix} -> new_name: Must not be the collection's name" + f"{prefix} new_name: Must not be the collection's name" ) def _validate_collection( From 98782023ba1c7f4ec33241dc45c323be3c53fbee Mon Sep 17 00:00:00 2001 From: Felix Fontein Date: Sat, 7 Sep 2024 22:02:37 +0200 Subject: [PATCH 07/14] Improve typing and validator naming, fix lint condition. --- src/antsibull/collection_meta.py | 10 +++++----- src/antsibull/collection_meta_lint.py | 8 +++----- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/src/antsibull/collection_meta.py b/src/antsibull/collection_meta.py index d12c1fb7..5fbed268 100644 --- a/src/antsibull/collection_meta.py +++ b/src/antsibull/collection_meta.py @@ -18,7 +18,7 @@ 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 +from typing_extensions import Annotated, Self if t.TYPE_CHECKING: from _typeshed import StrPath @@ -56,7 +56,7 @@ class RemovalInformation(p.BaseModel): redirect_replacement_version: t.Optional[int] = None @p.model_validator(mode="after") - def check_reason_text(self) -> t.Self: + def _check_reason_text(self) -> Self: if self.reason == "other": if self.reason_text is None: raise ValueError("reason_text must be provided if reason is other") @@ -68,7 +68,7 @@ def check_reason_text(self) -> t.Self: return self @p.model_validator(mode="after") - def check_reason_is_renamed(self) -> t.Self: + def _check_reason_is_renamed(self) -> Self: if self.reason != "renamed": return self if self.new_name is None: @@ -81,7 +81,7 @@ def check_reason_is_renamed(self) -> t.Self: return self @p.model_validator(mode="after") - def check_reason_is_not_renamed(self) -> t.Self: + def _check_reason_is_not_renamed(self) -> Self: if self.reason == "renamed": return self if self.new_name is not None: @@ -122,7 +122,7 @@ class CollectionsMetadata(p.BaseModel): collections: dict[str, CollectionMetadata] @staticmethod - def load_from(deps_dir: StrPath | None): + 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") diff --git a/src/antsibull/collection_meta_lint.py b/src/antsibull/collection_meta_lint.py index 1b840056..522eba43 100644 --- a/src/antsibull/collection_meta_lint.py +++ b/src/antsibull/collection_meta_lint.py @@ -37,11 +37,11 @@ def _validate_removal( if ( removal.announce_version is not None - and removal.announce_version.major != self.major_release + and removal.announce_version.major > self.major_release ): self.errors.append( f"{prefix} announce_version: Major version of {removal.announce_version}" - f" must be the current major version {self.major_release}" + f" must not be larger than the current major version {self.major_release}" ) if removal.redirect_replacement_version is not None: @@ -62,9 +62,7 @@ def _validate_removal( ) if removal.reason == "renamed" and removal.new_name == collection: - self.errors.append( - f"{prefix} new_name: Must not be the collection's name" - ) + self.errors.append(f"{prefix} new_name: Must not be the collection's name") def _validate_collection( self, collection: str, meta: CollectionMetadata, prefix: str From 1a18d511d4ba6a18caf53e184788f11428b290f1 Mon Sep 17 00:00:00 2001 From: Felix Fontein Date: Sat, 7 Sep 2024 22:08:55 +0200 Subject: [PATCH 08/14] Remove nested if. --- src/antsibull/collection_meta.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/antsibull/collection_meta.py b/src/antsibull/collection_meta.py index 5fbed268..54e1d080 100644 --- a/src/antsibull/collection_meta.py +++ b/src/antsibull/collection_meta.py @@ -73,11 +73,14 @@ def _check_reason_is_renamed(self) -> Self: return self if self.new_name is None: raise ValueError("new_name must be provided if reason is renamed") - if self.redirect_replacement_version is not None and self.version != "TBD": - if self.redirect_replacement_version >= self.version: - raise ValueError( - "redirect_replacement_version must be smaller than version" - ) + if ( + self.redirect_replacement_version is not None + and self.version != "TBD" + and self.redirect_replacement_version >= self.version + ): + raise ValueError( + "redirect_replacement_version must be smaller than version" + ) return self @p.model_validator(mode="after") From b1a0f540f34e028bac3ffa8eaf2d6b793c40e6e2 Mon Sep 17 00:00:00 2001 From: Felix Fontein Date: Sun, 8 Sep 2024 21:22:13 +0200 Subject: [PATCH 09/14] Improve PyPI version validation. Co-authored-by: Maxwell G --- src/antsibull/collection_meta.py | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/antsibull/collection_meta.py b/src/antsibull/collection_meta.py index 54e1d080..2517bf97 100644 --- a/src/antsibull/collection_meta.py +++ b/src/antsibull/collection_meta.py @@ -25,11 +25,15 @@ def _convert_pypi_version(v: t.Any) -> t.Any: - if not isinstance(v, str): - raise ValueError(f"must be a string, got {v!r}") - if not v: - raise ValueError(f"must be a non-trivial string, got {v!r}") - version = PypiVer(v) + 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}" From 91d4855d0055b29cebab8bb0ba5dfd6fd29334d2 Mon Sep 17 00:00:00 2001 From: Felix Fontein Date: Sun, 8 Sep 2024 21:23:05 +0200 Subject: [PATCH 10/14] Quote value. Co-authored-by: Maxwell G --- src/antsibull/collection_meta.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/antsibull/collection_meta.py b/src/antsibull/collection_meta.py index 2517bf97..884b13ee 100644 --- a/src/antsibull/collection_meta.py +++ b/src/antsibull/collection_meta.py @@ -63,11 +63,11 @@ class RemovalInformation(p.BaseModel): def _check_reason_text(self) -> Self: if self.reason == "other": if self.reason_text is None: - raise ValueError("reason_text must be provided if reason is other") + raise ValueError("reason_text must be provided if reason is 'other'") else: if self.reason_text is not None: raise ValueError( - "reason_text must not be provided if reason is not other" + "reason_text must not be provided if reason is not 'other'" ) return self @@ -76,7 +76,7 @@ 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") + raise ValueError("new_name must be provided if reason is 'renamed'") if ( self.redirect_replacement_version is not None and self.version != "TBD" @@ -92,13 +92,13 @@ 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") + raise ValueError("new_name must not be provided if reason is not 'renamed'") if self.redirect_replacement_version is not None: raise ValueError( - "redirect_replacement_version must not be provided if reason is not renamed" + "redirect_replacement_version must not be provided if reason is not 'renamed'" ) if self.version == "TBD": - raise ValueError("version must not be TBD if reason is not renamed") + raise ValueError("version must not be TBD if reason is not 'renamed'") return self From 16fcfd99b991a53aabf1faca8e911ec49b7daa40 Mon Sep 17 00:00:00 2001 From: Felix Fontein Date: Sun, 8 Sep 2024 21:27:34 +0200 Subject: [PATCH 11/14] Add major_ prefix for major version field names. --- src/antsibull/collection_meta.py | 42 ++++++++++++--------------- src/antsibull/collection_meta_lint.py | 25 +++++++++------- 2 files changed, 33 insertions(+), 34 deletions(-) diff --git a/src/antsibull/collection_meta.py b/src/antsibull/collection_meta.py index 884b13ee..34cd1641 100644 --- a/src/antsibull/collection_meta.py +++ b/src/antsibull/collection_meta.py @@ -25,15 +25,11 @@ 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 not isinstance(v, str): + raise ValueError(f"must be a string, got {v!r}") + if not v: + raise ValueError(f"must be a non-trivial string, got {v!r}") + version = PypiVer(v) 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}" @@ -51,23 +47,23 @@ class RemovalInformation(p.BaseModel): model_config = p.ConfigDict(extra="ignore", arbitrary_types_allowed=True) - version: t.Union[int, t.Literal["TBD"]] + major_version: t.Union[int, t.Literal["TBD"]] reason: t.Literal["deprecated", "considered-unmaintained", "renamed", "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_version: t.Optional[int] = None + redirect_replacement_major_version: t.Optional[int] = None @p.model_validator(mode="after") def _check_reason_text(self) -> Self: if self.reason == "other": if self.reason_text is None: - raise ValueError("reason_text must be provided if reason is 'other'") + raise ValueError("reason_text must be provided if reason is other") else: if self.reason_text is not None: raise ValueError( - "reason_text must not be provided if reason is not 'other'" + "reason_text must not be provided if reason is not other" ) return self @@ -76,14 +72,14 @@ 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'") + raise ValueError("new_name must be provided if reason is renamed") if ( - self.redirect_replacement_version is not None - and self.version != "TBD" - and self.redirect_replacement_version >= self.version + 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_version must be smaller than version" + "redirect_replacement_major_version must be smaller than major_version" ) return self @@ -92,13 +88,13 @@ 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_version 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_version must not be provided if reason is not 'renamed'" + "redirect_replacement_major_version must not be provided if reason is not renamed" ) - if self.version == "TBD": - raise ValueError("version must not be TBD 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 diff --git a/src/antsibull/collection_meta_lint.py b/src/antsibull/collection_meta_lint.py index 522eba43..10163d1f 100644 --- a/src/antsibull/collection_meta_lint.py +++ b/src/antsibull/collection_meta_lint.py @@ -29,9 +29,12 @@ def __init__(self, *, all_collections: list[str], major_release: int): def _validate_removal( self, collection: str, removal: RemovalInformation, prefix: str ) -> None: - if removal.version != "TBD" and removal.version <= self.major_release: + if ( + removal.major_version != "TBD" + and removal.major_version <= self.major_release + ): self.errors.append( - f"{prefix} version: Removal version {removal.version} must" + f"{prefix} major_version: Removal major version {removal.major_version} must" f" be larger than current major version {self.major_release}" ) @@ -44,21 +47,21 @@ def _validate_removal( f" must not be larger than the current major version {self.major_release}" ) - if removal.redirect_replacement_version is not None: - if removal.redirect_replacement_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_version: Redirect removal version" - f" {removal.redirect_replacement_version} must be larger than" + 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.version != "TBD" - and removal.redirect_replacement_version >= removal.version + removal.major_version != "TBD" + and removal.redirect_replacement_major_version >= removal.major_version ): self.errors.append( - f"{prefix} redirect_replacement_version: Redirect removal version" - f" {removal.redirect_replacement_version} must be smaller than" - f" the removal major version {removal.version}" + 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: From 25c92a6205ff460e7e278bef8b8cd715a35523cb Mon Sep 17 00:00:00 2001 From: Felix Fontein Date: Sun, 8 Sep 2024 21:31:45 +0200 Subject: [PATCH 12/14] Re-do PypiVer parsing. --- src/antsibull/collection_meta.py | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/antsibull/collection_meta.py b/src/antsibull/collection_meta.py index 34cd1641..063e446c 100644 --- a/src/antsibull/collection_meta.py +++ b/src/antsibull/collection_meta.py @@ -25,11 +25,15 @@ def _convert_pypi_version(v: t.Any) -> t.Any: - if not isinstance(v, str): - raise ValueError(f"must be a string, got {v!r}") - if not v: - raise ValueError(f"must be a non-trivial string, got {v!r}") - version = PypiVer(v) + 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}" From 15adfc2b0ee4f2edc46029d8e6d81be0af6acc54 Mon Sep 17 00:00:00 2001 From: Felix Fontein Date: Mon, 9 Sep 2024 23:39:05 +0200 Subject: [PATCH 13/14] Apparently I undid more things... --- src/antsibull/collection_meta.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/antsibull/collection_meta.py b/src/antsibull/collection_meta.py index 063e446c..8dc1d076 100644 --- a/src/antsibull/collection_meta.py +++ b/src/antsibull/collection_meta.py @@ -63,11 +63,11 @@ class RemovalInformation(p.BaseModel): def _check_reason_text(self) -> Self: if self.reason == "other": if self.reason_text is None: - raise ValueError("reason_text must be provided if reason is other") + raise ValueError("reason_text must be provided if reason is 'other'") else: if self.reason_text is not None: raise ValueError( - "reason_text must not be provided if reason is not other" + "reason_text must not be provided if reason is not 'other'" ) return self @@ -76,7 +76,7 @@ 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") + 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" @@ -92,13 +92,13 @@ 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") + 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" + "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") + raise ValueError("major_version must not be TBD if reason is not 'renamed'") return self From 2b6a5168fe49bcff98c45f06d075e309d24c3747 Mon Sep 17 00:00:00 2001 From: Felix Fontein Date: Mon, 9 Sep 2024 23:41:14 +0200 Subject: [PATCH 14/14] Add new reason 'guidelines-violation'. --- src/antsibull/collection_meta.py | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/src/antsibull/collection_meta.py b/src/antsibull/collection_meta.py index 8dc1d076..a56b6d6d 100644 --- a/src/antsibull/collection_meta.py +++ b/src/antsibull/collection_meta.py @@ -52,7 +52,13 @@ class RemovalInformation(p.BaseModel): 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", "other"] + 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 @@ -61,13 +67,16 @@ class RemovalInformation(p.BaseModel): @p.model_validator(mode="after") def _check_reason_text(self) -> Self: - if self.reason == "other": + reasons_with_text = ("other", "guidelines-violation") + if self.reason in reasons_with_text: if self.reason_text is None: - raise ValueError("reason_text must be provided if reason is 'other'") + 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( - "reason_text must not be provided if reason is not 'other'" + f"reason_text must not be provided if reason is not {reasons}" ) return self