-
Notifications
You must be signed in to change notification settings - Fork 952
Add set -e to buildspecs with multi line commands #6541
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
Conversation
buildspecs/build.yml
Outdated
| - echo $MAVEN_OPTIONS | ||
| - | | ||
| if [ "$JAVA_VERSION" -ge "9" ]; then | ||
| set -e |
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.
Why have we not done set -e as soon as we called | as in
- |
set -e
if [ "$JAVA_VERSION" -ge "9" ]; then
cd test/module-path-tests
mvn package
mvn exec:exec -P mock-tests
fi
?
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.
The check in if clause won't throw error, we don't need set -e if its false
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 ran a test below
version: 0.2
phases:
build:
commands:
- echo "Test with JAVA_VERSION somehow set incorrectly "
- export JAVA_VERSION=11.232
- |
if [ "JAVA_VERSION" -ge "9" ]; then
set -e
echo "Entering the if block"
cd /nonexistent/directory
echo "You will not see this"
fi
- echo "Build succeded even when if clause failed"
And the Build succeeded even though it gave a log as below
/codebuild/output/tmp/script.sh: line 4: [: JAVA_VERSION: integer expression expected
Build succeded even when if clause failed
I think currently we are assumming the If clause will always pass , there can be issues if the condition in if itself has some typos as I gave example
Inorder to make solution applicable to all the statements after | I think we need to add set -e as soon as we call |
This will also help us to to avoid any issues if we add else part or another if in future. WDYT ?
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.
Don't think we need it, but can move it up as it is harmless. JAVA_VERSION should never be null, and we don't currently have an else clause. The set e as is will apply to both mvn commands
We'll need to add set e for each separate multiline block that we add in the future
|
|
This pull request has been closed and the conversation has been locked. Comments on closed PRs are hard for our team to see. If you need more assistance, please open a new issue that references this one. |



Motivation and Context
In multi line commands, a subsequent successful command will mask a previous command that fails
Re-adding previous PR that was reverted due to integ test failures - #6524
Modifications
Add set -e to beginning of multi line commands in buildpsecs