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

Add client port config override #65

Merged
merged 7 commits into from
Jan 9, 2014
Merged

Conversation

zarfide
Copy link
Contributor

@zarfide zarfide commented Jan 6, 2014

I will also let this propagate through the ecosystem before referencing the new enum in other packages.

This can be a temporary feature until completion of Netflix/eureka#71

@ghost ghost assigned allenxwang Jan 6, 2014
@cloudbees-pull-request-builder

ribbon-pull-requests #58 SUCCESS
This pull request looks good

shouldUseOverridePort = true;

}else{
logger.warn("%s set to force client port but no secure port is set, so ignoring", clientName);

Choose a reason for hiding this comment

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

I don't think "%s" works here. Should be "{}".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad. I have a bunch of these -- I'll just replace it with string concat.

@allenxwang
Copy link

An alternative solution would be adding this logic to DiscoveryEnabledNIWSServerList.obtainServersViaDiscovery(). This will enable us to solve both internal and external requirements in one place.

@zarfide
Copy link
Contributor Author

zarfide commented Jan 7, 2014

DiscoveryEnabledNIWSServerList

I like this idea -- I'll go ahead and put the logic there.

@cloudbees-pull-request-builder

ribbon-pull-requests #60 SUCCESS
This pull request looks good

@cloudbees-pull-request-builder

ribbon-pull-requests #61 SUCCESS
This pull request looks good

*/
@RunWith(PowerMockRunner.class)
@PrepareForTest( {DiscoveryManager.class, DiscoveryClient.class} )
@PowerMockIgnore("javax.management.*")

Choose a reason for hiding this comment

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

why is that necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@allenxwang
Copy link

Thanks for taking the extra step to mock out the Discovery APIs!

allenxwang pushed a commit that referenced this pull request Jan 9, 2014
Add client port config override
@allenxwang allenxwang merged commit 539307e into Netflix:master Jan 9, 2014
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.

None yet

3 participants