Fix set -e fails from env hooks
#1179
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
triarius
changed the title
set -e fails from env hooks
In POSIX shells, `set -e` is not respected in certain contexts. One of these is if it is in a function that is called on the left of a `||`. See https://stackoverflow.com/questions/19789102/why-is-bash-errexit-not-behaving-as-expected-in-function-calls We previously attempted to run this script in a function and exit status 2 if there was an error, but this was not working due to the above. Since it was not working, I took the liberty to change the exit code. Exit code 2 is too overloaded in my opinion. I chose 53. This number looks like "S3", failure to network with which could be the source of some transient failure of this hook.
triarius
force-pushed
the
pdp-1434-fix-set-e-fails-from-env-hooks
branch
from
ff50217
to
798633b
Compare
lucaswilric
added a commit
that referenced
this pull request
Mar 4, 2024
When we introduced the `environment` hook error trap in #1179, we set it to propagate into all `source`-d plugin environment hooks. Unfortunately, this breaks the `retry` function in the ECR plugin's environment hook (which runs a command and then checks the exit code to determine whether to retry) by interrupting execution before the exit code can be checked. Repro: buildkite-plugins/ecr-buildkite-plugin#101 Fix by temporarily unsetting `-E`, stopping propagation of the trap command into the plugin environment hook.
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In POSIX shells,
set -eis not respected in certain contexts. One of these is: if it is in a function that is called on the left of a||.See https://stackoverflow.com/questions/19789102/why-is-bash-errexit-not-behaving-as-expected-in-function-calls
We previously attempted to run this script in a function and exit status 2 if there was an error, but this was not working due to the above. Since it was not working, I took the liberty to change the exit code. Exit code 2 is too overloaded in my opinion.
I chose 53. This number looks like "S3", failure to network with which, could be the source of some transient failure of this hook.
I recommend review with whitespace changes hidden.