Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow to track removed collections in collection-meta.yaml #173

Merged
merged 6 commits into from
Sep 23, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions changelogs/fragments/173-schema-removal.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
minor_changes:
- "Allow information on removed collections in collection metadata schema (https://github.com/ansible-community/antsibull-core/pull/173)."
75 changes: 72 additions & 3 deletions src/antsibull_core/collection_meta.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
CollectionMetadata,
CollectionsMetadata,
RemovalInformation,
RemovedCollectionMetadata,
RemovedRemovalInformation,
)

if t.TYPE_CHECKING:
Expand All @@ -42,6 +44,12 @@ def __init__(self, *, all_collections: list[str], major_release: int):
self.all_collections = all_collections
self.major_release = major_release

def _validate_removal_base(
self, collection: str, removal: RemovalInformation, prefix: str
) -> None:
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_removal(
self, collection: str, removal: RemovalInformation, prefix: str
) -> None:
Expand All @@ -63,8 +71,7 @@ def _validate_removal(
f" must not be larger than the current major version {self.major_release}"
)

if removal.reason == "renamed" and removal.new_name == collection:
self.errors.append(f"{prefix} new_name: Must not be the collection's name")
self._validate_removal_base(collection, removal, prefix)

def _validate_collection(
self, collection: str, meta: CollectionMetadata, prefix: str
Expand All @@ -75,7 +82,42 @@ def _validate_collection(
if meta.removal:
self._validate_removal(collection, meta.removal, f"{prefix} removal ->")

def validate(self, data: CollectionsMetadata) -> None:
def _validate_removal_for_removed(
self, collection: str, removal: RemovedRemovalInformation, prefix: str
) -> None:
if removal.major_version != self.major_release:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should major_version not be part of RemovedRemovalInformation? Having removed_collections[NAME].removal.major_version and removed_collection[NAME].removed_version seems duplicative.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My idea was that one can simply move the metadata entry for the removed collection to the removed_collections list with minimal modifications (the removal_version entry needs to be added).

But since that modification is needed anyway, how about replacing major_version with a new version entry that replaces the removal_version one level up? I originally wanted to use an unmodified RemovalInformation here, that's why I added removal_version one level up, but now that there's a different class it might be easier to do it this way.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My idea was that one can simply move the metadata entry for the removed collection to the removed_collections list with minimal modifications (the removal_version entry needs to be added).

Yeah, I figured that as well, but then I wondered if that was worth the potential confusion of having the same information twice.

But since that modification is needed anyway, how about replacing major_version with a new version entry that replaces the removal_version one level up? I originally wanted to use an unmodified RemovalInformation here, that's why I added removal_version one level up, but now that there's a different class it might be easier to do it this way.

That sounds reasonable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I implemented that in 4759371.

self.errors.append(
f"{prefix} major_version: Removal major version {removal.major_version} must"
f" be 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 less than the current major version {self.major_release}"
)

self._validate_removal_base(collection, removal, prefix)

def _validate_removed_collection(
self, collection: str, meta: RemovedCollectionMetadata, prefix: str
) -> None:
if meta.repository is None:
self.errors.append(f"{prefix} repository: Required field not provided")

self._validate_removal_for_removed(
collection, meta.removal, f"{prefix} removal ->"
)
if meta.removed_version.major != self.major_release:
self.errors.append(
f"{prefix} removed_version: Major version of {meta.removed_version}"
f" must be the current major version {self.major_release}"
)

def _validate_collections(self, data: CollectionsMetadata) -> None:
# Check order
sorted_list = sorted(data.collections)
raw_list = list(data.collections)
Expand Down Expand Up @@ -105,6 +147,33 @@ def validate(self, data: CollectionsMetadata) -> None:
for collection in sorted(remaining_collections):
self.errors.append(f"collections: No metadata present for {collection}")

def _validate_removed_collections(self, data: CollectionsMetadata) -> None:
# Check order
sorted_list = sorted(data.removed_collections)
raw_list = list(data.removed_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 removed collection list must be sorted; "
f"{sorted_entry!r} must come before {raw_entry}"
)
break

# Validate removed collection data
for collection, removed_meta in data.removed_collections.items():
if collection in self.all_collections:
self.errors.append(
f"removed_collections -> {collection}: Collection in ansible.in"
)
self._validate_removed_collection(
collection, removed_meta, f"removed_collections -> {collection} ->"
)

def validate(self, data: CollectionsMetadata) -> None:
self._validate_collections(data)
self._validate_removed_collections(data)


def lint_collection_meta(
*, collection_meta_path: StrPath, major_release: int, all_collections: list[str]
Expand Down
63 changes: 62 additions & 1 deletion src/antsibull_core/schemas/collection_meta.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,10 @@ class RemovalInformation(p.BaseModel):

model_config = p.ConfigDict(arbitrary_types_allowed=True)

# The Ansible major version from which the collection will be removed.
major_version: t.Union[int, t.Literal["TBD"]]

# The reason because of which the collection will be removed.
reason: t.Literal[
"deprecated",
"considered-unmaintained",
Expand All @@ -60,9 +63,19 @@ class RemovalInformation(p.BaseModel):
"other",
]
reason_text: t.Optional[str] = None

# The Ansible version in which the announcement was made. This is needed
# for changelog generation.
announce_version: t.Optional[PydanticPypiVersion] = None

# In case reason=renamed, the new name of the collection.
new_name: t.Optional[str] = None

# The link to the discussion of the removal.
discussion: t.Optional[p.HttpUrl] = None

# In case reason=renamed, the major Ansible release in which the collection's
# contents have been replaced by deprecated redirects.
redirect_replacement_major_version: t.Optional[int] = None

@p.model_validator(mode="after") # pyre-ignore[56]
Expand Down Expand Up @@ -112,28 +125,76 @@ def _check_reason_is_not_renamed(self) -> Self:
return self


class CollectionMetadata(p.BaseModel):
class RemovedRemovalInformation(RemovalInformation):
"""
Stores metadata on when and why a collection was removed.
"""

major_version: int
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth adding a comment that major_version cannot be TBD here (because it was already removed) like it is in the base class. That tripped me up for a minute. Or we could remove it entirely (and have another PendingRemovalInformation sub-model) like I suggested in another comment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be no longer necessary due to 4759371.



class BaseCollectionMetadata(p.BaseModel):
"""
Stores metadata about one collection.
"""

# If the collection does not use changelogs/changelog.yaml, it can provide
# an URL where the collection's changelog can be found.
felixfontein marked this conversation as resolved.
Show resolved Hide resolved
changelog_url: t.Optional[str] = p.Field(alias="changelog-url", default=None)

# In case the collection is not located in the root of its repository, the
# subdirectory in which the collection appears.
collection_directory: t.Optional[str] = p.Field(
alias="collection-directory", default=None
)

# The collection's repository.
repository: t.Optional[str] = None

# A regular expression to match the collection's version from a tag in the repository.
tag_version_regex: t.Optional[str] = None

# A list of maintainers. These should be usernames for the repository's
# hosting environment.
maintainers: list[str] = []


class CollectionMetadata(BaseCollectionMetadata):
"""
Stores metadata about one collection.
"""

# Optional information that the collection will be removed from
# a future Ansible release.
removal: t.Optional[RemovalInformation] = None


class RemovedCollectionMetadata(BaseCollectionMetadata):
"""
Stores metadata about a removed collection.
"""

model_config = p.ConfigDict(arbitrary_types_allowed=True)

# Information why the collection has been removed
removal: RemovedRemovalInformation

# The exact version from which the collection has been removed.
# This is needed for changelog generation.
removed_version: PydanticPypiVersion


class CollectionsMetadata(p.BaseModel):
"""
Stores metadata about a set of collections.
"""

# Metadata on the collections included in Ansible.
collections: dict[str, CollectionMetadata]

# Metadata on the collections removed from this major version of Ansible.
removed_collections: dict[str, RemovedCollectionMetadata] = {}

@staticmethod
def load_from(deps_dir: StrPath | None) -> CollectionsMetadata:
if deps_dir is None:
Expand Down
35 changes: 35 additions & 0 deletions tests/functional/test_collection_meta.py
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,22 @@
reason_text: The collection wasn't cow friendly, so the Steering Committee decided to kick it out.
discussion: https://forum.ansible.com/...
announce_version: 9.3.0
removed_collections:
bad.baz2:
repository: https://github.com/ansible-collections/collection_template
removed_version: 10.2.1
removal:
major_version: 9
reason: renamed
new_name: bad.bar2
announce_version: 9.3.0
redirect_replacement_major_version: 7
bad.baz1:
removed_version: 9.1.0
removal:
major_version: 8
reason: deprecated
announce_version: 7.1.0
""",
[
"foo.bar",
Expand All @@ -153,6 +169,7 @@
],
[
"The collection list must be sorted; 'baz.bam' must come before foo.bar",
"The removed collection list must be sorted; 'bad.baz1' must come before bad.baz2",
"collections -> bad.bar1 -> removal -> announce_version: Major version of 10.1.0 must not be larger than the current major version 9",
"collections -> bad.bar1 -> removal -> major_version: Removal major version 7 must be larger than current major version 9",
"collections -> bad.bar1: Collection not in ansible.in",
Expand All @@ -162,6 +179,10 @@
"collections -> baz.bam: Collection not in ansible.in",
"collections -> foo.bar -> repository: Required field not provided",
"collections: No metadata present for not.there",
"removed_collections -> bad.baz1 -> removal -> major_version: Removal major version 8 must be current major version 9",
"removed_collections -> bad.baz1 -> repository: Required field not provided",
"removed_collections -> bad.baz2 -> removal -> announce_version: Major version of 9.3.0 must be less than the current major version 9",
"removed_collections -> bad.baz2 -> removed_version: Major version of 10.2.1 must be the current major version 9",
],
),
(
Expand Down Expand Up @@ -271,6 +292,18 @@
removal:
major_version: 11
reason: foo
removed_collections:
bad.foo1:
repository: https://github.com/ansible-collections/collection_template
removed_version: 1.0.0
removal:
major_version: TBD
reason: deprecated
bad.foo2:
repository: https://github.com/ansible-collections/collection_template
removal:
major_version: 10
reason: deprecated
extra_stuff: baz
""",
[],
Expand All @@ -292,6 +325,8 @@
"collections -> bad.foo8 -> removal: Value error, new_name must not be provided if reason is not 'renamed'",
"collections -> bad.foo9 -> removal: Value error, redirect_replacement_major_version must not be provided if reason is not 'renamed'",
"extra_stuff: Extra inputs are not permitted",
"removed_collections -> bad.foo1 -> removal -> major_version: Input should be a valid integer, unable to parse string as an integer",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could add a pre validator that specifically checks for TBD and gives a more helpful message in that case, but that might be overengineering.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be no longer necessary due to 4759371.

"removed_collections -> bad.foo2 -> removed_version: Field required",
],
),
]
Expand Down