sh tasks: Run with set -e #711
Conversation
This ensures that the task doesn't continue when an error happened. This is also best practice in bash.
| @@ -1,4 +1,7 @@ | |||
| #!/usr/bin/env bash | |||
|
|
|||
| set -e | |||
There was a problem hiding this comment.
this should be tested a bit, I don't know if this works out of the box on all operating systems
There was a problem hiding this comment.
set is a bash builtin, so assuming the script is executed with Bash (as the shebang requires), then it should work fine.
ref: https://www.gnu.org/software/bash/manual/html_node/The-Set-Builtin.html
There was a problem hiding this comment.
That wasn't my point. I don't know if the whole code is compatible with -e.
There was a problem hiding this comment.
Either the code does proper error checking, either the test suite needs some fixes 🤷
From my point of view, the best test case is to release it to the public and if this cause trouble, fix the trouble.
There was a problem hiding this comment.
I pasted these into https://www.shellcheck.net/ and didn't see any findings related to error exits. Lots of other findings in install_shell.sh though.
|
I'm running this through our supported platforms to see if anything breaks. |
There was a problem hiding this comment.
Tests passed in https://jenkins-platform.delivery.puppetlabs.net/view/puppet-agent/view/puppetlabs-puppet_agent%20module/view/ad-hoc/job/forge-module_puppetlabs-puppet-agent-module_intn-sys_pa-acceptance_7-nightly_to_8-nightly-adhoc/46/ except some newer platforms failed to install due to using an older beaker 3.0.x I think it's because this PR was created before we updated our modulesync dependencies.
| @@ -1,5 +1,7 @@ | |||
| #!/bin/sh | |||
|
|
|||
There was a problem hiding this comment.
Shebang does not require bash here, so open to interpretation (default /bin/sh is dash on Ubuntu), @bastelfreak .
This ensures that the task doesn't continue when an error happened. This is also best practice in bash.