exceptionfactory commented on code in PR #7228:
URL: https://github.com/apache/nifi/pull/7228#discussion_r1190095727


##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/resources/docs/org.apache.nifi.processors.standard.ExecuteStreamCommand/additionalDetails.html:
##########
@@ -0,0 +1,166 @@
+<!DOCTYPE html>
+<html lang="en">
+    <!--
+        Licensed to the Apache Software Foundation (ASF) under one or more
+        contributor license agreements.  See the NOTICE file distributed with
+        this work for additional information regarding copyright ownership.
+        The ASF licenses this file to You under the Apache License, Version 2.0
+        (the "License"); you may not use this file except in compliance with
+        the License.  You may obtain a copy of the License at
+        http://www.apache.org/licenses/LICENSE-2.0
+        Unless required by applicable law or agreed to in writing, software
+        distributed under the License is distributed on an "AS IS" BASIS,
+        WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or 
implied.
+        See the License for the specific language governing permissions and
+        limitations under the License.
+    -->
+    <head>
+        <meta charset="utf-8" />
+        <title>ExecuteStreamCommand</title>
+        <link rel="stylesheet" href="../../../../../css/component-usage.css" 
type="text/css" />
+    </head>
+
+    <body>
+        <h2>Description</h2>
+        <p>The ExecuteStreamCommand processor provides a flexible way to 
integrate external commands and scripts into NiFi data flows.
+        ExecuteStreamCommand can pass the incoming FlowFile's content to the 
command that it executes similarly how piping works.</p>
+
+        <h2>Configuration options</h2>
+
+        <h3>Working Directory</h3>
+        <p>If not specified, NiFi root will be the default working 
directory.</p>
+
+        <h3>Configuring command arguments</h3>
+        <p>The ExecuteStreamCommand processor provides two ways to specify 
command arguments: using Dynamic Properties and the Command Arguments field.</p>
+
+        <h4>Command Arguments field</h4>
+        <p>This is the default. If there are multiple arguments, they need to 
be separated by a character specified in the Argument Delimiter field.
+        When needed, '-' and '--' can be provided, but in these cases Argument 
Delimiter should be a different character.</p>
+
+        <p>Consider that we want to list all files in a directory which is 
different from the working directory:</p>
+
+        <table>
+            <tr>
+                <th>Command Path</th>
+                <th>Command Arguments</th>
+            </tr>
+            <tr>
+                <td>ls</td>
+                <td>-lah;/path/to/dir</td>
+            </tr>
+        </table>
+
+        <p><b>NOTE:</b> the command should be on <code>$PATH</code> or it 
should be in the working directory, otherwise path also should be specified.</p>
+
+        <h4>Dynamic Properties</h4>
+        <p>Arguments can be specified with Dynamic Properties. Dynamic 
Properties with the pattern of 'command.arguments.&lt;commandIndex&gt;' will be 
appended
+        to the command in ascending order.</p>
+
+        <p>The above example with dynamic properties would look like this:</p>
+
+        <table>
+            <tr>
+                <th>Property Name</th>
+                <th>Property Value</th>
+            </tr>
+            <tr>
+                <td>command.arguments.0</td>
+                <td>-lah</td>
+            </tr>
+            <tr>
+                <td>command.arguments.1</td>
+                <td>/path/to/dir</td>
+            </tr>
+        </table>
+
+        <h3>Configuring environment variables</h3>
+        <p>In addition to specifying command arguments using the Command 
Argument field or Dynamic Properties, users can also use environment variables 
with
+        the ExecuteStreamCommand processor. Environment variables are a set of 
key-value pairs that can be accessed by processes running on the system.
+        ExecuteStreamCommand will treat every Dynamic Property as an 
environment variable that doesn't match the pattern 
'command.arguments.&lt;commandIndex&gt;'.</p>
+
+        <p>Consider that we want to execute a maven command with the 
processor. If there are multiple Java versions installed on the system, you can 
specify

