FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Refactor configuration of SSL to allow override with custom config by marcuslinke · Pull Request #596 · docker-java/docker-java · GitHub

  1. docker-java
  2. docker-java

Repository navigation

Refactor configuration of SSL to allow override with custom config - #596

Merged
marcuslinke merged 5 commits into
masterfrom
refact-sslconfig
Jun 13, 2016
ConversationCommits (5)ChecksFiles changed
Merged

Refactor configuration of SSL to allow override with custom config#596
marcuslinke merged 5 commits into
masterfrom
refact-sslconfig

Conversation

marcuslinke commented Jun 9, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor

This change is

marcuslinke mentioned this pull request Jun 9, 2016
Closed
KostyaSha reviewed Jun 9, 2016
View reviewed changes
Comment thread src/main/java/com/github/dockerjava/core/DefaultDockerClientConfig.java Outdated
// CHECKSTYLE:OFF
@Override
public boolean equals(Object o) {
if (this == o)

KostyaSha Jun 9, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

doesn't reflectionEquals checks all this things?

KostyaSha commented Jun 9, 2016

Copy link
Copy Markdown
Member

from first lookup LGTM

KostyaSha reviewed Jun 12, 2016
View reviewed changes
Comment thread src/main/java/com/github/dockerjava/core/DefaultDockerClientConfig.java Outdated
if (customSslContext == null) {
if (dockerTlsVerify) {
dockerCertPath = checkDockerCertPath(dockerCertPath);
sslContext = new LocalDirectorySSLConfig(dockerCertPath).getSSLContext();

KostyaSha Jun 12, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

KostyaSha commented Jun 12, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

From second look i can't extract dockerCertPath from SSLContext, DefaultDockerClientConfig should store SSLConfig.

KostyaSha commented Jun 12, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

717d9f8

KostyaSha commented Jun 12, 2016

Copy link
Copy Markdown
Member

Pushed commit into this branch

codecov-io commented Jun 12, 2016 •
edited
Loading

Copy link
Copy Markdown

Current coverage is 30.54%

Merging #596 into master will increase coverage by 7.16%

  1. 2 files in ...thub/dockerjava/core were modified. more
    • Misses -4
    • Hits +4
  2. 2 files in ...om/github/dockerjava were modified. more
    • Misses -21
    • Hits +21
  3. 3 files (not in diff) in ...erjava/netty/handler were modified. more
    • Misses -19
    • Partials -2
    • Hits +21
  4. 17 files (not in diff) in ...ockerjava/netty/exec were modified. more
    • Misses -36
    • Hits +36
  5. 2 files (not in diff) in ...hub/dockerjava/netty were modified. more
    • Misses -11
    • Hits +11
  6. 5 files (not in diff) in ...kerjava/jaxrs/filter were modified. more
    • Misses -23
    • Hits +23
  7. 2 files (not in diff) in ...java/jaxrs/connector were modified. more
    • Misses -49
    • Hits +49
  8. 21 files (not in diff) in ...hub/dockerjava/jaxrs were modified. more
    • Misses -46
    • Hits +46
  9. 5 files (not in diff) in ...dockerjava/core/util were modified. more
    • Misses -24
    • Partials -6
    • Hits +30
  10. 2 files (not in diff) in ...java/core/dockerfile were modified. more
    • Misses -15
    • Partials -11
    • Hits +26
@@             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

Powered by Codecov. Last updated by 3a2dcc4...0063563

marcuslinke commented Jun 12, 2016

Copy link
Copy Markdown
Contributor Author

What's the goal of your latest commit? Why you want DockerClientConfig expose SSLConfig?

KostyaSha commented Jun 12, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

What's the goal of your latest commit? Why you want DockerClientConfig expose SSLConfig?

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.
Fix is not final, still testing it. Found NPE (lost if check).

PS Found that com.github.dockerjava.netty.DockerCmdExecFactoryImpl.InetSocketInitializer#init has
final SocketAddress proxyAddress = new InetSocketAddress(addr, 8008); that never used.

KostyaSha commented Jun 13, 2016

Copy link
Copy Markdown
Member

Seems now works fine. Tested from my fork https://github.com/KostyaSha/docker-java/commits/3.0.0-kostyasha

KostyaSha reviewed Jun 13, 2016
View reviewed changes
Comment thread src/main/java/com/github/dockerjava/core/DefaultDockerClientConfig.java
*/
public class DefaultDockerClientConfig implements Serializable, DockerClientConfig {

private static final long serialVersionUID = -4307357472441531489L;

KostyaSha Jun 13, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

1L if it new class

KostyaSha commented Jun 13, 2016

Copy link
Copy Markdown
Member

TODO update README.md after merge because builder will change?

marcuslinke commented Jun 13, 2016

Copy link
Copy Markdown
Contributor Author

@KostyaSha OK, understood now. Feel free to fix your comments and merge.

KostyaSha commented Jun 13, 2016

Copy link
Copy Markdown
Member

ok

KostyaSha reviewed Jun 13, 2016
View reviewed changes
Comment thread src/main/java/com/github/dockerjava/core/DefaultDockerClientConfig.java
"Enabled TLS verification (DOCKER_TLS_VERIFY=1) but certifate path (DOCKER_CERT_PATH) is not defined.");
} else {
File certPath = new File(dockerCertPath);
}

KostyaSha Jun 13, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

removed nested ifs, should be better readable now

KostyaSha commented Jun 13, 2016

Copy link
Copy Markdown
Member

@marcuslinke please review

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

type/enhancement

Projects

None yet

Milestone

3.0.1

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL