-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Refactoring of DockerClientConfig #447
Conversation
configuration options
and killing my pr? |
.withServerAddress("https://index.docker.io/v1/") | ||
.withDockerCertPath("/home/user/.docker") | ||
.withDockerHost("tcp://my-docker-host.tld:2376") | ||
.withDockerTlsVerify("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.
what 1 means?
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 same as with DOCKER_TLS_VERIFY environment variable. But maybe we should use boolean?
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.
maybe add additional method with boolean for programmatical usage?
seems need to be rebased. |
.withRegistryUrl("https://index.docker.io/v1/") | ||
.withRegistryUsername("dockeruser") | ||
.withRegistryPassword("ilovedocker") | ||
.withRegistryEmail("dockeruser@github.com") |
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.
why do you need registry related things in client initialisation?
I.e. if you have 2 auth registries you should call auth cmd and only then push.
I also not sure what will happen with parallel pushes...
PS. found, pull/push supports optional auth methods.
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.
Btw, how AuthCmd should work?
final AuthConfig authConfig = new AuthConfig();
authConfig.setPassword("docker-registry-password");
authConfig.setUsername("docker-registry-login");
authConfig.setEmail("sdf@sdf.com");
authConfig.setServerAddress(String.format("%s:%d", d.getHost(), nginxContainer.getExposedPort()));
DockerClientConfig clientConfig = new DockerClientConfig.DockerClientConfigBuilder()
// .withUsername("docker-registry-login")
// .withServerAddress(String.format("http://%s:%d", d.getHost(), nginxContainer.getExposedPort()))
.withUri(String.format("http://%s:44447", d.getHost()))
.build();
DockerCmdExecFactoryImpl dockerCmdExecFactory = new DockerCmdExecFactoryImpl()
.withReadTimeout(0)
.withConnectTimeout(10000);
DockerClient dockerClient = DockerClientBuilder.getInstance(clientConfig)
.withDockerCmdExecFactory(dockerCmdExecFactory)
.build();
final AuthResponse authResponse = dockerClient.authCmd()
.withAuthConfig(authConfig)
.exec();
LOG.debug(authResponse.getStatus());
With uncommented lines fails because authCmd()
checks for username+server:
java.lang.NullPointerException: Configured username is null.
at com.github.kostyasha.yad.docker_java.com.google.common.base.Preconditions.checkNotNull(Preconditions.java:226)
at com.github.kostyasha.yad.docker_java.com.github.dockerjava.core.DockerClientImpl.authConfig(DockerClientImpl.java:141)
at com.github.kostyasha.yad.docker_java.com.github.dockerjava.core.DockerClientImpl.authCmd(DockerClientImpl.java:162)
And it impossible to set new auth with later command.
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 think credentials in config originated from times when only docker hub was there and no private registries existed. So currently you can think of it as a default credential. There is an open issue #316 that is related to a refactoring of the auth configurations also. IMHO these authentication issues are out of scope of this PR and should be fixed in a separate one.
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.
ok, 🏧 developing ability to specify credentials for pull/push images for jenkins plugin and decided to ask during refactoring.
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.
Feel free to merge your changes, hope styling didn't break a lot. |
This PR addresses issue #331. Additionally it renames all the config options/keys of docker-java to better match with the docker CLI environment options.