Skip to content
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

fix: format shell script and set option #473

Closed
wants to merge 1 commit into from

Conversation

bpgould
Copy link

@bpgould bpgould commented Feb 6, 2023

Remove extra space and set pipefail option

set -o pipefail
This setting prevents errors in a pipeline from being masked. If any command in a pipeline fails, that return code will be used as the return code of the whole pipeline. By default, the pipeline's return code is that of the last command even if it succeeds.

@linux-foundation-easycla
Copy link

CLA Missing ID CLA Not Signed

@bpgould
Copy link
Author

bpgould commented Feb 6, 2023

I signed the CLA, but I am not sure if I can re-run the CI check

@tsloughter
Copy link
Member

Was there something failing that led you to add this? I may be missing it but I don't see any pipes being used in this script. Does something get translated to a pipeline?

@bpgould
Copy link
Author

bpgould commented Feb 6, 2023

Was there something failing that led you to add this? I may be missing it but I don't see any pipes being used in this script. Does something get translated to a pipeline?

I was browsing the repo and noticed the whitespace, thought I would make a quick PR. Pipefail is generally a best practice for any shell script, thus I don't think YAGNI applies.

@tylerbenson
Copy link
Member

Unfortunately quick PRs still require the CLA. Do you want to go through that process or should we close the PR?

I do appreciate the consideration though. For future reference, here's the recommended settings for bash scripts:
https://sipb.mit.edu/doc/safe-shell/

@tylerbenson
Copy link
Member

Fixed in #1226

@tylerbenson tylerbenson closed this May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants