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

Describe how collections can be removed from the Ansible package #201

Merged
merged 17 commits into from Apr 27, 2022

Conversation

felixfontein
Copy link
Contributor

SUMMARY

I wrote this up based on the discussion we had at today's community meeting. The aim of this PR is to have some basic rules on which collections can be removed so we can vote on them soon and establish them. The aim is not to have a complete list of rules. I expect we will have more discussions and additional situations added to this document in follow-up PRs.

The two processes described here are modelled after two explicit examples:

  1. community.kubevirt is currently broken and does not work as inclued in Ansible 5, since it requires community.kubernetes < 2.0.0, while Ansible 5 includes community.kubernetes >= 2.0.0. Adjusting the collection to community.kubernetes >= 2.0.0 or kubernetes.core >= 2.0.0 have been unsuccessful and stagnated (Get working with k8s.core 2 and above community.kubevirt#34 has been inactive for six months now). Since more breakage could happen with kubernetes.core 3.0.0, we do not want to keep this collection in Ansible without active maintainers who promise to keep the code working with current versions. The first of the two processes will apply to this collection.

  2. community.google seems to be unmaintained. It is unclear whether it still works (there is no indication it does, but also no indication that it does not). It uses very old and outdated dependencies, and CI has been broken for a long time before it was automatically disabled months ago. (The broken part was sanity checks only, unit tests were passing.) Since there has been no community interaction on this for a long time (there is no issue except some generic announcements, and there are no PRs), and there has never been any real community work on this collection, it is more likely that nobody is using it and it might even be totally broken without anyone realizing. The second process is written with this collection in mind. It gives community and users of this collection a chance to say that they use it and to step up helping it, but if nobody does, we have a process to remove it. (Also if it is removed folks can still manually install it.)

ISSUE TYPE
  • Docs Pull Request
COMPONENT NAME

removal_from_ansible.rst

Copy link
Contributor

@briantist briantist left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together @felixfontein !

It's only the Y = X+1 part I'm a little unclear on, the rest I'd feel comfortable approving now as a first set of rules that can be iterated on.

removal_from_ansible.rst Outdated Show resolved Hide resolved
removal_from_ansible.rst Outdated Show resolved Hide resolved
@Andersson007
Copy link
Contributor

@felixfontein thanks for starting this!
How about the following structure for each paragraph?

Case 1
======

General things here

Conditions
~~~~~~~~

* bullet point 1
* bullet point 2
* ...

Process
~~~~~~

#. step 1
#. step 2
#. ...

What do you think?

removal_from_ansible.rst Outdated Show resolved Hide resolved
removal_from_ansible.rst Outdated Show resolved Hide resolved
removal_from_ansible.rst Outdated Show resolved Hide resolved
removal_from_ansible.rst Outdated Show resolved Hide resolved
removal_from_ansible.rst Outdated Show resolved Hide resolved
removal_from_ansible.rst Outdated Show resolved Hide resolved
removal_from_ansible.rst Outdated Show resolved Hide resolved
removal_from_ansible.rst Outdated Show resolved Hide resolved
removal_from_ansible.rst Outdated Show resolved Hide resolved
removal_from_ansible.rst Outdated Show resolved Hide resolved
@Andersson007
Copy link
Contributor

@felixfontein
Copy link
Contributor Author

@felixfontein thanks for starting this! How about the following structure for each paragraph?

Case 1
======

General things here

Conditions
~~~~~~~~

* bullet point 1
* bullet point 2
* ...

Process
~~~~~~

#. step 1
#. step 2
#. ...

What do you think?

So you mean for every case (like unmaintained collection) there are multiple subsections (removal, stop removal, re-adding), every one of them following this stucture?

felixfontein and others added 2 commits March 25, 2022 07:40
Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>
@felixfontein
Copy link
Contributor Author

felixfontein commented Mar 30, 2022

Ooops, wrong PR...

@Andersson007
Copy link
Contributor

So you mean for every case (like unmaintained collection) there are multiple subsections (removal, stop removal, re-adding), every one of them following this stucture?

yep

Copy link
Contributor

@acozine acozine left a comment

Choose a reason for hiding this comment

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

Overall this is a great start. I made a few suggestions to clarify and simplify the language. I'd be happy to review the wording again as we keep moving forward.

removal_from_ansible.rst Outdated Show resolved Hide resolved
removal_from_ansible.rst Outdated Show resolved Hide resolved
removal_from_ansible.rst Show resolved Hide resolved
removal_from_ansible.rst Show resolved Hide resolved
removal_from_ansible.rst Show resolved Hide resolved
removal_from_ansible.rst Outdated Show resolved Hide resolved
removal_from_ansible.rst Show resolved Hide resolved
removal_from_ansible.rst Outdated Show resolved Hide resolved
removal_from_ansible.rst Show resolved Hide resolved
removal_from_ansible.rst Outdated Show resolved Hide resolved
felixfontein and others added 6 commits April 6, 2022 21:54
Co-authored-by: Alicia Cozine <879121+acozine@users.noreply.github.com>
Co-authored-by: Alicia Cozine <879121+acozine@users.noreply.github.com>
Copy link
Contributor

@Andersson007 Andersson007 left a comment

Choose a reason for hiding this comment

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

@felixfontein great job, thanks!
Couple of cosmetic things from me. Generally the process LGTM

removal_from_ansible.rst Outdated Show resolved Hide resolved
removal_from_ansible.rst Show resolved Hide resolved
removal_from_ansible.rst Outdated Show resolved Hide resolved
removal_from_ansible.rst Outdated Show resolved Hide resolved
removal_from_ansible.rst Show resolved Hide resolved
removal_from_ansible.rst Outdated Show resolved Hide resolved
felixfontein and others added 3 commits April 7, 2022 12:30
Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>
Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>
Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>
Copy link
Contributor

@Andersson007 Andersson007 left a comment

Choose a reason for hiding this comment

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

LGTM

@felixfontein
Copy link
Contributor Author

@acozine @briantist @mariolenz any more comments on this? If not, let's start a vote in community-topics.

@briantist
Copy link
Contributor

I'm good with a vote


The announcement mentioned below must state the reasons for the proposed removal and alert maintainers and the Ansible community that, to prevent the removal, the collection urgently needs new maintainers who can fix the problems.

#. Announce upcoming removal in Ansible X+1 as described in :ref:`announce_removal`.
Copy link
Contributor

Choose a reason for hiding this comment

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

The internal references and links don't work for me. This one, for example, links to removal_from_ansible.rst#id1 although I think it should link to removal_from_ansible.rst#announcing-upcoming-removal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is unfortunately a general problem with GitHub's RST renderer, you can also see it in action here: https://github.com/ansible-collections/overview/blob/main/collection_requirements.rst#collection-infrastructure Basically GH does not support :ref:, see github/markup#122.

I've tried to fix the references by converting them to links. Now rst2html5 from docutils properly handles them, but GitHub's rest2html does not: it shows links (https://github.com/ansible-collections/overview/blob/130944d4e056ec829f97a48de223522929292201/removal_from_ansible.rst), but uses the wrong hrefs so the links do nothing. This is really sad.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

This is unfortunately a general problem with GitHub's RST renderer

In that case, should we keep it as it is and vote? The links are broken on GitHub but the content is OK. Maybe we find a way to fix this later. Or GitHub fixes its RST renderer if we're lucky.

This is really sad.

Definitely.

@mariolenz
Copy link
Contributor

any more comments on this? If not, let's start a vote in community-topics.

The internal references don't work for me (see above). Is this a GitHub issue or some problem on my side?

Anyway, I've read through the document again and the text looks OK to me. So start a vote in community-topics if you want to.

Copy link
Contributor

@acozine acozine left a comment

Choose a reason for hiding this comment

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

Overall looks good. I added some ideas for making it better.

removal_from_ansible.rst Show resolved Hide resolved
removal_from_ansible.rst Outdated Show resolved Hide resolved
#. Re-add collection to ``collection-meta.yaml`` (``https://github.com/ansible-community/ansible-build-data/blob/main/<X>/collection-meta.yaml``).
#. If the removal was announced in the Ansible changelog for a version that has not yet been released (``https://github.com/ansible-community/ansible-build-data/blob/main/<X>/changelog.yaml``), remove the announcement.

Broken collections
Copy link
Contributor

Choose a reason for hiding this comment

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

I would still add a heading here for "Reasons we remove collections" and then make "Broken collections" and "Unmaintained collections" sub-sections of that. But we can do this after the content is voted on and merged in.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with Alicia's points here

removal_from_ansible.rst Outdated Show resolved Hide resolved
removal_from_ansible.rst Outdated Show resolved Hide resolved
Co-authored-by: Mario Lenz <m@riolenz.de>
@felixfontein
Copy link
Contributor Author

Thanks @gotmax23 and @mariolenz!

Copy link
Contributor

@gundalow gundalow left a comment

Choose a reason for hiding this comment

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

+1 to this once existing review comments have been addresses.

#. Re-add collection to ``collection-meta.yaml`` (``https://github.com/ansible-community/ansible-build-data/blob/main/<X>/collection-meta.yaml``).
#. If the removal was announced in the Ansible changelog for a version that has not yet been released (``https://github.com/ansible-community/ansible-build-data/blob/main/<X>/changelog.yaml``), remove the announcement.

Broken collections
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with Alicia's points here

@dmsimard
Copy link
Contributor

Hi @ansible-collections/steering-committee, please read and approve or state objections for this PR so we can move ahead. Thanks.

@acozine
Copy link
Contributor

acozine commented Apr 20, 2022

+1 to merging now. We can make it progressively better over time.

@mariolenz
Copy link
Contributor

Hi @ansible-collections/steering-committee, please read and approve or state objections for this PR so we can move ahead. Thanks.

It's still WIP. I won't approve a PR that's work in progress. Feels somehow wrong to me.

@felixfontein felixfontein changed the title [WIP] Describe how collections can be removed from the Ansible package Describe how collections can be removed from the Ansible package Apr 20, 2022
@felixfontein
Copy link
Contributor Author

Sorry, I completely forgot last week to remove the WIP mark and create a voting issue for this...

@Andersson007
Copy link
Contributor

Andersson007 commented Apr 21, 2022

+1 to merging this now.
@felixfontein i think it makes sense to put links to this doc in collection_requirements at least (with a separate PR).
Maybe also in the collection inclusion's README somewhere at the top in bold:)

Copy link
Contributor

@russoz russoz left a comment

Choose a reason for hiding this comment

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

+1 from my side

@felixfontein
Copy link
Contributor Author

Please use the voting issue (ansible-community/community-topics#90) to vote on this, and not this PR. Thanks!

Copy link
Contributor

@jamescassell jamescassell left a comment

Choose a reason for hiding this comment

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

+1

@felixfontein
Copy link
Contributor Author

Approved by ansible-community/community-topics#90

@felixfontein felixfontein merged commit b605a63 into main Apr 27, 2022
@felixfontein felixfontein deleted the removal branch April 27, 2022 18:15
@felixfontein
Copy link
Contributor Author

Thanks everyone!

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.

None yet

10 participants