Wowu / docker-rollout

🚀 Zero Downtime Deployment for Docker Compose
https://github.com/Wowu/docker-rollout
MIT License
2.18k stars 59 forks source link

Change shell to `sh` #22

Closed rcknr closed 4 months ago

rcknr commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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

rcknr commented 5 months ago

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

Wowu commented 4 months ago

Closes #19