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

Add metadata to indicate which collections are unmaintained / deprecated / will be removed #450

Merged

Conversation

felixfontein
Copy link
Contributor

@felixfontein felixfontein commented Aug 26, 2024

This is necessary to implement ansible-community/antsibull-docs#93.

Basically there are four ways in this PR that mark a collection for removal. I've added three further possibilities below:

  1. It is considered unmaintained:
    removal:
      version: 11
      reason: considered-unmaintained
      discussion: https://forum.ansible.com/...
      announce_version: 9.3.0
  2. It has officially been deprecated:
    removal:
      version: 11
      reason: deprecated
      discussion: https://forum.ansible.com/...
      announce_version: 9.3.0
  3. It has been renamed (new name provided) and the removal version is known:
    removal:
      version: 10
      reason: renamed
      new_name: namespace.name
      announce_version: 9.3.0
  4. It has been renamed (new name provided) and the removal version is not yet known:
    removal:
      version: TBD
      reason: renamed
      new_name: namespace.name
      announce_version: 9.3.0
  5. (Not yet in use.) It is broken and no longer works with modern versions of Ansible:
    removal:
      version: 11
      reason: broken
      discussion: https://forum.ansible.com/...
      announce_version: 9.3.0
  6. (Not yet in use.) Removed for violations of the inclusion rules:
    removal:
      version: 11
      reason: inclusion-rules-violation
      discussion: https://forum.ansible.com/...
      announce_version: 9.3.0
  7. (Not yet in use.) Other:
    removal:
      version: 11
      reason: other
      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

What do you think? That should cover all cases in https://docs.ansible.com/ansible/devel/community/collection_contributors/collection_package_removal.html (if I didn't forgot about something), and there's the free-form version (reason: other / reason_text: ...) for other cases.

@felixfontein
Copy link
Contributor Author

I created a linter for collection-meta.yaml: ansible-community/antsibull#617

Copy link
Collaborator

@mariolenz mariolenz left a comment

Choose a reason for hiding this comment

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

Do we really want a removal version TBD? This doesn't look really helpful.

This might be OK for now, but I think we shouldn't have this in a release. Do you think we can give an actual version in time for Ansible 9.10.0 / 10.4.0, that is within the next two weeks?

edit: ibm.spectrum_virtualize deprecated / renamed

10/collection-meta.yaml Outdated Show resolved Hide resolved
9/collection-meta.yaml Outdated Show resolved Hide resolved
@felixfontein
Copy link
Contributor Author

Do we really want a removal version TBD? This doesn't look really helpful.

I disagree for renamed collections: there it simply says "it's renamed, but we haven't yet decided when to remove the redirects". Which IMO is totally fine, as long as we announce removal of the redirects in time.

This might be OK for now, but I think we shouldn't have this in a release. Do you think we can give an actual version in time for Ansible 9.10.0 / 10.4.0, that is within the next two weeks?

Right now the old collection is / will be replaced by a set of redirects, which avoid breakage for users. So let's keep these for some time.

We should definitely decide on how to handle renames, i.e. for how long to keep redirects, but I don't think we need to do this in the next two weeks so just that we can avoid a TBD.

@mariolenz
Copy link
Collaborator

I disagree for renamed collections: there it simply says "it's renamed, but we haven't yet decided when to remove the redirects". Which IMO is totally fine, as long as we announce removal of the redirects in time.

I think we should give a definite version when we remove a collection instead of just TBD. I'm not totally against TBD, though. That's why I only commented and didn't request a change ;-)

Copy link
Collaborator

@mariolenz mariolenz left a comment

Choose a reason for hiding this comment

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

Since nobody else left a comment, I guess people are fine with version: TBD.

So I've reconsidered and say it's OK, too... after all it's it' better than nothing 🤷

@felixfontein
Copy link
Contributor Author

We can always sitll disallow TBD later. If https://forum.ansible.com/t/8150 or something similar is accepted there should no longer be a need for TBD anyway.

@mariolenz
Copy link
Collaborator

Sorry, I think I've lost track and am a little bit confused about this PR. I've changed my opinion on the TBD issue, but I don't understand what should happen with this PR now:

  • Are you waiting on more approvals / comments?
  • Are you waiting on a discussion / vote to finish?
  • Are you waiting on any PRs in antsibull / antsibull-docs?

@felixfontein
Copy link
Contributor Author

  • Are you waiting on more approvals / comments?

Mainly this.

@felixfontein
Copy link
Contributor Author

I renamed two fields to include a prefix major_ for fields that denote major versions (versionmajor_version, redirect_replacement_versionredirect_replacement_major_version); see ansible-community/antsibull#617 (comment).

Copy link
Contributor

@gotmax23 gotmax23 left a comment

Choose a reason for hiding this comment

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

I didn't personally check the links/versions for validity, but this looks good to me other than that. I guess the only remaining thing is to actual run the linter in CI.

@felixfontein
Copy link
Contributor Author

The linting step is now run before test-building the releases.

@gotmax23
Copy link
Contributor

The linting step is now run before test-building the releases.

Should it be part of the release playbook in antsibull, instead?

@felixfontein
Copy link
Contributor Author

I don't think so, that playbook should use the data (and assume it's correct - it can fail if it isn't), but not do extra linting like making sure the order of the entries is correct.

@gotmax23
Copy link
Contributor

I don't think so, that playbook should use the data (and assume it's correct - it can fail if it isn't), but not do extra linting like making sure the order of the entries is correct.

Makes sense to me. We could also add the linter to the noxfile as a future enhancement.

@felixfontein
Copy link
Contributor Author

Anyone mind if I merge this?

@felixfontein
Copy link
Contributor Author

Let's wait until we decide the fate of ansible-community/antsibull#622.

@felixfontein
Copy link
Contributor Author

ansible-community/antsibull-docs#330 uses this data for the official docsite, to indicate that collections are deprecated and will be removed from Ansible.

Copy link
Contributor

@oraNod oraNod left a comment

Choose a reason for hiding this comment

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

Thanks @felixfontein

@samccann
Copy link
Contributor

So we should document this new workflow 'somewhere' So SC members know that once a vote to deprecate/remove a collection is final, they have to open a PR in this repo to add the details.

@mariolenz
Copy link
Collaborator

mariolenz commented Sep 13, 2024

So we should document this new workflow 'somewhere' So SC members know that once a vote to deprecate/remove a collection is final, they have to open a PR in this repo to add the details.

Maybe something like ansible/ansible-documentation#1884?

Feel free to close this PR if you think it's too broken to be fixed. It's just a suggestion 😉

edit:
With "this one" I meant the PR in ansible-documentation, not this this PR ;-)

@felixfontein felixfontein merged commit 5015d37 into ansible-community:main Sep 14, 2024
5 checks passed
@felixfontein
Copy link
Contributor Author

@mariolenz @gotmax23 @oraNod @samccann thanks for reviewing this!

@felixfontein felixfontein deleted the deprecate-rename-metadata branch September 14, 2024 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants