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

Removal of docker content for 2.0.0 #1304

Merged
merged 7 commits into from
Nov 23, 2020

Conversation

felixfontein
Copy link
Collaborator

SUMMARY

Removes all docker content from community.general 2.0.0. Adds redirects to community.docker.

ISSUE TYPE
  • Removal Pull Request
COMPONENT NAME

docker content

@felixfontein felixfontein added the breaking_change This PR contains a breaking change that MUST NOT be backported label Nov 14, 2020
@ansibullbot
Copy link
Collaborator

@felixfontein This PR was evaluated as a potentially problematic PR for the following reasons:

  • More than 50 changed files.

Such PR can only be merged by human. Contact a Core team member to review this PR on IRC: #ansible-devel on irc.freenode.net

click here for bot help

@ansibullbot ansibullbot added WIP Work in progress affects_2.10 needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Nov 14, 2020
@felixfontein
Copy link
Collaborator Author

My idea is to merge this one once we're preparing this collection for the 2.0.0 release, i.e. after releasing 1.3.0 (the last planned minor 1.x.0 release).

Let's polish this one so we can use it as a template for other content (community.routeros, community.postgresql, ...).

If you use Ansible 2.9 and the ``docker`` plugins or modules from this collections, community.general 2.0.0 results in errors when trying to use the docker content by FQCN, like ``community.general.docker_container``.
Since Ansible 2.9 is not able to use redirections, you will have to adjust your playbooks and roles manually to use the new FQCNs (``community.docker.docker_container`` for the previous example) and to make sure that you have ``community.docker`` installed.

If you use ansible-base 2.10 or newer and did not install Ansible 2.11, but installed (and/or upgraded) community.general manually, you need to make sure to also install ``community.docker`` if you are using any of the ``docker`` plugins or modules.
Copy link
Contributor

Choose a reason for hiding this comment

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

Did not install Ansible 2.11 ?? Did you mean 2.10?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The first Ansible version this will be contained in is actually 2.11 :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(And now it's 3.0.0 and no longer 2.11...)

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.

Changelog looks great, thank you.

@Andersson007
Copy link
Contributor

after this is done and merged, i'll do the same for postgres stuff

Copy link
Contributor

@dmsimard dmsimard 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 felixfontein changed the title [WIP] Removal of docker content for 2.0.0 Removal of docker content for 2.0.0 Nov 21, 2020
@ansibullbot ansibullbot added community_review needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR and removed WIP Work in progress labels Nov 21, 2020
@felixfontein felixfontein merged commit f896c29 into ansible-collections:main Nov 23, 2020
@felixfontein
Copy link
Collaborator Author

@acozine @samccann @gundalow @Andersson007 @dmsimard thanks a lot for reviewing and improving this PR!

@SimonGurney
Copy link

Is there no better way to handle the movement of plugins?

This PR broke my AWX workflows as it could no longer find the docker_login plugin under general. Could the docker plugins not be marked as deprecated under general and modified to output a warning etc rather than breaking orchestration overnight?

If not, could a warning not be added to the docs for 12 months and the plugin be available in both collections?

I shall pin the version of the library in my requirements file but I find this a less than ideal solution...

@felixfontein
Copy link
Collaborator Author

@SimonGurney with semantic versioning, you have two choices:

  1. You add version restrictions so software stays at the actual major version. That way you should not get breaking changes. (Can still happen, but at least people usually try to avoid that.)
  2. You do not add version restrictions. That's equivalent to stating "I don't mind when my system breaks". That's what you apparently chose to do, so please do not complain when it happens.

@SimonGurney
Copy link

Your own documentation states:

Deprecations are expected to have a deprecation cycle of at least 2 major versions (i.e. ~1 year). Maintainers can use a longer deprecation cycle if they want to support the old code for that long.

And yet you deprecated docker out of general without any warning...

I understand the benefit of semantic versioning, and use them when I feel I need to. I did not expect such poor disregard for all the organisations currently using this collection in their playbooks. Maybe stick to your own guidelines?

@felixfontein
Copy link
Collaborator Author

We did not deprecate the docker content. We moved it to another collection. That's a big difference. The content is still there, it's not removed from the ecosystem, you only need to update FQCN (and install the new collection, in case you did not simply install Ansible 2.10) if you do not use a modern Ansible/ansible-base/ansible-core version.

Also we did announce this change in time, and that actions are required for Ansible 2.9 users: https://github.com/ansible-collections/community.general/blob/stable-1/CHANGELOG.rst#major-changes-2
It is also explicitly mentioned in the community.general 2.0.0 changelog: https://github.com/ansible-collections/community.general/blob/stable-2/CHANGELOG.rst#breaking-changes-porting-guide

@SimonGurney
Copy link

We did not deprecate the docker content. We moved it to another collection. That's a big difference. The content is still there, it's not removed from the ecosystem, you only need to update FQCN (and install the new collection, in case you did not simply install Ansible 2.10) if you do not use a modern Ansible/ansible-base/ansible-core version.

I disagree with this distinction. You removed it one from collection and put it in another, meaning the existing references are now broken in AWX / Tower playbooks.

You say you gave adequate notice. Your guidelines say you will give ~ 12 months notice, you notification was issued on Nov 26 2020.

Whether you like it or not, and I note you are keen to blame the end user here, there will be plenty of people realising they have broken implementations because you felt it was ok to do this. This damages the reputation of Ansible collections.

You should have kept it in General for 12 months, with documentation advising users move immediately to the more specific collection. That's not rocket science, its a courtesy to your user base.

@felixfontein
Copy link
Collaborator Author

The references are only broken when using Ansible 2.9. Upgrade to a newer version of Ansible / ansible-base / ansible-core if you want redirections to just work.

I don't see why we should annoy all users of more modern versions of Ansible just so that 2.9 users are happy. Because that's the only other choice: annoy everyone by deprecating something that is not really deprecated, but just continues to work for everyone but Ansible 2.9 users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking_change This PR contains a breaking change that MUST NOT be backported community_review has_issue needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants