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

Allow docker_login to function with credential helpers. #61207

Conversation

@arlaneenalra
Copy link

commented Aug 23, 2019

SUMMARY

This is a potential Fix for #55738

I leveraged Store from docker-py/docker ( Also available from docker-pycreds when using docker-py before version 4.0.0. It appears to be installed by along with python-docker and python3-docker on Ubuntu systems at least.) This natively handles interacting with the credential helpers for both login and logout operations.

To reduce the complexity of docker_login itself, moved the operations for updating config file based credentials into a class that implements the parts of Store's API that are needed. This pushes the complexity of dealing with the config_file further back and makes the changed/not changed logic much simpler.

Additionally, I've added a few very crude tests since there were none I could see for this module.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

docker_login

ADDITIONAL INFORMATION

(This is the squashed version. It took a little more effort to get this clean than I was expecting.)

@ansibot ansibot added needs_revision and removed core_review labels Aug 23, 2019

@arlaneenalra

This comment has been minimized.

Copy link
Author

commented Aug 23, 2019

@felixfontein Sorry to split things up, you commented on #61201 just before I closed it and opened this one.

@ansibot ansibot added core_review and removed needs_revision labels Aug 23, 2019

@felixfontein
Copy link
Contributor

left a comment

First, one question: is this backwards compatible? I.e. does it run and have the same effect as the old docker_login module for every docker-py version >= 1.8.0?

Second: you'll need a changelog fragment (minor_changes).

@@ -50,3 +50,7 @@ openshift ; python_version >= '2.7'

# requirement for maven_artifact
semantic_version

# requirement for docker_login
docker ; python_version >= '2.7'

This comment has been minimized.

Copy link
@felixfontein

felixfontein Aug 26, 2019

Contributor

I'm not sure whether that's acceptable. In general, unit tests are supposed to mock libraries they are using. (Otherwise, the testing container would be swamped in a large amount of random and potentially incompatible libraries :) )

This comment has been minimized.

Copy link
@arlaneenalra

arlaneenalra Aug 26, 2019

Author

Honestly, I didn't realize mocking an import was even possible. I'll update the code shortly.

NEEDS_DOCKER_PYCREDS = True


if NEEDS_DOCKER_PYCREDS:

This comment has been minimized.

Copy link
@felixfontein

felixfontein Aug 26, 2019

Contributor

What happens if it is missing? Will the module still work? (Right now, it never checks this variable again.)

This comment has been minimized.

Copy link
@arlaneenalra

arlaneenalra Aug 26, 2019

Author

At the moment, it will fail with an ImportError. I can work on adding a better failure message there, though having the imports in docker_login instead of in module_utils/docker/common.py could lead to some duplication.

https://github.com/ansible/ansible/blob/devel/lib/ansible/module_utils/docker/common.py#L282-L287
https://github.com/ansible/ansible/blob/devel/lib/ansible/module_utils/docker/common.py#L327-L333

(If you have docker-py you need docker-pycreds, if you're on an old docker, you'll also need it, if you're on a current docker you don't because the code is include.)

Although, it does look like docker has docker-pycreds in it's requirements going back to https://github.com/docker/docker-py/blame/545c5ac3cca04fbb95622f2440e15af186ff19fd/setup.py

This comment has been minimized.

Copy link
@felixfontein

felixfontein Aug 27, 2019

Contributor

Right now, it won't fail with ImportError because you're catching that. You don't need to copy any messages from module_utils, you need to add specific messages about the imports which fail only for this module. So there won't be any duplication.

The commit you cite (docker/docker-py@545c5ac) first appeared in docker-py 1.10.0. Does it mean that from 1.10.0 no additional requirements are needed (since the code is either included in docker-py itself, or installing docker-py requires te necessary module)? (And for 1.8.x and 1.9.x, they are needed?)

@ansibot ansibot removed the needs_triage label Aug 26, 2019

@arlaneenalra

This comment has been minimized.

Copy link
Author

commented Aug 26, 2019

First, one question: is this backwards compatible? I.e. does it run and have the same effect as the old docker_login module for every docker-py version >= 1.8.0?

That's going to be difficult to test for every step, I can install the 1.8.0 version and try it, assuming that version will even function with the docker systems I have access to here. What's an appropriate strategy here?

Second: you'll need a changelog fragment (minor_changes).

I'll get that added.

@felixfontein

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2019

First, one question: is this backwards compatible? I.e. does it run and have the same effect as the old docker_login module for every docker-py version >= 1.8.0?

That's going to be difficult to test for every step, I can install the 1.8.0 version and try it, assuming that version will even function with the docker systems I have access to here. What's an appropriate strategy here?

docker-py 1.8.0 also works with current docker daemons. At least it still did half a year ago when I tested it the last time :) Most integration tests are written so that they will also run with old docker-py versions (and will skip the parts they can't run if it is too old either for some features or for the whole module).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.