-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Script mode set -xe by default when no shebang used #1736
Conversation
Can you document this behavior at https://github.com/tektoncd/pipeline/blob/master/docs/tasks.md#step-script too? |
Fixes #1735 When I use script mode without a #! the current behaviour is to insert one for me - `#!/bin/sh`. This is helpful and removes an unneccessary line from my script. In addition I'd like two things by default: First I want the script to error out if any of the commands error out. Second I'd like to see the commands the script runs before their output. These two features are baked into `sh` with the `set -xe` flags. After this change the default behaviour of script mode when no shebang is provided is to insert both the default shebang as well as the flags required to make shell errors and shell echos happen.
Good call, done! |
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
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ImJasonH The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Hrm. Second time I've seen this today. The website works when I access it from a browser. /test pull-tekton-pipeline-build-tests |
The link check might be not as smart as we are hoping for 😛 |
Yeah, its picking up the URL as : https://en.wikipedia.org/wiki/Shebang_(Unix which does indeed return a 404 response |
/test pull-tekton-pipeline-build-tests |
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.
Nice, thank you!
What about setting -o pipefail too? Otherwise piped commands can hide failures.
Or go even further: https://vaneyckt.io/posts/safer_bash_scripts_with_set_euxo_pipefail/
The blog post is about bash
but the pipefail option works fine with sh
too - I verified on a busybox image:
/home # cat test.sh
cat test.sh
#!/bin/sh
set -euxo pipefail
MSG=${MYMSG:-hello!}
false | echo $MSG
/home # ./test.sh
./test.sh
+ MSG='hello!'
+ false
+ echo 'hello!'
hello!
/home # echo $?
echo $?
1
/test pull-tekton-pipeline-build-tests |
Changes
Fixes #1735
When I use script mode without a #! the current behaviour is to insert
one for me -
#!/bin/sh
. This is helpful and removes an unneccessaryline from my script. In addition I'd like two things by default: First
I want the script to error out if any of the commands error out. Second
I'd like to see the commands the script runs before their output. These
two features are baked into
sh
with theset -xe
flags.After this change the default behaviour of script mode when no shebang
is provided is to insert both the default shebang as well as the flags
required to make shell errors and shell echos happen.
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
Reviewer Notes
If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.
Release Notes