Logic for when $METHOD is something unexpected #1917
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
… something unexpected.
ljharb
reviewed
Oct 2, 2018
install.sh
Outdated
| @@ -329,6 +329,9 @@ nvm_do_install() { | |||
| exit 1 | |||
| fi | |||
| install_nvm_as_script | |||
| else | |||
| echo >&2 'The environment variable $METHOD is set to "'"${METHOD}"'", which is not recognized as a valid installation method.' | |||
There was a problem hiding this comment.
Why the "'" thing, instead of something like this?:
echo >&2 "The environment variable \$METHOD is set to \"${METHOD}\", which is not recognized as a valid installation method.'There was a problem hiding this comment.
Personal style. \" is fine too. I just use "'" by default because \" has been difficult for me to get right when doing more complex quotation escapes in Bash.
There was a problem hiding this comment.
it seems like shellcheck is erroring out on this. can you update it so tests are passing? style choices beyond that can be figured out later :-)
There was a problem hiding this comment.
If I'm reading it right, this is the cause of the failure: "SC2016: Expressions don't expand in single quotes, use double quotes for that." I think getting rid of the single quotes like you suggest will fix that, so I'll give it a try.
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.
Print an error and exit the script if $METHOD is set to something unexpected.
In my own personal case, Salt was setting $METHOD to "loopback", and I did not realize it. With this patch, the script output will make it clear what's going on, and it will not pretend to complete successfully.
I did a small amount of testing of my own, but I don't have the tools or experience to complete all the testing asked for in CONTRIBUTING.md. Can someone else help with that?