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
NIFI-11061: implemented NiFi Registry property for specifying network… #6931
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.
Thanks for the improvement @emiliosetiadarma! The basic approach looks good and follows the pattern of the NiFi implementation. As this is a new feature for Registry, there are a couple opportunities to simplify the approach.
...istry-core/nifi-registry-jetty/src/main/java/org/apache/nifi/registry/jetty/JettyServer.java
Outdated
Show resolved
Hide resolved
...istry-core/nifi-registry-jetty/src/main/java/org/apache/nifi/registry/jetty/JettyServer.java
Outdated
Show resolved
Hide resolved
...istry-core/nifi-registry-jetty/src/main/java/org/apache/nifi/registry/jetty/JettyServer.java
Outdated
Show resolved
Hide resolved
* | ||
* @return the property name and network interface name of all HTTPS network interfaces | ||
*/ | ||
public Map<String, String> getHttpsNetworkInterfaces() { |
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.
As the only usage of this method does not use the property names, it looks like this could be simplified to just return a Set<String>
of interface names that are not blank.
public Map<String, String> getHttpsNetworkInterfaces() { | |
public Set<String> getHttpsNetworkInterfaceNames() { |
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.
Will simplify
74f7183
to
2854945
Compare
… interface when connecting over HTTPS
2854945
to
2320d06
Compare
// determine if the property is a network interface name | ||
if (StringUtils.startsWith(propertyName, WEB_HTTPS_NETWORK_INTERFACE_PREFIX)) { | ||
// get the network interface property value | ||
networkInterfaceNames.add(getProperty(propertyName)); |
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.
This can add an empty value to networkInterfaceNames
, so this should be adjusted to filter out empty values.
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.
Thanks for working through the feedback @emiliosetiadarma, the latest version looks good and works as expected! +1 merging
… interface when connecting over HTTPS
Summary
NIFI-11061
Tracking
Please complete the following tracking steps prior to pull request creation.
Issue Tracking
Pull Request Tracking
NIFI-00000
NIFI-00000
Pull Request Formatting
main
branchVerification
Please indicate the verification steps performed prior to pull request creation.
Build
mvn clean install -P contrib-check
Licensing
LICENSE
andNOTICE
filesDocumentation