WIP Expose sslcontext after it was set.#591
WIP Expose sslcontext after it was set.#591KostyaSha wants to merge 2 commits intodocker-java:masterfrom
Conversation
Current coverage is 22.85%@@ master #591 diff @@
==========================================
Files 296 296
Lines 6218 6220 +2
Methods 0 0
Messages 0 0
Branches 547 547
==========================================
Hits 1421 1421
- Misses 4705 4707 +2
Partials 92 92
|
|
@KostyaSha-auto test it-1.11 |
|
@marcuslinke what would be the suggestion with dockerCertPath? Add one more setter with ignore cert path? |
| throw new DockerClientException( | ||
| "Enabled TLS verification (DOCKER_TLS_VERIFY=1) but certifate path (DOCKER_CERT_PATH) is not defined."); | ||
| return dockerCertPath; | ||
| // throw new DockerClientException( |
There was a problem hiding this comment.
Hmm. I see... I don't like this. Maybe we have to rethink config class(es) again then. What about introducing a DockerClientConfig interface that must provide a SSLContext. Then we have a DefaultDockerClientConfig implementation of this DockerClientConfig interface that acts like the former DockerClientConfig class and someone (like you) could implement a second class that constructs the SSLContext in a different way (maybe from a database or whatever).
There was a problem hiding this comment.
Well, we have the mess in configs for creating connections i..e i hade DockerClientCOnfigBuilderForPlugin, then Builder for factory and then the whole builder for two builders... Now ssl moved...
Is it possible to make one Config for creating connection?
🏧 i can be satisfied even with additional boolean in DockerClientConfig...
There was a problem hiding this comment.
As I said: I really don't like this. This is bad design in my eyes. I will experiment locally with interface and different impl. I think only the default config impl needs a builder. If someone wants a different configuration implementation its his / her reponsibility to create the instance, maybe even with a custom builder. WDYT?
There was a problem hiding this comment.
I think anybody would be glad to fill configuration only in single DockerClientBuilder and get connection :)
As I said: I really don't like this.
Didn't get what exactly. Making one builder, adding boolean for now while there is no better solution to unblock programmatic usage or having multiple configs/builders?
There was a problem hiding this comment.
I mean its bad design because for using a custom SSLContext you would have to do two things:
- disable usage of dockerCertPath in the config
- Set custom SSLContext into DockerCmdExecFActory
There was a problem hiding this comment.
Maybe the existing config builder could create the SSLContext from dockerCertPath by default and pass it to the constructor of the config when build() is called. Then we could introduce a withCustomSSLContext method to the builder that overrides the default behavior. I think this would be the simplest and cleanest solution.
|
@KostyaSha What do you think of #596? |
|
Moving discussion for better realisation #596 |
3 participants
This change is