Skip to content
This repository has been archived by the owner on Mar 21, 2022. It is now read-only.

Support loading auth config for repos with no scheme in their URL #910

Merged
merged 2 commits into from
Nov 15, 2017

Conversation

56quarters
Copy link
Contributor

@56quarters 56quarters commented Oct 30, 2017

Support loading auth configuration for repos when their serverAddress
is specified with no scheme component but their entry in the configuration
file includes a scheme component.

Older versions of Docker write authentication tokens to the configuration
file including their scheme ("https://") but newer onces do not. Thus
in order to allow people to use the newer format (looking up auth for
a repo that isn't prefixed with scheme) with an older configuration file
we try prefixing the serverAddress with "https://" or "http://" in some cases.

Fixes #804

Support loading auth configuration for repos when their `serverAddress`
is specified with no scheme component but their entry in the configuration
file includes a scheme component.

Older versions of Docker write authentication tokens to the configuration
file including their scheme ("https://") but newer onces do not. Thus
in order to allow people to use the newer format (looking up auth for
a repo that isn't prefixed with scheme) with an older configuration file
we try prefixing the `serverAddress` with "http://" in some cases.

Fixes spotify#804
@codecov-io
Copy link

codecov-io commented Oct 30, 2017

Codecov Report

Merging #910 into master will increase coverage by 0.03%.
The diff coverage is 63.63%.

@@             Coverage Diff              @@
##             master     #910      +/-   ##
============================================
+ Coverage     66.04%   66.08%   +0.03%     
- Complexity      769      771       +2     
============================================
  Files           171      171              
  Lines          3240     3249       +9     
  Branches        369      372       +3     
============================================
+ Hits           2140     2147       +7     
- Misses          940      942       +2     
  Partials        160      160

davidxia
davidxia previously approved these changes Nov 13, 2017
Copy link
Contributor

@davidxia davidxia left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Just one thing to address.

try {
final URI serverAddressUri = new URI(serverAddress);
if (serverAddressUri.getScheme() == null) {
final String addrWithProto = "https://" + serverAddress;
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's also check with http://.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, will do.

@davidxia davidxia requested review from mattnworb and caipre November 13, 2017 16:15
@davidxia
Copy link
Contributor

@mattnworb @caipre How does this look?

@davidxia davidxia merged commit c6cd2e6 into spotify:master Nov 15, 2017
@demaniak
Copy link

Hi!

Has this been released yet?

@davidxia
Copy link
Contributor

Yes, should be in latest docker-client.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pushing to private registry fails with 'No basic auth' for older style config.json
5 participants