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

Change shell to sh #22

Merged
merged 7 commits into from
Mar 4, 2024
Merged

Change shell to sh #22

merged 7 commits into from
Mar 4, 2024

Conversation

rcknr
Copy link
Contributor

@rcknr rcknr commented Jan 15, 2024

Because the official docker images are based on alpine and don't come with bash installed, it makes sense to use busybox's shell to run the script. Also grep 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.

@wowu
Copy link
Owner

wowu commented Jan 17, 2024

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?

@rcknr
Copy link
Contributor Author

rcknr commented Jan 17, 2024

In my CI script I had docker rollout $SERVICE so I can re-use it for multiple services. This failed because the script only checks only for variable existence not content. In my opinion, renaming won't solve the issue in the same way like a simple check I've added.

@wowu
Copy link
Owner

wowu commented Feb 4, 2024

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
./docker-rollout:9:4: warning: In POSIX sh, [[ ]] is undefined. [SC3010]
./docker-rollout:22:7: warning: In POSIX sh, [[ ]] is undefined. [SC3010]
./docker-rollout:23:6: warning: In POSIX sh, [[ ]] is undefined. [SC3010]
./docker-rollout:67:3: warning: In POSIX sh, 'local' is undefined. [SC3043]
./docker-rollout:78:3: warning: In POSIX sh, 'local' is undefined. [SC3043]
./docker-rollout:79:3: warning: In POSIX sh, 'local' is undefined. [SC3043]
./docker-rollout:87:6: warning: In POSIX sh, [[ ]] is undefined. [SC3010]
./docker-rollout:103:101: warning: In POSIX sh, process substitution is undefined. [SC3001]
./docker-rollout:118:10: warning: In POSIX sh, [[ ]] is undefined. [SC3010]
./docker-rollout:133:8: warning: In POSIX sh, [[ ]] is undefined. [SC3010]
./docker-rollout:150:10: note: Double quote to prevent globbing and word splitting. [SC2086]
./docker-rollout:150:26: note: Double quote to prevent globbing and word splitting. [SC2086]
./docker-rollout:153:7: warning: In POSIX sh, [[ ]] is undefined. [SC3010]
./docker-rollout:180:8: warning: In POSIX sh, [[ ]] is undefined. [SC3010]
./docker-rollout:192:4: warning: In POSIX sh, [[ ]] is undefined. [SC3010]
./docker-rollout:9:4: warning: In POSIX sh, [[ ]] is undefined. [SC3010]
./docker-rollout:22:7: warning: In POSIX sh, [[ ]] is undefined. [SC3010]
./docker-rollout:23:6: warning: In POSIX sh, [[ ]] is undefined. [SC3010]
./docker-rollout:67:3: warning: In POSIX sh, 'local' is undefined. [SC3043]
./docker-rollout:78:3: warning: In POSIX sh, 'local' is undefined. [SC3043]
./docker-rollout:79:3: warning: In POSIX sh, 'local' is undefined. [SC3043]
./docker-rollout:87:6: warning: In POSIX sh, [[ ]] is undefined. [SC3010]
./docker-rollout:103:101: warning: In POSIX sh, process substitution is undefined. [SC3001]
./docker-rollout:118:10: warning: In POSIX sh, [[ ]] is undefined. [SC3010]
./docker-rollout:133:8: warning: In POSIX sh, [[ ]] is undefined. [SC3010]
./docker-rollout:150:10: note: Double quote to prevent globbing and word splitting. [SC2086]
./docker-rollout:150:26: note: Double quote to prevent globbing and word splitting. [SC2086]
./docker-rollout:153:7: warning: In POSIX sh, [[ ]] is undefined. [SC3010]
./docker-rollout:[180](https://github.com/Wowu/docker-rollout/actions/runs/7531267073/job/21197415752?pr=22#step:3:187):8: warning: In POSIX sh, [[ ]] is undefined. [SC3010]
./docker-rollout:192:4: warning: In POSIX sh, [[ ]] is undefined. [SC3010]

In fact, in alpine images the default available shell is ash, which implements some of the bash features (e.g. [[). So after changing the shebang from #!/bin/bash to #!/bin/sh this script will not work on devices with POSIX-compatible /bin/sh if we don't fix everything that shellcheck suggests.

What about merging this PR, but leaving the #!/bin/bash shebang? This way we can have unofficial support for ash, requiring users to change /bin/bash to /bin/ash or /bin/sh.

@rcknr
Copy link
Contributor Author

rcknr commented Feb 4, 2024

Changes required are quite straight-forward, so I have updated the code to fix Shellcheck warnings.

@rcknr
Copy link
Contributor Author

rcknr commented Feb 5, 2024

I have updated and tested the script: everything appears to be working right. Please have a look.

Copy link
Owner

@wowu wowu left a 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 😃

@wowu wowu merged commit 8ab13a3 into wowu:main Mar 4, 2024
1 check passed
@wowu
Copy link
Owner

wowu commented Mar 4, 2024

Closes #19

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.

2 participants