[GitHub] [jclouds] nacx commented on a change in pull request #64: JCLOUDS-1520 - Reuse SSLSocketFactory in JavaUrlHttpCommandExecutorSe…

[GitBox <gi...@apache.org>]

nacx commented on a change in pull request #64:
URL: https://github.com/apache/jclouds/pull/64#discussion_r412727800



##########
File path: core/src/main/java/org/jclouds/http/config/SSLModule.java
##########
@@ -43,8 +44,8 @@
    @Override
    protected void configure() {
       bind(HostnameVerifier.class).annotatedWith(Names.named("untrusted")).to(LogToMapHostnameVerifier.class);
-      bind(new TypeLiteral<Supplier<SSLContext>>() {
-      }).annotatedWith(Names.named("untrusted")).to(new TypeLiteral<UntrustedSSLContextSupplier>() {
+      bind(new TypeLiteral<Supplier<SSLSocketFactory>>() {
+      }).annotatedWith(Names.named("untrusted")).to(new TypeLiteral<UntrustedSSLSocketFactorySupplier>() {

Review comment:
       I'm a bit concerned about this change. There are still uses of the SSLContext injection. For example, take a look at the AapcheHC or the OkHttp drivers. Those would need to be updated too.
   
   I can also imagine that there can also be users that are providing a custom version of the SSLContext, to customize how it behaves, and I think we should try not to break them. Is there any way you can introduce your changes by keeping the old supplier? Would it be possible to have both, the old one and your SSLSocketFactory new one, and let your supplier use the injected SSLContext one? This way we keep backward-compatibility and the only thing we care about is our codebase using the factory supplier instead of directly getting it from the context.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org