introduce up --wait condition - #8777
Conversation
|
Wondering why this should not be the default for |
we hardly can change the existing behavior |
|
So, it currently already waits for things to be created, and started. Are there any cases where the Looking at available options; what does |
|
"service_started" indeed is the default condition.
First, service might not define a healthcheck. Other than that, this is just how things used to be, aka "Legacy" |
|
Do we need any unit tests, integration tests, E2E tests? |
For that case, we should consider "running" to be the equivalent of "healthcheck==ok". Healthcheck during startup is just a more customized "readiness" check. If there's no customized one, then "container running" means it's ready. Curious; what's the current behavior in this PR if I would pass a |
|
I don't think we should assume healthy == running, better detect container has no healthcheck configuration (by ContainerInspect.Config.Healthcheck) and report an error |
|
we actually already return error when service/docker image doesn't define a |
8f0605c to
e1e2d06
Compare
|
@ulyssessouza any thoughts? |
The point is that the general design for healthchecks was to (during startup) be a readiness check; in that design "no healthcheck" implicitly means "no readiness checck, so
So, what are the other options?
I'm personally still in favor of making this part of the standard behavior; we'e already printing the steps taken to bring up the stack, so it would be one more output to that list
The UX would be more logical for |
There's indeed no distinction in docker between "ready" and "healthy", but I think we can assume actual usages based on this. Better reject a --wait when no healthcheck is set than pretending to be clever.
Then this flag is not needed.
this is what's implemented here
yep. Basically, init container.
This is all about backward compatibilty, can't just change the behavior for a command used by thousands (millions?) because is looks niceer |
4bc573f to
5df5f77
Compare
I'm not sure what you mean with this "assume actual usages"
For both cases, the
I think we're waaaay past "backward compatiblity" when we did #8655. We could still use the
It's not "looking nicer"; I think the current behavior could be considered a bug; the expectation of a |
|
#8655 changes container names, but doesn't prevent most users to |
|
seems we hardly can find a consensus here. Here is my last proposal, reducing the scope: |
|
implemented the proposed "simpler" --wait boolean flag. Please reconsider |
09976f7 to
61d63cd
Compare
…r.yml. Checks - Add Django checks - Add PYTHONWARNINGS check - Remove diff-cover check Move build step to Docker workflow - Remove permissions section - Remove CI=True POSTGRES_HOST=127.0.0.1 DOMAIN_URL=http://127.0.0.1/api environment variables - Use --wait instead of sleep 60 docker/compose#8777 - Use default values for API_PREFIX, POSTGRES_DB, POSTGRES_USER, JOB_FILES_TIMEOUT - Move if-statement for SKIP_TEST Refactor test workflow - Move Transifex upload to new job - Remove coverage thresholds (defer to coveralls) - Remove CI=True DEBUG=True environment variables - Remove fetch-depth: 0 - Use default values for API_PREFIX, POSTGRES_DB, POSTGRES_USER - Change service ports - Change PostgreSQL credentials Remove pytest configuration from setup.cfg - Add --cov spoonbill_web - Allow auto-discovery of DJANGO_SETTINGS_MODULE - Use default for python_files (test_*.py) and testpaths (all) - Remove norecursedirs = .git Other changes - Delete .envrc - Remove ALLOWED_HOSTS from docker-compose.test.yaml - Change settings defaults to require fewer overrides: - API_PREFIX - CELERY_BACKEND - CELERY_BROKER - DB_HOST - POSTGRES_DB - POSTGRES_PASSWORD - POSTGRES_USER
The previous code would wait for dependencies to become healthy forever, even if they'd become unhealthy in the meantime. I can't find an issue report for this bug, but it was described in a comment on the PR that introduced the `--wait` flag [0]. [0]: docker#8777 (comment) Signed-off-by: Nikhil Benesch <nikhil.benesch@gmail.com>
|
How long does this wait? |
|
@noorul wait until dependent service reports a healthy state, as configured by HEALTHCHECK |
|
@ndeloof I think a timeout option will add value here instead of waiting forever. |
This enables starting lms services and the dev server by running `make services && make dev`, without encountering errors due to the DB not being ready. nb. The `--wait` flag is missing from the docs but see docker/compose#8777.
This enables starting apps with `make services dev` without potentially running into errors from the app due to services not being ready when the web server tries to connect. Services must define a healthcheck [1] in `docker-compose.yml` for this to work. The `--wait` flag is missing from the Docker Compose docs, but see docker/compose#8777. [1] https://docs.docker.com/compose/compose-file/05-services/#healthcheck
This enables starting apps with `make services dev` without potentially running into errors from the app due to services not being ready when the web server tries to connect. Services must define a healthcheck [1] in `docker-compose.yml` for this to work. The `--wait` flag is missing from the Docker Compose docs, but see docker/compose#8777. [1] https://docs.docker.com/compose/compose-file/05-services/#healthcheck
|
@ndeloof Thanks for the useful feature Do you have a proposal how to avoid the issue with init containers that exit with services:
db:
image: mysql:8.0
init-db:
image: mysql:8.0
command: ./init-db.sh
environment:
<<: *cenv
volumes:
- ./init-db.sh:./init-db.sh
depends_on:
db:
condition: service_starteddocker compose up -d --wait |
|
@nawa Had the same issue and took me a while to find a work around. I use the command in a github action, which fails when the init container exits. In case anyone else stumbles upon this, I separated it into two commands: docker compose up db -d --wait
docker compose up init-dbDoes anyone have a more elegant solution? |
docker/compose#8777, it's not in podman-compose and I'm not sure whether it's in docker.
docker/compose#8777, it's not in podman-compose but it is in docker-compose.
docker/compose#8777, it's not in podman-compose but it is in docker-compose.
Copilot reviewed PR #15 and raised nine findings. Eight were real and are fixed here; the ninth is rejected below. copytocontainer applied dirname to the SOURCE rather than the destination, so `copytocontainer vendor` ran `docker cp <project>/. <container>:/var/www/html/vendor` - the entire project root into the wrong path, followed by fixowns and fixperms over it. dirname now names the destination parent, which one expression handles for both a file and a directory, and that parent is created inside the container first because docker cp will not create it. copyfromcontainer's non-flag branch already had this right; it is the mirror of that. copyfromcontainer --realpath tested a CONTAINER path against the host filesystem, then built destinations like <project>/tmp//tmp/file whose parent was never created, so the normal case - a file that only exists in the container - always failed. Both branches were wrong, the -f one too for any nested path. Everything now lands flat in the project tmp folder, as --cachegrind and --traces already do, and the success line names where the file actually went. convert wrote ROLL_NO_STATIC_CACHING, a key no schema defines, so it only ever produced an unknown-key warning and had no effect. The supported key is positive-form, so the value has to be inverted rather than renamed: writing ROLL_MAGENTO_STATIC_CACHING=1 for a legacy NO_STATIC_CACHING=1 would have moved converted projects onto the production nginx template. Nothing is written when the legacy key is absent, since the schema default already means what the old forced value meant. utils/backup.sh carried the same rewrite in its legacy migration path and is fixed with it - Copilot did not flag that copy. Version-pin detection judged a pin on the resolved value, but global config is loaded into the same cache first, so a version in ~/.roll/.env made check-pins report success while the project file stayed unpinned and a colleague without that line resolved a different image - the exact failure the pin mechanism exists to prevent. loadConfigFromFile now records which file each key came from, pins are judged on the project's own keys, and the recommended value is the one being inherited rather than the legacy literal, so fix-pins still never changes which image comes up. DB_DISTRIBUTION_VERSION honours the older per-distribution spellings in both directions. Verified end to end: before, check-pins reported "Every enabled service already has its version pinned" with no PHP_VERSION line in the project file; after, it reports it missing and fix-pins writes the inherited 8.3, not the built-in 8.1. roll theme decided Gulp-vs-Yarn once for the whole project from whether any discovered theme was a boilerplate one, so a project holding both kinds ran `yarn run dev` against themes that only have a Gulp build. The choice is now made per theme, with ROLL_YARN_INSTEAD_OF_GULP still overriding in both directions. That override and ROLL_NODE_PACKAGE_MANAGER were registered in no schema, so setting either in .env.roll warned about an unknown key; both are now registered as optional, which keeps "unset means decide per theme" alive where a 0/1 default would not have. tableplus.help had two descriptions concatenated on the usage line, a regression from giving the command a @description header without removing the old text. doctor's search-engine write probe used a fixed index name and deleted it unconditionally, so a leftover from an interrupted run made the PUT fail as resource_already_exists and the check report a false negative. The name now carries the invocation's PID and the delete only runs when the write succeeded. The ShellCheck workflow ran test-syntax.sh and test-interact.sh through smoke.sh but listed neither in its path filters, so a PR touching only those skipped the workflow, and the lint step did not cover them either. Both filters and the lint scope now take .github/scripts. Rejected: the claim that `env up --wait` is unavailable across the supported Compose range because --wait needs 2.20.0. It was introduced in 2.1.1 (docker/compose#8777) and only --wait-timeout came later, in 2.17.0, so DOCKER_COMPOSE_REQUIRE=2.2.3 already covers it. No change made. Verified with the CI ShellCheck command (clean before and after), the smoke suite in both native and bash32 modes, the pin logic across four config scenarios plus the errexit path through bin/roll with every pin absent, and the convert translation across all four legacy values on bash 3.2.57. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
What I did
introduced
compose up --waitto run detached BUT wait for services to reach staterunningorhealthy(for those with a Healthcheck defined).Typical usage is to have some backend services (let's say postgres database) ran by compose, and user need to wait for service to be actually up so he can apply database migrations and start an happy coding day.
Related issue
#8351
(not mandatory) A picture of a cute animal, if possible in relation with what you did
