Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support configuration of custom SSLContext/HostnameVerifier #1001

Merged

Conversation

mattnelson
Copy link
Contributor

Implementation of #816

Supports configuration of custom SSLContext/HostnameVerifier for jersey1/jersey2 implementations.

@mattnelson
Copy link
Contributor Author

@qiangdavidliu @twicksell Could I get a review on this?

@qiangdavidliu
Copy link
Contributor

@mattnelson we'll take a look. Thanks for the PR.

Copy link
Contributor

@qiangdavidliu qiangdavidliu left a comment

Choose a reason for hiding this comment

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

Some minor quibbles, but LGTM as a whole. Thanks for the PR!

instanceInfo.getActionType().name()};
logger.debug("The instance id %s is found with status %s and actiontype %s", args);
logger.debug("The instance id {} is found with status {} and actiontype {}",
instanceInfo.getStatus().name(), instanceInfo.getActionType().name());
Copy link
Contributor

Choose a reason for hiding this comment

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

You missed an instanceInfo.getId() here.

@@ -112,13 +116,18 @@ public void shutdown() {
public static JerseyEurekaHttpClientFactory create(EurekaClientConfig clientConfig,
Collection<ClientFilter> additionalFilters,
InstanceInfo myInstanceInfo,
AbstractEurekaIdentity clientIdentity) {
Copy link
Contributor

@qiangdavidliu qiangdavidliu Oct 27, 2017

Choose a reason for hiding this comment

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

I know this is very unlikely to be used outside of internal classes, but give that the method has a public signature, do you mind creating a new method with the optionals and delegate over (rather than changing) from the original? Thanks.

@@ -78,14 +80,19 @@ public void shutdown() {
public static Jersey2ApplicationClientFactory create(EurekaClientConfig clientConfig,
Collection<ClientRequestFilter> additionalFilters,
InstanceInfo myInstanceInfo,
AbstractEurekaIdentity clientIdentity) {
Copy link
Contributor

@qiangdavidliu qiangdavidliu Oct 27, 2017

Choose a reason for hiding this comment

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

I know this is very unlikely to be used outside of internal classes, but give that the method has a public signature, do you mind creating a new method with the optionals and delegate over (rather than changing) from the original? Thanks.

@qiangdavidliu qiangdavidliu merged commit 6efea5d into Netflix:master Oct 27, 2017
@mattnelson mattnelson deleted the discovery_config_ssl_hostname branch November 6, 2017 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants