-
Notifications
You must be signed in to change notification settings - Fork 23.7k
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
docker scenario guide: docker-py -> docker #44856
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good addition, thanks. I pointed out a few places where you could make it even better.
@@ -43,12 +43,24 @@ Requirements | |||
------------ | |||
|
|||
Using the docker modules requires having `docker-py <https://docker-py.readthedocs.io/en/stable/>`_ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably also want to change the link and the link text in this line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's actually a good point. The link is correct (despite the docker-py
in it's name), same as the Github repository for the library still being https://github.com/docker/docker-py/. Also, in the modules (both source and documentation), the library is usually referenced as docker-py
(to distinguish it f.ex. from the docker
executable). In fact, the docker-py
authors admit (if you look at the checkmarks in this issue: docker/docker-py#1310) that they never finished the rename. It's really a big mess (and the main reason why all explanations tend to get long and ugly).
How about writing it as this (without mentioning docker
or docker-py
):
Using the docker modules requires having the `Docker SDK for Python <https://docker-py.readthedocs.io/en/stable/>`_
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds great, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. I also changed the module
a bit below to SDK (since module could be understood as Ansible module).
|
||
.. code-block:: bash | ||
|
||
$ pip install 'docker-py>=1.7.0' | ||
|
||
Please note that only one of ``docker`` and ``docker-py`` must be installed. Installing both will | ||
result in a broken installation, which can be recovered by first uninstalling both and then re-installing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What behavior would the user see if they had this broken installation? Error message? Failure?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docker modules will detect this and print an error (including instructions on how to fix it):
Cannot have both the docker-py and docker python modules 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 module. It is recommended to install the docker module if no support for Python 2.6 is required. Please note that simply uninstalling one of the modules can leave the other module in a broken state.
https://github.com/ansible/ansible/blob/devel/lib/ansible/module_utils/docker_common.py#L185-L189
About the above paragraph: how about adding "If this happens, Ansible will detect it and inform you about it." between the first two sentences?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, good solution. If you know what text Ansible will spit out, include that too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed it to Please note that only one of ``docker`` and ``docker-py`` must be installed. Installing both will result in a broken installation. If this happens, Ansible will detect it and inform you about it::
followed by the error message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
d'oh, that was the error text - thanks for including it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep; it got a bit longer over time, but hopefully it will prevent some more issues and complaints from people who think they will have docker
still installed (after removing docker-py
) and then getting strange error messages... ;)
SUMMARY
I noticed that in all the PRs (#43238, #42457) which tell users to install docker instead of docker-py (short summary:
docker-py
has been renamed todocker
for version 2.0, so please installdocker
if you don't need Python 2.6 support; also don't install both at the same time as both use the same namespace), I never updated the scenario guide. Well, here's an update for it.CC @kassiansun @acozine
ISSUE TYPE
COMPONENT NAME
docs/docsite/rst/scenario_guides/guide_docker.rst
ANSIBLE VERSION