-
-
Notifications
You must be signed in to change notification settings - Fork 225
Use exec in docker entrypoints to ensure bundler receives stop signals #934
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
base: dev
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 "${@}" |
Copilot
AI
Jun 5, 2025
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot is wrong, see https://unix.stackexchange.com/a/129077
ceb03c8 to
6e14217
Compare
6e14217 to
543242c
Compare
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).