Review Comment:
   `Maven` should be capitalized.
   ```suggestion
           <p>Consider that we want to execute a Maven command with the 
processor. If there are multiple Java versions installed on the system, you can 
specify
   ```



##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/resources/docs/org.apache.nifi.processors.standard.ExecuteStreamCommand/additionalDetails.html:
##########
@@ -0,0 +1,166 @@
+<!DOCTYPE html>
+<html lang="en">
+    <!--
+        Licensed to the Apache Software Foundation (ASF) under one or more
+        contributor license agreements.  See the NOTICE file distributed with
+        this work for additional information regarding copyright ownership.
+        The ASF licenses this file to You under the Apache License, Version 2.0
+        (the "License"); you may not use this file except in compliance with
+        the License.  You may obtain a copy of the License at
+        http://www.apache.org/licenses/LICENSE-2.0
+        Unless required by applicable law or agreed to in writing, software
+        distributed under the License is distributed on an "AS IS" BASIS,
+        WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or 
implied.
+        See the License for the specific language governing permissions and
+        limitations under the License.
+    -->
+    <head>
+        <meta charset="utf-8" />
+        <title>ExecuteStreamCommand</title>
+        <link rel="stylesheet" href="../../../../../css/component-usage.css" 
type="text/css" />
+    </head>
+
+    <body>
+        <h2>Description</h2>
+        <p>The ExecuteStreamCommand processor provides a flexible way to 
integrate external commands and scripts into NiFi data flows.
+        ExecuteStreamCommand can pass the incoming FlowFile's content to the 
command that it executes similarly how piping works.</p>
+
+        <h2>Configuration options</h2>
+
+        <h3>Working Directory</h3>
+        <p>If not specified, NiFi root will be the default working 
directory.</p>
+
+        <h3>Configuring command arguments</h3>
+        <p>The ExecuteStreamCommand processor provides two ways to specify 
command arguments: using Dynamic Properties and the Command Arguments field.</p>
+
+        <h4>Command Arguments field</h4>
+        <p>This is the default. If there are multiple arguments, they need to 
be separated by a character specified in the Argument Delimiter field.
+        When needed, '-' and '--' can be provided, but in these cases Argument 
Delimiter should be a different character.</p>
+
+        <p>Consider that we want to list all files in a directory which is 
different from the working directory:</p>
+
+        <table>
+            <tr>
+                <th>Command Path</th>
+                <th>Command Arguments</th>
+            </tr>
+            <tr>
+                <td>ls</td>
+                <td>-lah;/path/to/dir</td>
+            </tr>
+        </table>
+
+        <p><b>NOTE:</b> the command should be on <code>$PATH</code> or it 
should be in the working directory, otherwise path also should be specified.</p>
+
+        <h4>Dynamic Properties</h4>
+        <p>Arguments can be specified with Dynamic Properties. Dynamic 
Properties with the pattern of 'command.arguments.&lt;commandIndex&gt;' will be 
appended
+        to the command in ascending order.</p>
+
+        <p>The above example with dynamic properties would look like this:</p>
+
+        <table>
+            <tr>
+                <th>Property Name</th>
+                <th>Property Value</th>
+            </tr>
+            <tr>
+                <td>command.arguments.0</td>
+                <td>-lah</td>
+            </tr>
+            <tr>
+                <td>command.arguments.1</td>
+                <td>/path/to/dir</td>
+            </tr>
+        </table>
+
+        <h3>Configuring environment variables</h3>
+        <p>In addition to specifying command arguments using the Command 
Argument field or Dynamic Properties, users can also use environment variables 
with
+        the ExecuteStreamCommand processor. Environment variables are a set of 
key-value pairs that can be accessed by processes running on the system.
+        ExecuteStreamCommand will treat every Dynamic Property as an 
environment variable that doesn't match the pattern 
'command.arguments.&lt;commandIndex&gt;'.</p>
+
+        <p>Consider that we want to execute a maven command with the 
processor. If there are multiple Java versions installed on the system, you can 
specify
+        which version will be used by setting the <code>JAVA_HOME</code> 
environment variable. The output FlowFile will looke like this if we run
+        <code>mvn</code> command with <code>--version</code> argument:</p>
+
+        <code>
+            <pre style="overflow:scroll">
+Apache Maven 3.8.6 (84538c9988a25aec085021c365c560670ad80f63)
+Maven home: /path/to/maven/home
+Java version: 11.0.18, vendor: Eclipse Adoptium, runtime: 
/path/to/default/java/home
+Default locale: en_US, platform encoding: UTF-8
+OS name: "mac os x", version: "13.1", arch: "x86_64", family: "mac"
+            </pre>
+        </code>
+
+        <table>
+            <tr>
+                <th>Property Name</th>
+                <th>Property Value</th>
+            </tr>
+            <tr>
+                <td>JAVA_HOME</td>
+                <td>path/to/another/java/home</td>
+            </tr>
+        </table>
+
+        <p>After specifying the <code>JAVA_HOME</code> property, you can 
notice that maven is using a different runtime:</p>
+        <code>
+            <pre>
+Apache Maven 3.8.6 (84538c9988a25aec085021c365c560670ad80f63)
+Maven home: /path/to/maven/home
+Java version: 11.0.18, vendor: Eclipse Adoptium, runtime: 
/path/to/another/java/home
+Default locale: en_US, platform encoding: UTF-8
+OS name: "mac os x", version: "13.1", arch: "x86_64", family: "mac"
+            </pre>
+        </code>
+
+        <h3>Streaming input to the command</h3>
+        <p>ExecuteStreamCommand passes the incoming FlowFile's content to the 
command that it executes similarly how piping works.
+        It is possible to disable this behavior with the Ignore STDIN 
property. In the above examples we didn't use the
+        incoming FlowFile's content, so in this cases we could leverage this 
property for additional performance gain.</p>

