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(install): Use proper bash testing #673

Merged
merged 4 commits into from
Sep 14, 2020

Conversation

RichardBronosky
Copy link
Contributor

@RichardBronosky RichardBronosky commented Sep 14, 2020

Fixes #672

I split this PR up into 4 commits. The first one is the bare minimum for the issue. The rest are just consistency corrections that we neckbeards at irc://chat.freenode.net/%23bash would always make.

See: https://tldp.org/LDP/abs/html/comparison-ops.html#FTN.AEN3669

The old `[` test has a problem checking for empty strings in combination with
logical OR/AND. In this case, it causes the error message:
./install.sh: line 271: [: ==: unary operator expected
If the `:` side of the `||` were ever called, this `if` condition would cause
the error:
syntax error: operand expected (error token is "== 0")
- Change remaining uses of `[` to `[[`
- Don't use quotes around numbers used in arithmetic tests
- Use quotes around variables and command substitutions that could be null
@BYK BYK self-requested a review September 14, 2020 20:06
@BYK BYK changed the title Install script fixes for #672 fix(install): Use proper bash testing Sep 14, 2020
Copy link
Member

@BYK BYK left a comment

Choose a reason for hiding this comment

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

I tried to avoid bash-isms in the past but we already passed that point. Thanks for bringing order to this chaos! :)

@BYK BYK merged commit 5631d45 into getsentry:master Sep 14, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Dec 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error in install.sh caused by bad bash logic
2 participants