-
Notifications
You must be signed in to change notification settings - Fork 71
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
Change shell to sh
#22
Conversation
Thanks for the PR! Can you describe your case with CI? What was the problem exactly? Can it be solved by renaming the variable used in this script? |
In my CI script I had |
I'm not sure we want to maintain full POSIX sh compatibility (see shellcheck output below). Especially process substitution, which requires the use of temporary files as shown here: https://www.shellcheck.net/wiki/SC3001. Shellcheck output
In fact, in What about merging this PR, but leaving the |
Changes required are quite straight-forward, so I have updated the code to fix Shellcheck warnings. |
I have updated and tested the script: everything appears to be working right. Please have a look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks! I like that you managed to simplify the script despite sh being usually more verbose than bash 😃
Closes #19 |
Because the official
docker
images are based on alpine and don't come withbash
installed, it makes sense to use busybox's shell to run the script. Alsogrep
options are adjusted to be compatible with the version available in alpine.This addresses the issue #19.
Also, I have added an additional check for
$SERVICE
variable to see if it equals the first argument provided. This is to address a use case I had in my CI where the service name is provided with a variable and led to an error.