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

correct how to install Docker SDK for Python #74288

Merged
merged 3 commits into from
Apr 20, 2021

Conversation

daeseokyoun
Copy link
Contributor

SUMMARY

The latest version 5.0.0 do not support Python 2.7 anymore.

Documents and error message to guide docker/docker-py module with pip corresponding Python version.

ISSUE TYPE
  • Docs Pull Request
COMPONENT NAME

docker

ADDITIONAL INFORMATION

When install Docker SDK for Python 5.0.0 in system used Python 2.7, ansible failed to run related to docker module with following message.

"msg": "Failed to import the required Python library (Docker SDK for Python: docker (Python >= 2.7) or docker-py (Python 2.6)) on cstgnmss0022.lip2's Python /usr/bin/python. Please read module documentation and install in the appropriate location. If the required library is installed, but Ansible is using the wrong Python interpreter, please consult the documentation on ansible_python_interpreter, for example via `pip install docker` or `pip install docker-py` (Python 2.6). The error was: cannot import name credentials",

But it is not correct, the 5.0.0 do not support Python 2.7. We should install docker sdk version before 5.0.0 or install Python 3.6 in target system. so correct message following below:

"msg": "Failed to import the required Python library (Docker SDK for Python: docker above 5.0.0 (Python >= 3.6) or docker before 5.0.0 (Python >= 2.7) or docker-py (Python 2.6)) on sstgnmss0003.lip2's Python /usr/bin/python. Please read module documentation and install in the appropriate location. If the required library is installed, but Ansible is using the wrong Python interpreter, please consult the documentation on ansible_python_interpreter, for example via `pip install docker` (Python >= 3.6) or `pip install docker==4.4.4` (Python >= 2.7) or `pip install docker-py` (Python 2.6). The error was: cannot import name credentials"

@ansibot
Copy link
Contributor

ansibot commented Apr 15, 2021

@daeseokyoun The following file(s) in this pull request are bundled copies of modules used to support incidental tests and should not be updated:

  • test/support/integration/plugins/module_utils/docker/common.py
    • Possible match in the following collections: community.docker, community.kubernetes, community.yang, f5devcentral.cloudservices, f5networks.f5_beacon, holyhope.ovh, ibm.ibm_zhmc, jacklotusho.f5_modules, radware.radware_modules

Because the original module(s) have been migrated to collections, please re-submit this pull request in relevant collection repositories, typically under https://github.com/ansible-collections.

If you need further assistence with identifying the correct repository, please stop by IRC or the mailing list:

click here for bot help

@ansibot ansibot added affects_2.12 docs This issue/PR relates to or includes documentation. needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. needs_triage Needs a first human triage before being processed. new_contributor This PR is the first contribution by a new community member. python3 support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests. core_review In order to be merged, this PR must follow the core review workflow. and removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Apr 15, 2021
@ansibot
Copy link
Contributor

ansibot commented Apr 15, 2021

The test ansible-test sanity --test pep8 [explain] failed with 2 errors:

test/support/integration/plugins/module_utils/docker/common.py:340:161: E501: line too long (168 > 160 characters)
test/support/integration/plugins/module_utils/docker/common.py:341:161: E501: line too long (190 > 160 characters)

click here for bot help

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. core_review In order to be merged, this PR must follow the core review workflow. and removed core_review In order to be merged, this PR must follow the core review workflow. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Apr 15, 2021
@ansibot
Copy link
Contributor

ansibot commented Apr 15, 2021

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

test/support/integration/plugins/module_utils/docker/common.py:343:23: E127: continuation line over-indented for visual indent

click here for bot help

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. core_review In order to be merged, this PR must follow the core review workflow. and removed core_review In order to be merged, this PR must follow the core review workflow. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Apr 15, 2021
@jborean93 jborean93 removed the needs_triage Needs a first human triage before being processed. label Apr 15, 2021
@jborean93
Copy link
Contributor

cc @felixfontein not sure if you are still working on the Docker modules, if so are you able to look at the doc changes here. As for the modification of the help code I'm not 100% sure it's needed. If the module for Python 2.7 only supports up to a certain version then pip should be handling that automatically without the user having to supply docker<=4.4.4.

@jborean93 jborean93 added the needs_verified This issue needs to be verified/reproduced by maintainer label Apr 15, 2021
@felixfontein
Copy link
Contributor

cc @felixfontein not sure if you are still working on the Docker modules,

I still do.

if so are you able to look at the doc changes here. As for the modification of the help code I'm not 100% sure it's needed. If the module for Python 2.7 only supports up to a certain version then pip should be handling that automatically without the user having to supply docker<=4.4.4.

I also said this before, but it looks like that doesn't really work. In community.docker's CI some jobs were failing when docker-py 5.0.0 was released (CentOS 7 and Ubuntu 16.04) - see the first commit in ansible-collections/community.docker#120. Maybe the pip versions in these images are too old to support that?

