X Tutup
Skip to content

Remove JAXRS/ApacheConnector implementation specific properties from DockerClientConfig#282

Merged
marcuslinke merged 1 commit intomasterfrom
refact-config
Jul 31, 2015
Merged

Remove JAXRS/ApacheConnector implementation specific properties from DockerClientConfig#282
marcuslinke merged 1 commit intomasterfrom
refact-config

Conversation

@marcuslinke
Copy link
Copy Markdown
Contributor

... so things like readTimeout, connectTimeout, maxTotalConnections, maxPerRouteConnections are configurable via DockerCmdExecFactoryImpl now.

@ericfjosne
Copy link
Copy Markdown

Looking at the changes, it should be fine.
However, I'm questioning what is now the best way to actually create a DockerClient?

Shouldn't the interface DockerCmdExecFactory contain the methods that were added, meaning:

public DockerCmdExecFactory withReadTimeout(Integer readTimeout);
public DockerCmdExecFactory withConnectTimeout(Integer connectTimeout);
public DockerCmdExecFactory withMaxTotalConnections(Integer maxTotalConnections);
public DockerCmdExecFactory withMaxPerRouteConnections(Integer maxPerRouteConnections);
public DockerCmdExecFactory withClientResponseFilters(ClientResponseFilter... clientResponseFilter);
public DockerCmdExecFactory withClientRequestFilters(ClientRequestFilter... clientRequestFilters);

I used to create my DockerClient object this way:
DockerClientConfig config = DockerClientConfig.createDefaultConfigBuilder()
.withUri(dockerRemoteApiUrl)
.withUsername(registryUser)
.withPassword(registryPass)
.withEmail(registryMail)
.withServerAddress(registryUrl)
.build();

DockerClient client = DockerClientBuilder.getInstance(config).build();

As of now, I don't really see a clean way to add those parameters. What would be perfect is something like this:

DockerClientConfig config = DockerClientConfig.createDefaultConfigBuilder()
.withUri(dockerRemoteApiUrl)
.withUsername(registryUser)
.withPassword(registryPass)
.withEmail(registryMail)
.withServerAddress(registryUrl)
.build();

DockerClientBuilder builder = DockerClientBuilder.getInstance(config);
DockerCmdExecFactory dExecFactory = DockerClientBuilder.getDefaultDockerCmdExecFactory();
dExecFactory.setReadTimeout(...);
dExecFactory.setConnectTimeout(...);
DockerClient client = builder.withDockerCmdExecFactory(dExectFactory).build();

My apologies if I just don't see it like I should, but many thanks for moving forward with this as a pull request.

Best regards,

Eric

@marcuslinke
Copy link
Copy Markdown
Contributor Author

Thanks for your feedback. As all these settings are highly implementation dependent the only way to configure is to do it manually like this:

DockerClientConfig config = DockerClientConfig.createDefaultConfigBuilder()
  .withUri(dockerRemoteApiUrl)
  .withUsername(registryUser)
  .withPassword(registryPass)
  .withEmail(registryMail)
  .withServerAddress(registryUrl)
  .build();

DockerCmdExecFactoryImpl dockerCmdExecFactory = new DockerCmdExecFactoryImpl()
  .withReadTimeout(1000)
  .withConnectTimeout(1000);

DockerClient dockerClient = DockerClientBuilder.getInstance(config)
  .withDockerCmdExecFactory(dockerCmdExecFactory)
  .build();

In know this is a bit cumbersome but the main goal is to free DockerClientConfig from implementation specific details. You can think of DockerClientConfig as an representation of the configuration possibilities of the docker CLI (https://docs.docker.com/reference/commandline/cli/). You can't configure something like a read timeout or request filters there.

@ericfjosne
Copy link
Copy Markdown

Apologies for the delay, but I still have to rework my old code in order to support the new workflow with the callbacks. I haven't had time yet to do so, but I'll let you know ASAP.

Thanks for your support!

@marcuslinke
Copy link
Copy Markdown
Contributor Author

@ericfjosne So do you think I should merge into master now?

@ericfjosne
Copy link
Copy Markdown

I say we have a winner indeed :)

screen shot 2015-07-31 at 14 33 58

By the way, the callback objects are really sexy compared to what was existing before !

marcuslinke added a commit that referenced this pull request Jul 31, 2015
Remove JAXRS/ApacheConnector implementation specific properties from DockerClientConfig
@marcuslinke marcuslinke merged commit a92bb00 into master Jul 31, 2015
@marcuslinke
Copy link
Copy Markdown
Contributor Author

@ericfjosne Great that it works for you now. FYI: Just released v2.0.0. Thanks for input!

@marcuslinke marcuslinke deleted the refact-config branch August 28, 2015 19:30
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.

2 participants

X Tutup