Skip to content

implement new docker config file format for authentication #727

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

Merged
merged 6 commits into from
Nov 18, 2017

Conversation

fbuecklers
Copy link
Contributor

@fbuecklers fbuecklers commented Oct 16, 2016

Maybe we should use a common implementation of the X-Registry-Auth header logic. Currently it is duplicated in each CommandImpl.

WDYT?


This change is Reviewable

@codecov-io
Copy link

codecov-io commented Nov 20, 2016

Codecov Report

Merging #727 into master will increase coverage by 0.31%.
The diff coverage is 66.43%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #727      +/-   ##
==========================================
+ Coverage   61.58%   61.89%   +0.31%     
==========================================
  Files         411      410       -1     
  Lines        8172     8152      -20     
  Branches      530      521       -9     
==========================================
+ Hits         5033     5046      +13     
+ Misses       2838     2809      -29     
+ Partials      301      297       -4
Impacted Files Coverage Δ
.../dockerjava/netty/exec/CreateContainerCmdExec.java 100% <100%> (ø) ⬆️
...om/github/dockerjava/core/command/AuthCmdImpl.java 100% <100%> (ø) ⬆️
...a/com/github/dockerjava/core/DockerClientImpl.java 74.25% <100%> (+0.72%) ⬆️
...github/dockerjava/netty/exec/PullImageCmdExec.java 100% <100%> (+7.69%) ⬆️
...thub/dockerjava/core/command/PullImageCmdImpl.java 84.21% <100%> (+8.02%) ⬆️
.../com/github/dockerjava/jaxrs/PushImageCmdExec.java 92.3% <100%> (-0.55%) ⬇️
.../com/github/dockerjava/jaxrs/PullImageCmdExec.java 100% <100%> (+7.69%) ⬆️
...ockerjava/core/command/CreateContainerCmdImpl.java 53.35% <100%> (+0.63%) ⬆️
...ithub/dockerjava/jaxrs/CreateContainerCmdExec.java 100% <100%> (ø) ⬆️
...github/dockerjava/netty/exec/PushImageCmdExec.java 92.3% <100%> (-0.55%) ⬇️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8902e32...b05b1bb. Read the comment docs.

@zifik
Copy link

zifik commented Apr 20, 2017

Does this help solve bug #316 ?

@fbuecklers
Copy link
Contributor Author

fbuecklers commented Apr 20, 2017

Yep, I use this changes to get the auth information from the new config.json file format.

import com.github.dockerjava.api.model.AuthConfig;
import com.github.dockerjava.api.model.AuthConfigurations;

public class AuthConfigFile {
Copy link
Member

Choose a reason for hiding this comment

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

seems we can't remove public class in minor release :(

return withOptionalAuthConfig(authConfig);
}

private CreateContainerCmd withOptionalAuthConfig(AuthConfig authConfig) {
Copy link
Member

Choose a reason for hiding this comment

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

what is the difference?

return authConfig;
}

public CreateContainerCmd withAuthConfig(AuthConfig authConfig) {
Copy link
Member

Choose a reason for hiding this comment

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

and here

@KostyaSha
Copy link
Member

Frankly speaking i don't understand what docker did with all this changes, my use case is calls from jenkins/java code with custom credentials, so i'm passing auth for all commands without using login.

I guess this configs are needed if docker-java is used as docker cli analogue?

@KostyaSha KostyaSha self-assigned this Apr 24, 2017
@KostyaSha KostyaSha removed their assignment Jun 28, 2017
@KostyaSha
Copy link
Member

master is open for refactorings, but people added additional hotfixes for auth. could you rebase/review/propose now changes?

@fbuecklers
Copy link
Contributor Author

fbuecklers commented Sep 7, 2017

Hi, yes I will rebase my pull request on weekend. Currently I do not have enough time for it.

@KostyaSha
Copy link
Member

please wait.. i'm refactoring all tests for master.

@fbuecklers
Copy link
Contributor Author

I have rebased my changes on the latest master, hope that this pull request is fine now

@fbuecklers
Copy link
Contributor Author

Are there any issues on my side, which cause the error?

@@ -197,6 +200,8 @@
@CheckForNull
Boolean isTty();

CreateContainerCmd withAuthConfig(AuthConfig authConfig);
Copy link
Member

Choose a reason for hiding this comment

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

why it needed for create container?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because docker can pull the image while creating a container, if it is missing on the current host. The auth config is then needed if a private registry is used.

add auth option to create image command
@fbuecklers
Copy link
Contributor Author

Ok, I have changed your last suggestions. Anything left?

@KostyaSha
Copy link
Member

let's try

@KostyaSha KostyaSha merged commit 97f4827 into docker-java:master Nov 18, 2017
@KostyaSha KostyaSha added this to the 3.1.0 milestone Nov 18, 2017
@fbuecklers fbuecklers deleted the new-auth-config branch November 21, 2017 11:45
KostyaSha added a commit to KostyaSha/docker-java that referenced this pull request Nov 27, 2018
Such use case existed before.
related to docker-java#727
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants