fix: tolerate missing env file on scale, watch and shell completion - #13973
fix: tolerate missing env file on scale, watch and shell completion#13973glours wants to merge 1 commit into
Conversation
Follow-up to docker#13603: scale, watch and shell completion loaded the project without any tolerance option, so a missing env_file on a service not involved in the operation aborted the command, while up/exec/ps already tolerate this since docker#13156 and docker#13603. Mirror the WithServices pattern: load with WithoutEnvironmentResolution and resolve the environment once the project has been reduced to the selected services, so targeted services still get their env_file validated. Completion only needs names and never resolves. This also aligns the config hash of scale-created containers with up-created ones. Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The fix correctly mirrors the WithServices pattern already used in cmd/compose/compose.go, cmd/compose/run.go, and cmd/compose/config.go: load with cli.WithoutEnvironmentResolution first (so the project is filtered to the selected services), then call project.WithServicesEnvironmentResolved(true) on the reduced set. This ensures only the env_files of targeted services are validated. Shell completion correctly skips environment resolution entirely since only service names are needed. The completeScaleArgs rename from cli → dockerCli avoids a name collision with the newly imported package. One minor test-coverage gap noted inline.
Lower-confidence findings (not posted inline)
None.
| res.Assert(t, icmd.Expected{Out: "serviceA"}) | ||
| res.Assert(t, icmd.Expected{Out: "serviceB"}) | ||
|
|
||
| c.RunDockerComposeCmd(t, "-f", "./fixtures/env_file/compose.yaml", "down") |
There was a problem hiding this comment.
[low] No e2e test for watch tolerating missing env_file on unrelated service
The PR title and description explicitly name watch as one of the three commands fixed, and cmd/compose/watch.go gets the same WithoutEnvironmentResolution + WithServicesEnvironmentResolved treatment as scale. However, TestUnusedMissingEnvFile adds coverage for scale and shell completion but not for watch.
A minimal watch-specific check would be something like:
// watch should work even with missing env file on a service not being watched
// (requires --no-up to avoid a blocking daemon; or check exit code directly)
res = c.RunDockerComposeCmdNoCheck(t, "-f", "./fixtures/env_file/compose.yaml", "watch", "--no-up", "serviceA")
// watch starts a long-running daemon, so just assert it doesn't immediately fail
// with the env_file error
res.Assert(t, icmd.Expected{ExitCode: 0})Without this, a future regression (e.g. removing WithoutEnvironmentResolution from watch.go) would not be caught by the test suite.
| Confidence | Score |
|---|---|
| 🟢 strong | 100/100 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
2 participants
What I did
Follow-up to #13603: scale, watch and shell completion loaded the project without any tolerance option, so a missing env_file on a service not involved in the operation aborted the command, while up/exec/ps already tolerate this since #13156 and #13603.
Mirror the WithServices pattern: load with WithoutEnvironmentResolution and resolve the environment once the project has been reduced to the selected services, so targeted services still get their env_file validated. Completion only needs names and never resolves. This also aligns the config hash of scale-created containers with up-created ones.
Related issue
N/A
(not mandatory) A picture of a cute animal, if possible in relation to what you did