Review Comment:
   ```suggestion
           incoming FlowFile's content, so in this case we could leverage this 
property for additional performance gain.</p>
   ```



##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/ExecuteStreamCommand.java:
##########
@@ -363,8 +348,37 @@ protected PropertyDescriptor 
getSupportedDynamicPropertyDescriptor(final String
                     
.expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
                     .addValidator(ATTRIBUTE_EXPRESSION_LANGUAGE_VALIDATOR)
                     .build();
+        } else {
+            return new PropertyDescriptor.Builder()
+                    .name(propertyDescriptorName)
+                    .description("Sets the environment variable '" + 
propertyDescriptorName + "' for the process' environment")
+                    .dynamic(true)
+                    .addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
+                    .build();
         }
-        return null;
+    }
+
+    @Override
+    protected Collection<ValidationResult> customValidate(ValidationContext 
validationContext) {
+        final List<ValidationResult> validationResults = new 
ArrayList<>(super.customValidate(validationContext));
+
+        final String argumentStrategy = 
validationContext.getProperty(ARGUMENTS_STRATEGY).getValue();
+        if (DYNAMIC_PROPERTY_ARGUMENTS_STRATEGY.getValue() != 
argumentStrategy) {
+            for (final PropertyDescriptor propertyDescriptor : 
validationContext.getProperties().keySet()) {
+                if (!propertyDescriptor.isDynamic()) {
+                    continue;
+                }
+
+                final String propertyName = propertyDescriptor.getName();
+                final Matcher matcher = 
COMMAND_ARGUMENT_PATTERN.matcher(propertyName);
+                if (matcher.matches()) {
+                    logger.warn(String.format("'%s' should be set to '%s' when 
command arguments are supplied as Dynamic Properties. The property '%s' will be 
ignored.",

Review Comment:
   That sounds like a good approach. For this pull request, `String.format()` 
should not be used for the log message, instead `{}` placeholders should be 
used.
   ```suggestion
                       logger.warn("[{}] should be set to [{}] when command 
arguments are supplied as Dynamic Properties. The property [{}] will be 
ignored.",
   ```



-- 
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: issues-unsubscr...@nifi.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to