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.

Sign up for GitHub

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

Jump to bottom

Error if docker and docker-py are simultaneously #38884

Merged
merged 2 commits into from Apr 17, 2018
Merged

Error if docker and docker-py are simultaneously #38884

merged 2 commits into from Apr 17, 2018

Conversation

sivel
Copy link
Member

@sivel sivel commented Apr 17, 2018

SUMMARY

Error if docker and docker-py are simultaneously installed over top of each other. Fixes #36125

I did a bit of research and found that docker.models only exists in the docker python package, and docker.ssladapter only in docker-py. By importing these we can determine if they are installed simultaneously and error appropriately.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

lib/ansible/module_utils/docker_common.py

ANSIBLE VERSION
2.5
2.6
ADDITIONAL INFORMATION

@sivel sivel requested a review from chouseknecht April 17, 2018 14:16
@sivel sivel mentioned this pull request Apr 17, 2018
Closed
@ansibot ansibot added bug This issue/PR relates to a bug. needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Apr 17, 2018
@ryansb ryansb removed the needs_triage Needs a first human triage before being processed. label Apr 17, 2018
chouseknecht
chouseknecht reviewed Apr 17, 2018
View reviewed changes
lib/ansible/module_utils/docker_common.py Outdated
@@ -144,6 +163,10 @@ def __init__(self, argument_spec=None, supports_check_mode=False, mutually_exclu
required_together=required_together_params,
required_if=required_if)

if HAS_DOCKER_MODELS and HAS_DOCKER_SSLADAPTER:
self.fail("Cannot have both the docker-py and docker python modules installed installed together as they use the same namespace and "
Copy link
Contributor

Choose a reason for hiding this comment

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

Message cleanup needed. Notice installed installed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Duplicate "installed" removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

This was merged so fast I didn't put in my $0.02: 'docker' and 'docker-py' are not 'python modules'. They are 'python packages' and should be called that to avoid confusion with Ansible's 'docker modules'.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest this language be:

Cannot have both the docker-py and docker Python packages installed together as they use the same namespace and cause a corrupt installation. Please uninstall both packages, and re-install only the docker-py or docker Python package.

If it's possible to add a single quote without causing a parsing problem, it would be even better as:

Cannot have both the 'docker-py' and 'docker' Python packages installed together as they use the same namespace and cause a corrupt installation. Please uninstall both packages, and re-install only the 'docker-py' or 'docker' Python package.

@chouseknecht
Copy link
Contributor

One small comment on message text. Otherwise, LGTM.

@chouseknecht
Copy link
Contributor

CI fail is an 'Unstabble' Fedora 24 thing that seems unrelated. Merging.

@chouseknecht chouseknecht merged commit 68e3ff8 into ansible:devel Apr 17, 2018
sivel added a commit to sivel/ansible that referenced this pull request Apr 17, 2018
@sivel
Error if docker and docker-py are simultaneously (ansible#38884)
c99ae77 
* Error if docker and docker-py are simultaneously installed over top of each other. Fixes ansible#36125

* Remove duplicate installed

(cherry picked from commit 68e3ff8)
@sivel sivel mentioned this pull request Apr 17, 2018
Merged
skylerbunny
skylerbunny reviewed Apr 17, 2018
View reviewed changes
Copy link
Contributor

@skylerbunny skylerbunny left a comment

Choose a reason for hiding this comment

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

Error could be changed if you feel it appropriate

lib/ansible/module_utils/docker_common.py Outdated
@@ -144,6 +163,10 @@ def __init__(self, argument_spec=None, supports_check_mode=False, mutually_exclu
required_together=required_together_params,
required_if=required_if)

if HAS_DOCKER_MODELS and HAS_DOCKER_SSLADAPTER:
self.fail("Cannot have both the docker-py and docker python modules installed installed together as they use the same namespace and "
Copy link
Contributor

Choose a reason for hiding this comment

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

This was merged so fast I didn't put in my $0.02: 'docker' and 'docker-py' are not 'python modules'. They are 'python packages' and should be called that to avoid confusion with Ansible's 'docker modules'.

lib/ansible/module_utils/docker_common.py Outdated
@@ -144,6 +163,10 @@ def __init__(self, argument_spec=None, supports_check_mode=False, mutually_exclu
required_together=required_together_params,
required_if=required_if)

if HAS_DOCKER_MODELS and HAS_DOCKER_SSLADAPTER:
self.fail("Cannot have both the docker-py and docker python modules installed installed together as they use the same namespace and "
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest this language be:

Cannot have both the docker-py and docker Python packages installed together as they use the same namespace and cause a corrupt installation. Please uninstall both packages, and re-install only the docker-py or docker Python package.

If it's possible to add a single quote without causing a parsing problem, it would be even better as:

Cannot have both the 'docker-py' and 'docker' Python packages installed together as they use the same namespace and cause a corrupt installation. Please uninstall both packages, and re-install only the 'docker-py' or 'docker' Python package.

sivel added a commit that referenced this pull request Apr 20, 2018
@sivel
Backport #38884 for 2.5 - docker/docker-py guard (#38889)
07ca962 
* Error if docker and docker-py are simultaneously (#38884)

* Error if docker and docker-py are simultaneously installed over top of each other. Fixes #36125

* Remove duplicate installed

(cherry picked from commit 68e3ff8)

* Add changelog for #36470
@sivel sivel mentioned this pull request May 2, 2018
Closed
desimaniac added a commit to Cloudbox/Cloudbox that referenced this pull request May 19, 2018
@desimaniac
Ansible: Newer versions of ansible do not like having both docker and…
cacd87b 
… docker-py installed.

Ref: ansible/ansible#38884
@desimaniac desimaniac mentioned this pull request May 21, 2018
Closed
ilicmilan pushed a commit to ilicmilan/ansible that referenced this pull request Nov 7, 2018
@sivel @ilicmilan
Error if docker and docker-py are simultaneously (ansible#38884)
617d2cf 
* Error if docker and docker-py are simultaneously installed over top of each other. Fixes ansible#36125

* Remove duplicate installed
@ansible ansible locked and limited conversation to collaborators Apr 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@chouseknecht chouseknecht chouseknecht left review comments

@skylerbunny skylerbunny skylerbunny left review comments

Assignees
No one assigned
Labels
bug This issue/PR relates to a bug. docker support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Milestone
No milestone
6 participants
@sivel @chouseknecht @skylerbunny @dagwieers @ryansb @ansibot