-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Exit batch files explictly using ERRORLEVEL #29583
Conversation
Pinging @elastic/es-core-infra |
@Mpdreamz Do you want to bring this PR up to date? There are a number of new bat files now that xpack has been opened. |
Yes, on vacay in Japan now won't be back for another week. |
ping @Mpdreamz |
4396f62
to
6a95092
Compare
I've updated this PR and also fixed cases where we were always setting the exit code to e.g currently cc @elastic/es-core-infra @elastic/microsoft |
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 left a question.
call "%~dp0elasticsearch-cli.bat" ^ | ||
%%* ^ | ||
|| exit /b 1 | ||
call "%~dp0elasticsearch-cli.bat" %%* |
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.
Is there a reason this wouldn't work? Then we can keep the exit close to the failing script, we really do want to error out immediately here.
call "%~dp0elasticsearch-cli.bat" %%* | |
call "%~dp0elasticsearch-cli.bat" ^ | |
%%* ^ | |
|| exit /b %errorlevel% |
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 review @jasontedor, finally took the effort to get back to this PR.
Sadly we need the exit /b %errorlevel%
after the endlocal
calls otherwise it does not survive in a non CMD
context.
I also tried the special :eof
label as documented here
call "%~dp0elasticsearch-cli.bat" %%* || goto :eof
We could get rid of the endlocal
calls alltogether since the local context is automatically closed when the bat file exits but non CMD contexts still need the explicit exit /B
I will update the PR with the following:
call "%~dp0elasticsearch-cli.bat" %%* || goto exit
endlocal
endlocal
:exit
exit /B %ERRORLEVEL%
Which I think makes the early exit explicit and satisfies carrying over the exit code to non CMD contexts.
|
||
endlocal | ||
endlocal | ||
exit /b %ERRORLEVEL% |
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.
exit /b %ERRORLEVEL% |
6a95092
to
d24b37d
Compare
call "%~dp0elasticsearch-cli.bat" ^ | ||
%%* ^ | ||
|| exit /b 1 | ||
call "%~dp0elasticsearch-cli.bat" %%* || goto exit |
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.
Would you maintain the line breaks that we have? It keeps these scripts looking the same as the Unix scripts too.
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 left one request, about the line breaks.
@jasontedor, updated the linebreaks on the bat files to match the unix scripts. |
Test this please |
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 are two new scripts:
- elasticsearch-node
- elasticsearch-shard
Would you address those as well please? Otherwise this looks good. Thanks for picking this up.
This makes sure the exit code is preserved when calling the batch files from different contexts other than DOS Fixes elastic#29582 This also fixes specific error codes being masked by an explict exit /b 1 causing the useful exitcodes from ExitCodes to be lost.
407f5e5
to
94d31bc
Compare
Thanks for taking a look again @jasontedor afaict |
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.
LGTM.
* Exit batch files explictly using ERRORLEVEL This makes sure the exit code is preserved when calling the batch files from different contexts other than DOS Fixes #29582 This also fixes specific error codes being masked by an explict exit /b 1 causing the useful exitcodes from ExitCodes to be lost. * fix line breaks for calling cli to match the bash scripts * indent size of bash files is 2, make sure editorconfig does the same for bat files * update indenting to match bash files * update elasticsearch-keystore.bat indenting * Update elasticsearch-node.bat to exit outside of endlocal (cherry picked from commit dfecb25)
* elastic/master: (68 commits) Fix potential IllegalCapacityException in LLRC when selecting nodes (elastic#37821) Mute CcrRepositoryIT#testFollowerMappingIsUpdated Fix S3 Repository ITs When Docker is not Available (elastic#37878) Pass distribution type through to docs tests (elastic#37885) Mute SharedClusterSnapshotRestoreIT#testSnapshotCanceledOnRemovedShard SQL: Fix casting from date to numeric type to use millis (elastic#37869) Migrate o.e.i.r.RecoveryState to Writeable (elastic#37380) ML: removing unnecessary upgrade code (elastic#37879) Relax cluster metadata version check (elastic#37834) Mute TransformIntegrationTests#testSearchTransform Refactored GeoHashGrid unit tests (elastic#37832) Fixes for a few randomized agg tests that fail hasValue() checks Geo: replace intermediate geo objects with libs/geo (elastic#37721) Remove NOREPLACE for /etc/elasticsearch in rpm and deb (elastic#37839) Remove "reinstall" packaging tests (elastic#37851) Add unit tests for ShardStateAction's ShardStartedClusterStateTaskExecutor (elastic#37756) Exit batch files explictly using ERRORLEVEL (elastic#29583) TransportUnfollowAction should increase settings version (elastic#37859) AsyncTwoPhaseIndexerTests race condition fixed (elastic#37830) Do not allow negative variances (elastic#37384) ...
This makes sure the exit code is preserved when calling the batch
files from different contexts other than DOS
Fixes #29582