Allow to track removed collections in collection-meta.yaml #173
Conversation
felixfontein
force-pushed
the
schema-removal
branch
from
18e9646
to
d5366c6
Compare
|
The updated data can be seen here: ansible-community/ansible-build-data#459 |
felixfontein
force-pushed
the
schema-removal
branch
from
d5366c6
to
fadd8d1
Compare
| Stores metadata on when and why a collection was removed. | ||
| """ | ||
|
|
||
| major_version: int |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Should be no longer necessary due to 4759371.
| @@ -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", | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Should be no longer necessary due to 4759371.
| def _validate_removal_for_removed( | ||
| self, collection: str, removal: RemovedRemovalInformation, prefix: str | ||
| ) -> None: | ||
| if removal.major_version != self.major_release: |
There was a problem hiding this comment.
Should major_version not be part of RemovedRemovalInformation? Having removed_collections[NAME].removal.major_version and removed_collection[NAME].removed_version seems duplicative.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
My idea was that one can simply move the metadata entry for the removed collection to the
removed_collectionslist with minimal modifications (theremoval_versionentry 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_versionwith a newversionentry that replaces theremoval_versionone level up? I originally wanted to use an unmodifiedRemovalInformationhere, that's why I addedremoval_versionone level up, but now that there's a different class it might be easier to do it this way.
That sounds reasonable.
There was a problem hiding this comment.
I implemented that in 4759371.
Co-authored-by: Maxwell G <[email protected]>
| and self.major_version != "TBD" | ||
| and self.redirect_replacement_major_version | ||
| >= self.major_version # pyre-ignore[58] | ||
| and self.redirect_replacement_major_version >= self.version.major | ||
| ): | ||
| raise ValueError( | ||
| "redirect_replacement_major_version must be smaller than major_version" |
There was a problem hiding this comment.
This text needs to be adjusted now that the major_version field is no longer there. Also, might be good to add a test case for this error.
|
@gotmax23 thanks for reviewing this! |
This allows to create all changelog fragments related to deprecated and removed collections automatically.