-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Switch to send full AuthConfig object for build action #682
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
Conversation
In order to support the docker API for version 1.7+, this command changes the way the `X-Registry-Config` header is sent when attempting to build an image.
…into moutten-fix-build-auth
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 only issue that this causes, for my particular situation, is that docker-compose
would still be broke since it registers the client API version as 1.18. I don't know really how to detect what version the docker server is expecting. I'm not sure that an API version was changed when this authentication header was changed.
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.
ping @aanand :3
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.
Looks like the change went out in Docker 1.7.1, according to the milestone on this PR: moby/moby#14139
I don't see any switching being done on the API version, so it's possible that backwards compatibility is broken. I've asked for clarification: moby/moby#14139 (comment)
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.
From Josh's answer, seems like the change only affects 1.7.0 and was reverted in 1.7.1 - making this PR unnecessary. Users of 1.7.0 should upgrade to 1.7.1.
@moutten Can you confirm that this change is unnecessary with Docker 1.7.1 ?
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 change was still necessary with 1.7.1. You'll see that the previous code was including a top level key of configs
. Removing the top level key of configs
works with 1.7.1. Based on Josh's comment, I'm not sure that was previously necessary. I haven't tried testing to see if removing the configs
still works against older versions of docker (prior to 1.7).
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 tested my setup using docker version 1.6.2, docker-compose 1.3.3, along with a patched version of docker-py, and I can verify it did work correctly with the change in this pull request. This looks like the cleanest way for docker-py to implement this change. It looks like I'm going to have to wait for docker-compose to change the supported API to 1.19.
By the way, thanks @shin-, your solution is much better. |
Switch to send full AuthConfig object for build action
Thanks for following up so closely @moutten ! |
Follow-up to #677