kevdoran commented on code in PR #10556:
URL: https://github.com/apache/nifi/pull/10556#discussion_r2554601301
##########
nifi-extension-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/ListenSyslog.java:
##########
@@ -101,26 +100,18 @@
@SeeAlso({PutSyslog.class, ParseSyslog.class})
public class ListenSyslog extends AbstractSyslogProcessor implements
ListenComponent {
- // See migrateProperties() below for how legacy properties are handled on
upgrade
- private static final String LEGACY_PORT_PROPERTY_NAME = "Port";
- private static final String LEGACY_PROTOCOL_PROPERTY_NAME = "Protocol";
-
- public static final PropertyDescriptor TCP_PORT = new PropertyDescriptor
- .Builder().name("TCP Port")
- .description("The port to listen on for TCP Syslog communication.
Either this or UDP Port must be set, but both cannot be set at the same time.")
- .required(false)
- .addValidator(StandardValidators.PORT_VALIDATOR)
+ public static final PropertyDescriptor TCP_PORT = new
PropertyDescriptor.Builder()
+ .fromPropertyDescriptor(PORT)
+ .name("TCP Port")
.identifiesListenPort(org.apache.nifi.components.listen.TransportProtocol.TCP,
"syslog")
- .expressionLanguageSupported(ExpressionLanguageScope.ENVIRONMENT)
+ .dependsOn(PROTOCOL, TCP_VALUE)
.build();
- public static final PropertyDescriptor UDP_PORT = new PropertyDescriptor
- .Builder().name("UDP Port")
- .description("The port to listen on for UDP Syslog communication.
Either this or TCP Port must be set, but both cannot be set at the same time.")
- .required(false)
- .addValidator(StandardValidators.PORT_VALIDATOR)
+ public static final PropertyDescriptor UDP_PORT = new
PropertyDescriptor.Builder()
+ .fromPropertyDescriptor(PORT)
+ .name("UDP Port")
Review Comment:
Same as above:
```suggestion
.name("UDP Port")
.displayName("UDP Port")
```
##########
nifi-extension-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/ListenSyslog.java:
##########
@@ -101,26 +100,18 @@
@SeeAlso({PutSyslog.class, ParseSyslog.class})
public class ListenSyslog extends AbstractSyslogProcessor implements
ListenComponent {
- // See migrateProperties() below for how legacy properties are handled on
upgrade
- private static final String LEGACY_PORT_PROPERTY_NAME = "Port";
- private static final String LEGACY_PROTOCOL_PROPERTY_NAME = "Protocol";
-
- public static final PropertyDescriptor TCP_PORT = new PropertyDescriptor
- .Builder().name("TCP Port")
- .description("The port to listen on for TCP Syslog communication.
Either this or UDP Port must be set, but both cannot be set at the same time.")
- .required(false)
- .addValidator(StandardValidators.PORT_VALIDATOR)
+ public static final PropertyDescriptor TCP_PORT = new
PropertyDescriptor.Builder()
+ .fromPropertyDescriptor(PORT)
+ .name("TCP Port")
Review Comment:
It's neat that it actually does work to have 2 properties with the same
display name (`Port` in this case) interchangeable using `dependsOn`, but I
think all in all it might be preferable to have the display name match the
property name, to make it clearer that the property is switching when the
protocol value changes, and therefore the value must also be set again... I
think it could also help with troubleshooting.
```suggestion
.name("TCP Port")
.displayName("TCP Port")
```
I don't feel strongly, so fee free to disregard if you feel differently.
I'll approve the PR either way.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