@@ -334,11 +334,13 @@ def __init__(self, argument_spec=None, supports_check_mode=False, mutually_exclu

if not HAS_DOCKER_PY:
if NEEDS_DOCKER_PY2:
msg = missing_required_lib("Docker SDK for Python: docker")
msg = msg + ", for example via `pip install docker`. The error was: %s"
Copy link
Contributor

Choose a reason for hiding this comment

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

Changing this file makes no sense, this is a copy that is only used in ansible/ansible's integration tests. You should modify https://github.com/ansible-collections/community.docker/blob/main/plugins/module_utils/common.py#L264 instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Jinx 😉

@samdoran
Copy link
Contributor

Files in test/support/ are used only for testing and we only update them to address failures in CI. If a change needs to be made to the actual module, a PR should be submitted for the https://github.com/ansible-collections/community.docker repository since that module now lives in a collection outside this repository.

@jborean93
Copy link
Contributor

Thanks @felixfontein for confirming, disappointing that the metadata isn't present to have pip automatically use the older version.

@felixfontein
Copy link
Contributor

@jborean93 the setup.py included in https://files.pythonhosted.org/packages/fa/a2/e46d7c1b51394a09271a3b07c3a68deb3a669429beafd444d9553ed52868/docker-5.0.0.tar.gz does say python_requires='>=3.6', but I'm not very familiar with how setup.py files work to be able to say whether that's the correct way to do it or not...

@jborean93
Copy link
Contributor

Maybe it requires a pip version not present in CI as that's how it should work.

@daeseokyoun
Copy link
Contributor Author

@felixfontein @jborean93 Do I need to create new PR for this to community.docker repo? how about the docs/docsite/rst/scenario_guides/guide_docker.rst file?

@felixfontein
Copy link
Contributor

@daeseokyoun you still need to change docs/docsite/rst/scenario_guides/guide_docker.rst in this repo, but not the other file. For communit.docker you need to create a new PR.

@daeseokyoun
Copy link
Contributor Author

@felixfontein Thanks for your comment. I will update this PR to leave only the commit related to guide_docker.rst, and create a new PR for the common.py on community.docker repo.

The latest version 5.0.0 do not support Python 2.7 anymore.
It need to guide how to install docker / docker-py module for
proper Python version.
@ansibot ansibot added the docs_only All changes are to files within the docs/docsite/ directory label Apr 16, 2021
indicate available python version range with only text instead of using mathematical symbol.

Co-authored-by: Felix Fontein <felix@fontein.de>
@samccann samccann added the pr_day Has been reviewed during a PR review Day label Apr 20, 2021
Co-authored-by: Sandra McCann <samccann@redhat.com>
@acozine
Copy link
Contributor

acozine commented Apr 20, 2021

Thanks @daeseokyoun for this PR!

@samccann samccann merged commit 9369bd6 into ansible:devel Apr 20, 2021
acozine pushed a commit to acozine/ansible that referenced this pull request Apr 27, 2021
* docs: correct guide for the latest Docker SDK for Python
Co-authored-by: Felix Fontein <felix@fontein.de>
Co-authored-by: Sandra McCann <samccann@redhat.com>
Co-authored-by: Daeseok Youn <daeseok.youn@navercorp.com>
Co-authored-by: Alicia Cozine <879121+acozine@users.noreply.github.com>

(cherry picked from commit 9369bd6)
@acozine acozine mentioned this pull request Apr 27, 2021
acozine added a commit that referenced this pull request Apr 28, 2021
* Fix issue with version 3 in docs version list (#74089)

Previously would subsitute the "3" in "s3" instead of the version location in the URL

(cherry picked from commit 325ccf2)

* corrected epmhasis line (#74254)

need to be careful when adding lines as you create a different offset

(cherry picked from commit 7b39ee3)

* correct how to install Docker SDK for Python (#74288)

* docs: correct guide for the latest Docker SDK for Python
Co-authored-by: Felix Fontein <felix@fontein.de>
Co-authored-by: Sandra McCann <samccann@redhat.com>
Co-authored-by: Daeseok Youn <daeseok.youn@navercorp.com>
Co-authored-by: Alicia Cozine <879121+acozine@users.noreply.github.com>

(cherry picked from commit 9369bd6)

* Provide results to examples (#73984)

(cherry picked from commit 99a2b5f)

* remove deprecated ansible.module_utils._text from documentation (#73211)

According to comment in ansible.module_utils._text it is deprecated and
should not be used. This is now reflected in the documentation.

(cherry picked from commit 5e5bfa8)

* Update playbooks_filters.rst (#74242)

##### SUMMARY
Make the `random` filter description more clear.

(cherry picked from commit 5f391a7)

* Docs: Fix k8s_config_resource_name YAML example (#74129)

The `name` key should be beneath `metadata`:

(cherry picked from commit c9c8459)

* Update lookup.rst (#73716)

Document that users must pass `allow_unsafe=True` as an option in the lookup to allow templating, with a note about security implications.

Co-authored-by: Alicia Cozine <879121+acozine@users.noreply.github.com>
Co-authored-by: Sandra McCann <samccann@redhat.com>
(cherry picked from commit c0cc574)

* vmware: Add a note about known issue (#73273)

Signed-off-by: Abhijeet Kasurde <akasurde@redhat.com>
Co-authored-by: Sandra McCann <samccann@redhat.com>
Co-authored-by: Alicia Cozine <879121+acozine@users.noreply.github.com>
(cherry picked from commit 6e56e72)

Co-authored-by: Scott Sinclair <252082+pwae@users.noreply.github.com>
Co-authored-by: Brian Coca <bcoca@users.noreply.github.com>
Co-authored-by: daeseokyoun <daeseok.youn@gmail.com>
Co-authored-by: Baptiste Mille-Mathias <baptiste.millemathias@gmail.com>
Co-authored-by: schurzi <github@drachen-server.de>
Co-authored-by: yuri <1969yuri1969@gmail.com>
Co-authored-by: ml <6209465+ml-@users.noreply.github.com>
Co-authored-by: sry9681 <sry9681@users.noreply.github.com>
Co-authored-by: Abhijeet Kasurde <akasurde@redhat.com>
@ansible ansible locked and limited conversation to collaborators May 18, 2021
@sivel sivel removed the needs_verified This issue needs to be verified/reproduced by maintainer label Feb 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.12 core_review In order to be merged, this PR must follow the core review workflow. docs_only All changes are to files within the docs/docsite/ directory docs This issue/PR relates to or includes documentation. new_contributor This PR is the first contribution by a new community member. pr_day Has been reviewed during a PR review Day python3 support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants