Fix deploy hangs and env_file word-splitting, on a new test harness - #22
Merged
Conversation
Introduce a bats suite for the two shell scripts, run in CI alongside shellcheck. Neither existed before, so every open shell bug in the tracker sits in code with no test and no lint covering it. The runner image is built FROM the action image on purpose: the scripts need GNU xargs from findutils, and against the busybox xargs in a plain alpine image the env_file tests fail for reasons unrelated to the code. Tests drive a fake docker CLI from fixtures, so no Swarm is needed. A test documenting an open bug calls skip with a link to the issue and asserts the desired behaviour, so fixing the bug means deleting the skip line rather than writing a new test. Issues 13, 19 and 21 each have one. scripts/stack-wait.sh is now maintained as a fork rather than tracking upstream, and its header says so. All shellcheck findings are resolved; where word splitting is intentional the line carries an explanatory disable instead of a change. One behaviour change: an unknown flag is now rejected instead of being silently ignored. Also adds a dependabot config covering both the workflows and the Dockerfile base image, and bumps actions/checkout to v7. Refs #13, #19, #21
The whole file was passed through an unquoted command substitution, so
the shell word-split it before export ran. Any value with a space in it
was truncated to its first token and the remainder was handed to export
as a separate argument, which then failed with a message naming the
second token:
export: `-Xmx1536m': not a valid identifier
No quoting style in the env file could work around it, because the split
happened after substitution. JVM options were the obvious casualty, but
connection strings and user agents hit it too.
Read one line at a time instead and pass export a single quoted
argument. Values are taken verbatim, matching `docker --env-file`, so
quotes in the file are part of the value rather than delimiters around
it. The SC2046 suppression that was hiding this is gone.
A line that is not NAME=VALUE is now rejected with a message naming the
offending line, rather than reaching export and producing one about
identifiers. This is also the error someone gets after passing a path
where the option wants the variables themselves.
Note that ENV_FILE carries no trailing newline, so the loop needs the
-n test to avoid dropping the last line.
Closes #21
Both open timeout reports come from the same place: the wait loop treats "fewer replicas running than wanted" as "still converging", which is wrong for any service that is not supposed to be running. A `mode: replicated-job` service that has finished reports its replicas as "0/1 (1/1 completed)". The completion count was cut away before anything looked at it, leaving current(0) != target(1) forever. Read the whole field and use the count: a job is settled once every requested run has completed, and reported as job_running until then. A service the stack file declares with `replicas: 0` is a different case, and indistinguishable from the one above by the replicas field alone -- there is no completion count to disambiguate a finished run from a service that failed to start. So consult the spec instead of the observed target. The two disagree exactly when this bites: stack deploy returns before the new spec is applied, and an external scheduler may have scaled the service since. The spec is re-read every poll, so a stale target corrects itself rather than being trusted once. A service whose spec really does ask for replicas still times out; that is covered by a test, because the shortcut above would be easy to widen into swallowing genuine failures. On either failure the task list is now dumped via docker stack ps. The per-service log only reports changes, so working out which service stalled previously meant reading back through the whole log. Not included: passing --detach=false to docker stack deploy, suggested on the second issue. It would make deploy block until convergence, which replaces this script's configurable timeout with docker's own waiting and could hang on precisely the services above. Worth its own discussion rather than folding in here. Closes #13 Closes #19
Adds the towncrier config, an empty CHANGELOG.md, the fragment template and a workflow that fails a pull request carrying no news fragment. News fragments for the three fixes on this branch come with it. Two things had to follow from it. The changelog workflow exempts a pull request labelled "skip changelog", and that label did not exist in the repository, so the escape hatch worked for nobody; it exists now. Dependabot cannot write a fragment for itself, so both of its update blocks carry that label, otherwise every dependency bump would arrive with a failing check. The generated CHANGELOG.md pointed contributors at Plone's contributing guide. It points at this repository's README instead, which now documents the fragment types, the naming convention and the label. Also excludes the changelog files from the docker build context, the way README.md and Makefile already are.
This was referenced Sep 4, 2026
Closed
davisagli
reviewed
Sep 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the three open deploy-hang /
env_filebugs, on top of a test harness the repository did not have.Closes #13
Closes #19
Closes #21
The fixes
#21 —
env_filevalues with spaces. The file went through an unquoted$(...), so the shell word-split it beforeexportran. Now read one line at a time, value taken verbatim to matchdocker --env-file. A line that is notNAME=VALUEis rejected naming that line, which also makes #3's failure legible without changing the option's semantics.#13 — completed
replicated-jobservices. A finished job reports0/1 (1/1 completed); the completion count wascutaway before anything read it, leavingcurrent != targetforever.#19 — services the stack file scales to zero. As @davisagli noted on the issue, this is indistinguishable from #13 by the replicas field alone. So it consults
.Spec.Mode.Replicated.Replicas— what the stack file asks for — instead of the observed target, re-read every poll so a stale target corrects itself. A service that genuinely wants replicas still times out, and there is a test for that. Both failure paths now dumpdocker stack ps --no-trunc, which @fredvd asked for.Test harness
Every bug here was in code with no test and no lint, so the first commit adds both. The runner image is built FROM the action image, not
bats/bats: the scripts need GNUxargsfromfindutils, and against busyboxxargstheenv_filetests fail for reasons unrelated to the code. Tests drive a fakedockerCLI from fixtures, so no Swarm is needed. Each bug test was committedskipped, then un-skipped by the commit that fixes it.Also:
stack-wait.shis now maintained as a fork of sudo-bmitch upstream rather than tracking it; towncrier for the change log with a CI check; a dependabot config.Worth a second opinion
stack-wait.shnow rejects unknown flags instead of ignoring them — a typo'd flag used to still deploy.env_filenow fails on a malformed line instead of exporting nothing and continuing.--detach=false, suggested on Dont' wait for services that are by default scaled to zero. #19. It makes deploy block until convergence, replacing the configurabledeploy_timeoutwith docker's own waiting, and could hang on precisely these services.Verification
make lintclean,make test43 passing / 0 skipped. Both fixes mutation-tested — removing either fails its own tests independently.make create-tagrun end-to-end in a sandboxed clone to confirm the release path.