Refactor configuration of SSL to allow override with custom config#596
Refactor configuration of SSL to allow override with custom config#596marcuslinke merged 5 commits intomasterfrom
Conversation
| // CHECKSTYLE:OFF | ||
| @Override | ||
| public boolean equals(Object o) { | ||
| if (this == o) |
There was a problem hiding this comment.
doesn't reflectionEquals checks all this things?
|
from first lookup LGTM |
| if (customSslContext == null) { | ||
| if (dockerTlsVerify) { | ||
| dockerCertPath = checkDockerCertPath(dockerCertPath); | ||
| sslContext = new LocalDirectorySSLConfig(dockerCertPath).getSSLContext(); |
There was a problem hiding this comment.
Is it possible to store SSLConfig in DefaultClientConfig instead context?
I need know from what dockerCertPath connection was build.
Use case: calling default dockerclientconfig builder that gather configs, env vars, properties and they i extract dockercertPath for creating special remote test connection.
|
From second look i can't extract dockerCertPath from SSLContext, DefaultDockerClientConfig should store SSLConfig. |
|
Pushed commit into this branch |
Current coverage is 30.54%
@@ master #596 diff @@
==========================================
Files 296 296
Lines 6262 6260 -2
Methods 0 0
Messages 0 0
Branches 561 560 -1
==========================================
+ Hits 1464 1912 +448
+ Misses 4704 4348 -356
+ Partials 94 0 -94
|
|
What's the goal of your latest commit? Why you want |
Because i need know exact SSLConfig configuration that provided SSL connection. I can get hosts, names, etc, but not ssl part. That's why SSLConfig (like it was before) should be stored and not SSLContext. PS Found that |
|
Seems now works fine. Tested from my fork https://github.com/KostyaSha/docker-java/commits/3.0.0-kostyasha |
| */ | ||
| public class DefaultDockerClientConfig implements Serializable, DockerClientConfig { | ||
|
|
||
| private static final long serialVersionUID = -4307357472441531489L; |
|
TODO update README.md after merge because builder will change? |
|
@KostyaSha OK, understood now. Feel free to fix your comments and merge. |
|
ok |
| "Enabled TLS verification (DOCKER_TLS_VERIFY=1) but certifate path (DOCKER_CERT_PATH) is not defined."); | ||
| } else { | ||
| File certPath = new File(dockerCertPath); | ||
| } |
There was a problem hiding this comment.
removed nested ifs, should be better readable now
|
@marcuslinke please review |
3 participants
This change is