-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Feature/issue 1323 inetutils support #4600
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
base: master
Are you sure you want to change the base?
Feature/issue 1323 inetutils support #4600
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not yet have a look at the changes in detail yet, but please clean up the PR
@@ -0,0 +1,54 @@ | |||
<?xml version="1.0" encoding="UTF-8"?> | |||
<project xmlns="http://maven.apache.org/POM/4.0.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think all these oauth2 sample files do not belong here
*/ | ||
public class DefaultApplicationFactory implements ApplicationFactory { | ||
|
||
// Removed InetUtils dependency; using standard Java APIs for host resolution. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there was no InetUtils dependency removed here, please remove the comment
return address.getHostName(); | ||
default: | ||
return address.getCanonicalHostName(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did you change the syntax here?
|
||
private final MetadataContributor metadataContributor; | ||
|
||
private final Optional<?> inetUtils; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not one of the contributors of this project but just a client of it, but I must say that this <?>
really smells of bad code and potential broken injection points...
try { | ||
java.lang.reflect.Method m = utils.getClass().getMethod("findFirstNonLoopbackHostInfo"); | ||
Object host = m.invoke(utils); | ||
if (host instanceof String && StringUtils.hasText((String) host)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can probably be simplified with pattern matching for instanceof
if the Java version of the project allows it ;)
if (this.inetUtils != null && this.inetUtils.isPresent()) { | ||
Object utils = this.inetUtils.get(); | ||
try { | ||
java.lang.reflect.Method m = utils.getClass().getMethod("findFirstNonLoopbackHostInfo"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this may easily break in case of native compilation.
@erikpetzold your thoughts?
This PR implements support for Spring Cloud's InetUtils in DefaultApplicationFactory.getLocalHost().
Closes #1323.