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

WIP Expose sslcontext after it was set. by KostyaSha · Pull Request #591 · docker-java/docker-java · GitHub

  1. docker-java
  2. docker-java

Repository navigation

WIP Expose sslcontext after it was set. - #591

Closed
KostyaSha wants to merge 2 commits into
docker-java:masterfrom
KostyaSha:exposeSSL
ConversationCommits (2)ChecksFiles changed
Closed

WIP Expose sslcontext after it was set.#591
KostyaSha wants to merge 2 commits into
docker-java:masterfrom
KostyaSha:exposeSSL

Conversation

KostyaSha commented Jun 5, 2016 •
edited by marcuslinke
Loading

Copy link
Copy Markdown
Member

This change is

codecov-io commented Jun 5, 2016

Copy link
Copy Markdown

Current coverage is 22.85%

Merging #591 into master will decrease coverage by <.01%

@@             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

Powered by Codecov. Last updated by 0a1b34c...95ea1ee

KostyaSha mentioned this pull request Jun 6, 2016
Closed

KostyaSha commented Jun 6, 2016

Copy link
Copy Markdown
Member Author

@KostyaSha-auto test it-1.11

KostyaSha commented Jun 8, 2016

Copy link
Copy Markdown
Member Author

@marcuslinke what would be the suggestion with dockerCertPath? Add one more setter with ignore cert path?

KostyaSha reviewed Jun 8, 2016
View reviewed changes
Comment thread src/main/java/com/github/dockerjava/core/DockerClientConfig.java
throw new DockerClientException(
"Enabled TLS verification (DOCKER_TLS_VERIFY=1) but certifate path (DOCKER_CERT_PATH) is not defined.");
return dockerCertPath;
// throw new DockerClientException(

KostyaSha Jun 8, 2016

Copy link
Copy Markdown
Member Author

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

problem is here

marcuslinke Jun 8, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

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).

KostyaSha Jun 8, 2016 •
edited
Loading

Copy link
Copy Markdown
Member Author

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

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...

marcuslinke Jun 8, 2016

Copy link
Copy Markdown
Contributor

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

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?

KostyaSha Jun 8, 2016

Copy link
Copy Markdown
Member Author

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

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?

marcuslinke Jun 8, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

I mean its bad design because for using a custom SSLContext you would have to do two things:

  1. disable usage of dockerCertPath in the config
  2. Set custom SSLContext into DockerCmdExecFActory

marcuslinke Jun 8, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

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.

marcuslinke commented Jun 9, 2016

Copy link
Copy Markdown
Contributor

@KostyaSha What do you think of #596?

KostyaSha commented Jun 12, 2016

Copy link
Copy Markdown
Member Author

Moving discussion for better realisation #596

KostyaSha closed this Jun 12, 2016
KostyaSha added this to the 3.0.1 milestone Jun 13, 2016
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

None yet

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