Use exec in docker entrypoints to ensure bundler receives stop signals#934
Conversation
eb81738 to
1ffd457
Compare
1ffd457 to
ceb03c8
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR updates the Docker entrypoint scripts to use exec when launching processes so that signals are forwarded properly, while also improving variable quoting and removing unnecessary assignments.
- Replaces Bash process with target processes via exec
- Consistently quotes variable usage to avoid unintended shell expansion
- Removes redundant environment variable assignments
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| docker/web-entrypoint.sh | Improves quoting of DATABASE_URL extractions and uses exec for signal handling |
| docker/sidekiq-entrypoint.sh | Similar quoting improvements with a minor inconsistency in quoting DATABASE_URL |
Comments suppressed due to low confidence (1)
docker/sidekiq-entrypoint.sh:17
- For consistency and to prevent any unexpected word splitting, consider quoting DATABASE_URL as in the other assignments, for example: DATABASE_NAME=$(echo "$DATABASE_URL" | awk -F[@/] '{print $5}').
DATABASE_NAME=$(echo $DATABASE_URL | awk -F[@/] '{print $5}')
|
|
||
| # run passed commands | ||
| bundle exec ${@} | ||
| exec bundle exec "${@}" |
There was a problem hiding this comment.
Quoting the entire "${@}" may cause the arguments to be passed as a single string; consider using exec bundle exec "$@" to correctly separate multiple command arguments.
| exec bundle exec "${@}" | |
| exec bundle exec "$@" |
There was a problem hiding this comment.
Copilot is wrong, see https://unix.stackexchange.com/a/129077
ceb03c8 to
6e14217
Compare
6e14217 to
543242c
Compare
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Thanks for contribution! |
Currently when stopping a Docker Compose stack using
docker compose stop/downDocker simply waits 10 seconds (default timeout) and then kills the contains. This happens because Bash is running as PID 1 inside the container. Bash receives the stop signal, but doesn't forward it to Dawarich. When usingexecto start Dawarich, Dawarich replaces the Bash process instead of being spawned as a child and therefore receives all signals directly. See krallin/tini#8 (comment) for a really good explanation on signals in Docker.Additionally, I escaped all variables and removed useless assignments. Previously, unquoted variable usages could lead to unwanted shell expansion (although it shouldn't have been a problem before when using "sane" inputs for the variables).