Add DefaultDockerClientConfig.Builder#isDockerHostSetExplicitly - #1692
Conversation
…ilder has default value
| /** | ||
| * Considered default if dockerHost is set to the default value. | ||
| */ | ||
| public final boolean hasDefaultDockerHost() { |
There was a problem hiding this comment.
let's change it to isDockerHostSetExplicitly(), to detect user's intention to override it (even if set to the default value)
There was a problem hiding this comment.
Alright, will invert the logic (changed it to usesImplicitDockerHost() before).
As you can see, the checks are all over the place in DefaultDockerClientConfig now, I lack the idea of a clean way to capture all places atm.
There was a problem hiding this comment.
Have you considered removing docker.host from DEFAULT_PROPERTIES, using dockerHost != null to detect explicit value, and then passing dockerHost != null ? dockerHost : DEFAULT_DOCKER_HOST to DefaultDockerClientConfig from build()?
There was a problem hiding this comment.
would work nicely if dockerHost could be null in Builder 😐
But I will check further in this in this direction.
There was a problem hiding this comment.
It can now. However, it changes the contract of Builder.withDockerHost(), meaning it will only do parsing of host URI on build().
I would consider this an acceptable tradeoff for the dramatically simplified changeset.
There was a problem hiding this comment.
I think we can keep the validation, and call it from withProperties only if the value is not null
kiview
changed the title
DefaultDockerClientConfig.Builder has default valueDefaultDockerClientConfig.Builder set by user
| return this; | ||
| } | ||
|
|
||
| public final boolean isDockerHostSetExplicit() { |
There was a problem hiding this comment.
@rnorth I need your help with naming 😅 shouldn't it be "explicitly"?
There was a problem hiding this comment.
uff, it probably should :D
bsideup
changed the title
DefaultDockerClientConfig.Builder set by userDefaultDockerClientConfig.Builder#isDockerHostSetExplicitly
2 participants
The current implementation specifies that
hasDefaultDockerHost()returnsfalse, ifdockerHosthas a value different from the defaultDOCKER_HOSTvalue.This means, this implementation will also return
trueifdockerHostis set through ENV or properties to the default value.This PR would increase the robustness of testcontainers/testcontainers-java#4387.