This repository has been archived by the owner on Jul 25, 2024. It is now read-only.
fix: Call sub-scripts with bash instead of sh #714
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
github-actions
bot
added
component: delivery
rogerthatdev
suggested changes
Dec 8, 2022
|
|
||
| if [[ -z "${REPO_NAME}" || -z "${REPO_NAME}" ]] |
There was a problem hiding this comment.
I think we should keep the non-posix compliant double brackets, but yes this line should be changed to remove the second part of this OR
added 2 commits
December 8, 2022 12:32
github-actions
bot
added
type: feature request
unforced
changed the title
github-actions
bot
added
type: bug
github-actions
bot
added
type: bug
|
So made some edits to my top-level comment up there, but basically the issues in #713 were happening because we were calling the subscripts explicitly with sh which meant the shebang at the top telling them to run with bash didn't matter, and therefore we didn't get to use the fancy bash functionality like This PR is ready to go now |
unforced
commented
Dec 8, 2022
| @@ -295,7 +295,7 @@ if [[ -z "$SKIP_AUTH" ]]; then | |||
| read -rp "Would you like to configure $(tput bold)$(tput setaf 3)end-user authentication?$(tput sgr0) (y/n) " auth_yesno | |||
|
|
|||
| if [[ ${auth_yesno} == "y" ]]; then | |||
| sh ./scripts/configure_auth.sh | |||
| ./scripts/configure_auth.sh | |||
There was a problem hiding this comment.
Is this the right way to run it or should we use bash explicitly?
There was a problem hiding this comment.
this should work just fine 👍
unforced
changed the title
github-actions
bot
added
type: bug
rogerthatdev
approved these changes
Dec 8, 2022
| @@ -295,7 +295,7 @@ if [[ -z "$SKIP_AUTH" ]]; then | |||
| read -rp "Would you like to configure $(tput bold)$(tput setaf 3)end-user authentication?$(tput sgr0) (y/n) " auth_yesno | |||
|
|
|||
| if [[ ${auth_yesno} == "y" ]]; then | |||
| sh ./scripts/configure_auth.sh | |||
| ./scripts/configure_auth.sh | |||
There was a problem hiding this comment.
this should work just fine 👍
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
This fixes many of the key issues in #713
Which it turned out had less to do with POSIX compliance as it did with us explicitly calling sub-scripts configure-delivery and configure-auth using sh. Removing this call lets the shebang at the top of the files do their work so they end up being run with the environment bash.
On some machines, sh is bash so this wasn't an issue, but on many machines sh is a POSIX compliant dash variant.
This PR changes those calls to not run with sh, and also removes a weird conditional discovered in the process.