bejancsaba commented on code in PR #8015: URL: https://github.com/apache/nifi/pull/8015#discussion_r1404636904
########## minifi/minifi-nar-bundles/minifi-framework-bundle/minifi-framework/minifi-resources/src/main/resources/bin/install-service.bat: ########## @@ -65,12 +125,21 @@ set LOG_PREFIX=minifi.log --StopMethod="%STOP_METHOD%" ^ --StopTimeout="%STOP_TIMEOUT%" ^ --Classpath="%CLASS_PATH%" ^ ---JvmOptions="%JAVA_ARGS%" ^ +--JvmOptions9="%JAVA_ARGS%" ^ Review Comment: That was unexpected :) I read up on it so at least now I know what this is ########## minifi/minifi-nar-bundles/minifi-framework-bundle/minifi-framework/minifi-resources/src/main/resources/bin/delete-service.bat: ########## @@ -16,8 +16,16 @@ rem See the License for the specific language governing permissions and rem limitations under the License. rem -set SERVICE_NAME=minifi -set SRV_BIN=%SERVICE_NAME%.exe - -REM Remove service -%SRV_BIN% //DS//%SERVICE_NAME% +setlocal enabledelayedexpansion + +set arg1=%1 +if "!arg1:~1,11!" equ "serviceName" ( Review Comment: This pattern is repeated in this PR multiple times so I will just comment here. Isn't there a cleaner way to do this on windows? I mean it clearly gets the job done but it is quite hard to read. While "findstr" has very limited pattern matching capabilities I think it should be able to handle this basic case or was there a reason for not using it? Also cutting the string along some delimiter (in this case the equal sign) would make it more robust as hardcoding positions can be super fragile. What do you think or you already tried these just was not working? ########## minifi/minifi-nar-bundles/minifi-framework-bundle/minifi-framework/minifi-resources/src/main/resources/bin/install-service.bat: ########## @@ -16,13 +16,77 @@ rem See the License for the specific language governing permissions and rem limitations under the License. rem -call minifi-env.bat +call %~sdp0\minifi-env.bat -set CONF_DIR=%MINIFI_ROOT%conf +setlocal enabledelayedexpansion +set "arg1=" +set "arg2=" +set "arg3=" +set "errorFlag=0" + +set "argCount=0" +for %%A in (%*) do ( + set /a "argCount+=1" + + if !argCount! equ 1 set "arg1=%%~A" + if !argCount! equ 2 set "arg2=%%~A" + if !argCount! equ 3 set "arg3=%%~A" + + if !argCount! geq 4 ( + set "errorFlag=tooManyArguments" + goto :error + ) + + if "!arg1:~0,11!" equ "serviceName" ( Review Comment: I think the indentation is not right here. ########## minifi/minifi-nar-bundles/minifi-framework-bundle/minifi-framework/minifi-resources/src/main/resources/bin/install-service.bat: ########## @@ -16,13 +16,77 @@ rem See the License for the specific language governing permissions and rem limitations under the License. rem -call minifi-env.bat +call %~sdp0\minifi-env.bat -set CONF_DIR=%MINIFI_ROOT%conf +setlocal enabledelayedexpansion +set "arg1=" +set "arg2=" +set "arg3=" +set "errorFlag=0" + +set "argCount=0" +for %%A in (%*) do ( Review Comment: Can we make this somewhat more intuitive, it is not straightforward what is happening here. There are at most 3 arguments here we could use the "%1", "%2" and "%3" notation match the argument to a variable and after this simple preprocessing I suppose the depth of the if / else below would be much more manageable. What do you think? -- 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
