Compose: init containers#25435
Conversation
✅ Deploy Preview for docsdocker ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🔴 CRITICAL
This PR adds documentation for the new pre_start lifecycle hook. Three issues need attention before merging: one unresolved internal placeholder that would ship to users, and two factual inconsistencies between the how-to guide and the reference page.
d9d77ea to
6cf1272
Compare
glours
left a comment
There was a problem hiding this comment.
Just 1 little nit to fix before merging, thanks @aevesdocker 🙏
Co-authored-by: Guillaume Lours <705411+glours@users.noreply.github.com>
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
Two medium-severity issues found in the new pre_start reference documentation added by this PR. Both are in content/reference/compose-file/services.md.
The new how-to page (content/manuals/compose/how-tos/init-containers.md) is well-written and clear. No missing redirects, no vendored content concerns, no front matter issues.
dvdksn
left a comment
There was a problem hiding this comment.
Three issues with the Postgres examples that need fixing before merge.
| - data:/data | ||
|
|
||
| db: | ||
| image: postgres:16 |
There was a problem hiding this comment.
This example is confusing because it mixes two unrelated concerns in one pre_start sequence:
./manage.py migrate— a DB migration (needs thedbservice)chown -R 1000:1000 /data— fixing ownership of a volume
The app service has volumes: data:/data but the db service has no volume. A reader trying to understand pre_start from this example will wonder why the app is chowning a path that the database doesn't share, and why the database has no persistent volume of its own.
Also, db: image: postgres:16 has no POSTGRES_PASSWORD, POSTGRES_USER, or POSTGRES_DB — the postgres container won't start.
Suggest splitting into two focused examples: one for migrations (the DB case, already done in init-containers.md) and one for volume ownership (the busybox/chown case, already done separately in init-containers.md). The combined example here adds confusion rather than showing a realistic use case.
| - command: ["./manage.py", "migrate"] | ||
|
|
||
| db: | ||
| image: postgres:18 |
There was a problem hiding this comment.
Two problems with this db service that would prevent it from actually working:
-
No
POSTGRES_PASSWORD— Postgres refuses to start without it (orPOSTGRES_HOST_AUTH_METHOD=trust), with the error:Error: Database is uninitialized and superuser password is not specified. You must specify POSTGRES_PASSWORD to a non-empty value for the superuser. -
Version inconsistency — this example uses
postgres:18but theservices.mdreference example usespostgres:16. Pick one version and use it consistently across both files.
At minimum the db service should look like:
db:
image: postgres:17
environment:
POSTGRES_PASSWORD: example
POSTGRES_USER: myuser
POSTGRES_DB: mydb
healthcheck:
test: ["CMD-SHELL", "pg_isready -U $${POSTGRES_USER} -d $${POSTGRES_DB}"]
interval: 10s
retries: 5
start_period: 30s
timeout: 10s| - command: ["./manage.py", "loaddata", "fixtures.json"] | ||
|
|
||
| db: | ||
| image: postgres:18 |
There was a problem hiding this comment.
Same issue here: postgres:18 with no POSTGRES_PASSWORD — this db container won't start. Same version inconsistency vs services.md (postgres:16).
Description
As per docker/compose#13862 and compose-spec/compose-spec#647
Related issues or tickets
Reviews