Skip to content

hack/bin/upload-image: Retry despite set -e#2953

Merged
akshaymankar merged 1 commit intodevelopfrom
akshaymankar/fix-retry-upload-image
Dec 28, 2022
Merged

hack/bin/upload-image: Retry despite set -e#2953
akshaymankar merged 1 commit intodevelopfrom
akshaymankar/fix-retry-upload-image

Conversation

@akshaymankar
Copy link
Member

Executing "$@" within first argument of if prevents set -e from immediately failing the whole script.

It could also be written as while ! "$@"; do ..., but then getting status of "$@" is more complicated as ! "$@" has status=0 and overwrites the value of $?.

Checklist

Executing `"$@"` within first argument of `if` prevents `set -e` from
immediately failing the whole script.

It could also be written as `while ! "$@"; do ...`, but then getting status of
`"$@"` is more complicated as `! "$@"` has status=0 and overwrites the value of
`$?`.
@akshaymankar akshaymankar temporarily deployed to cachix December 27, 2022 16:04 — with GitHub Actions Inactive
@akshaymankar akshaymankar temporarily deployed to cachix December 27, 2022 16:04 — with GitHub Actions Inactive
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Dec 27, 2022
Copy link
Contributor

@mdimjasevic mdimjasevic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My Bash skills are rusty, but I'd say the PR is OK.

@akshaymankar akshaymankar merged commit 3f9c17e into develop Dec 28, 2022
@akshaymankar akshaymankar deleted the akshaymankar/fix-retry-upload-image branch December 28, 2022 10:03
@jschaul
Copy link
Member

jschaul commented Jan 2, 2023

One could also do

set -e

# ... do stuff and if something fails fail the script

set +e

# ... do stuff which can fail but we don't care ....

set -e

# fail script if something fails

Which might be more readable (to me). But whatever works, really.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants