Move default Docker constants + methods into DockerHost

[davidxia]

Default Docker constants and methods for constructing them were
duplicated in DockerHost and DefaultDockerClient. Move them all into
DockerHost and add tests.

[davidxia]

@johnflavin @mattnworb

addresses #440

[johnflavin]

This exact series of method calls—fromNullable(getenv("DOCKER_HOST")).or(dockerUtils.defaultDockerEndpoint());—is used multiple times in different places. It seems to me this would be better as a single method in DockerUtils, something like getHost().

Same with fromNullable(getenv("DOCKER_CERT_PATH")).or(dockerUtils.defaultCertPath()).

[davidxia]

Ah good point

On Thursday, May 19, 2016, Flavin notifications@github.com wrote:

In src/main/java/com/spotify/docker/client/DefaultDockerClient.java
#441 (comment):

@@ -1685,9 +1684,11 @@ public static Builder builder() {
* @throws DockerCertificateException if we could not build a DockerCertificates object
*/
public static Builder fromEnv() throws DockerCertificateException {

• final String endpoint = fromNullable(getenv("DOCKER_HOST")).or(defaultEndpoint());
• final DockerUtils dockerUtils = new DockerUtils();
• final String endpoint = fromNullable(getenv("DOCKER_HOST"))

This exact series of method calls—
fromNullable(getenv("DOCKER_HOST")).or(dockerUtils.defaultDockerEndpoint());—is
used multiple times in different places. It seems to me this would be
better as a single method in DockerUtils, something like getHost().

Same with
fromNullable(getenv("DOCKER_CERT_PATH")).or(dockerUtils.defaultCertPath())
.

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
https://github.com/spotify/docker-client/pull/441/files/ac2c911da1dc2013c62a4f2950d13ae9857742da#r63953389

David Xia
Software Engineer
dxia@spotify.com

[mattnworb]

or easier to read IMHO:

endpoint = MoreObjects.firstNonNull(System.getenv("DOCKER_HOST"), dockerUtils.defaultCertPath());

[mattnworb]

1. I find it a little weird though (and probably bug-prone down the road) that DockerUtils has both accessible constants for DEFAULT_HOST etc, and methods for properties that have to be computed. I think it should all be the latter, e.g. defaultHost(), so that if there is ever a need to replace a constant DOCKER_HOST with a calculated value then the foundation is already there. In other words, if we have to use methods to access some properties, then make all of the property access via methods.
2. Couldn't all of this logic just go in DockerHost, if that class is meant to "represent a dockerd endpoint? It's kind of confusing to me why DefaultDockerClient uses DockerUtils, and DockerHost uses DockerUtils, but DefaultDockerClient (and everyone else) doesn't just use DockerHost.

[davidxia]

Having this as a static attribute that you can set during tests seems bad, but if this is an instance attribute, all the currently static methods like endpointFromEnv() would have to go into another static class in order to be used by the static factory method fromEnv(). Since setSystemDelegate() is package local, I thought this is OK although inelegant.

[mattnworb]

I think you could get away from this whole thing by having a way to pass in the "env" or "system properties" as a Map<String, String> in the test, rather than having to swap out a "delegate".

[davidxia]

@mattnworb Just for my own edification, in cases like this where you want to test a static method (eg defaultDockerEndpoint()) that calls an external depedency's final class' static methods (eg System.getProperty()), what are good approaches, if any? I set the static attribute with a package local static method, but what are other ways?

[mattnworb]

What is here feels like the best thing that I know of for this case. I suppose it is worth minimizing the chain of static method -> static method calls because of this reason.

[mattnworb]

👍