Repository navigation
Conversation
|
Thank you for documenting the failures. I found a case that needs addressing: |
Thanks — and the ${FOO:-fallback} case is a real one. I traced it rather than patching the symptom, because the root cause changes the fix. Self-resolution. interpolatedEnvironment is environment.mapValues it resolves each value against the service's own environment: block. So for FOO: "${FOO:-fallback}" the lookup finds FOO already present, holding the unresolved literal, and substitutes that back into itself. The default never gets a chance. The correct source is the external environment (process env plus .env), which is what Compose interpolates against; a service's own declared value must not be visible to its own interpolation. Fixing the lookup source rather than adding a self-reference guard, since the guard would still leave A: "${B}" / B: "${A}" resolving against the wrong table. Discarded ${ overrides. I haven't reproduced this one yet — I'll build the regression first and let it tell me the shape before I touch the merge logic, rather than guessing at which overrides are affected. Coordination with #102. Will read it before writing anything. Same area, and I'd rather rebase onto or defer to that work than hand you two conflicting interpolation changes — tell me if you'd prefer #102 to land first and this PR to be rewritten on top. CPU rounding. Agreed it doesn't belong here; it's two commits (the rounding and its test seam) and I'll move them to their own PR. On the range check: Int(someDouble) in Swift traps on NaN, infinity and anything outside Int's range, so a hostile or fat-fingered cpus: is a crash rather than a bad value. The split-out PR will validate before converting and reject with a message naming the service. |
A compose file that selects its ingress network per environment writes
networks:
reverse-proxy:
name: ${INGRESS_NET:-local-ingress}
The service-level network reference is resolved at the run-args site, but the
top-level name was passed through raw, so container rejected it:
Creating network: reverse-proxy (Actual name: ${INGRESS_NET:-local-ingress})
Error: invalid network name: ${INGRESS_NET:-local-ingress}
environmentVariables is already an instance property here, so this was an
omission rather than a structural limit.
The resolution is extracted as a pure function because setupNetwork has no test
seam - the same reason healthcheck.timeout could be decoded, asserted on by two
parsing tests, and still never reach the runtime.
Third and fourth instance of the same shape as the network name. This project
resolves ${VAR} field by field rather than in one pass after parsing, so each
field is a separate chance to forget. An audit of ComposeUp.swift found 17 call
sites that resolve and these that did not: image, labels, container_name, user,
platform, entrypoint, command, volumes.
Measured against a real 23-service compose file, only three fields carried
variables: image (19 services), labels (2) and ports (7, already resolved). The
first two are fixed here; the rest are left alone rather than changed
speculatively, and the audit is in the PR description so the shape is visible.
Without this, `image: repo/name:${TAG:-1.0}` reaches container verbatim:
Pulling Image docker.io/lldap/lldap:${LLDAP_VERSION:-2026-05-26-alpine}...
Error: invalid format for image reference
The test suite is renamed to Compose Value Interpolation and documents which
fields are covered, so the next field is added to a list rather than to a
one-off test somewhere else.
It also pins a rule that was previously unguarded: resolveVariable gives the
process environment precedence over the map it is handed, which is the Compose
rule (a shell value beats a .env value). An earlier draft of these tests used a
variable name that happened to be exported and failed only on this machine;
without a test naming the rule, the obvious "fix" is to invert the merge and
silently break it.
The previous commit added the interpolation but tested it by calling resolveVariable from the test, which passes whether or not the call site uses it - mutation-checked: removing both call sites left all 8 tests green. That is the same vacuous shape this change set exists to remove, reproduced by the author one commit after arguing against it. Both mappings are now pure functions the tests call directly. Extracting labelRunArgs also brought two documented-but-unguarded behaviours under test: the compose project/service labels overriding a user label of the same key, and the argv being sorted for determinism.
`setupNetwork` creates a top-level network under the interpolated name
(`resolvedNetworkName`), but two consumers still took the declared
`name:` raw:
- the `--network` arg read `networks?[key]??.name` directly, so a
compose file declaring `name: ${INGRESS_NET:-dev-cluster-local-ingress}`
created `dev-cluster-local-ingress` and then asked `container run` to
join the literal `${INGRESS_NET:-dev-cluster-local-ingress}`, which
fails with "network ... not found".
- the host-gateway lookup interpolated the network *key* and ignored a
`name:` override, so `container network inspect` missed and it fell
back to the host's LAN default-route gateway. That path only warns,
so the wrong address reaches the container silently.
Both now go through `resolvedNetworkName`, the same mapping the create
path uses. Its doc comment already claimed the service-level reference
was interpolated "for the same reason"; that is now true.
The previous commit fixed both network-name consumers, but nothing would
have caught them regressing: every existing test calls
`resolvedNetworkName` directly, and those stayed green for as long as the
bug existed precisely because `configService` did not call it.
Extract the two decisions `configService` makes into pure functions —
`networkRunArgs` (every `--network` argument a service contributes) and
`gatewayNetworkName` (the network whose gateway `host-gateway` means) —
and test both through a decoded compose document that declares
`name: ${CC_TEST_INGRESS:-fallback-ingress}`.
913afe2 to
9d38406
Compare
|
Pushed, rebased onto current Service-environment interpolation is deferred to #102. I read it before touching anything, as promised. #102's combined[key] = resolveVariable(value, with: envVars) // #102 — resolves against the external envwhereas the commit that was here resolved against the service's own block: environment.mapValues { resolveVariable($0, with: environment) } // this PR — resolves against itselfThat second form is the The "earlier merge still discards some overrides containing CPU rounding is now #160. Two commits moved out, plus the range check you asked for. What's left here is the part neither #102 nor #160 touches — the values that reach
Five commits, down from eight. Static suite: 259 tests / 24 suites green. Say the word if you'd rather #102 land first and I rebase what's left on top of it — there's no overlap now, but the ordering is yours. |
2 participants
Summary
Compose variables were resolved for some values on the way to
container runand not others, so a compose file that parameterises its network names, image tags, labels or service environment got the literal${VAR}passed through to the daemon. Each is a separate one-line omission with the same shape, andresolveVariable— already in this repo and already used for ports — handles all of them.Found while bringing a 23-service compose file up on Apple Container; every failure below is one that file hit.
What's fixed
Top-level network
name:—setupNetworkusednetworkConfig?.name ?? networkNameraw, soname: ${INGRESS_NET:-local}reachedcontainer network create, which rejects it:Its two consumers — fixing the create path alone was a half-fix. The
--networkargument and thehost-gatewaylookup both still took.nameraw, so the network was created under the interpolated name and then joined by the literal one:The gateway lookup is the quieter of the two: a miss there only warns before falling back to the host's LAN default route, so the wrong address reaches the container silently.
Image references and label values —
image: foo:${TAG}andlabels: {v: "${VERSION}"}were passed through unresolved.Service
environment:—configServiceresolved these with its own parser: strip a leading${, drop the last character, look the remainder up. That matches only a value which is exactly${NAME}.${NAME:-default}and${NAME:?message}yielded a "variable name" carrying the whole modifier, missed the lookup and fell through; so did a reference with surrounding text, and a value holding two of them. An LDAP server started with the literal${LLDAP_BASE_DN:?LLDAP_BASE_DN is required}as its base DN and refused to bind.cpusrounding —container run --cpustakes a whole number;cpus: 0.5was passed through and rejected. Rounds up, and says so.Test seams
Three commits here are pure refactors, and they are the point rather than tidying.
The first version of the image and label fixes was tested by calling
resolveVariabledirectly, so removing both call sites left all eight tests green. The same trap caught the network fix later: every existing test calledresolvedNetworkNameitself, and stayed green for as long asconfigServicewas not calling it — which is exactly how the consumer bug survived.So each fix now has a pure function that owns the whole decision —
resolvedImageReference,labelRunArgs,networkRunArgs,gatewayNetworkName,interpolatedEnvironment,builderCPUCount— and the tests go through those. Verified by mutation: reverting each body to the old behaviour turns the intended tests red and only those.Verification
Independent of #144/#145 — no symbol from that work appears here, so this can be reviewed on its own.