Repository navigation
Conversation
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
docker-agent
left a comment
There was a problem hiding this comment.
This review covers only the commits pushed since a7cd4a2.
Assessment: 🟢 APPROVE
The incremental diff fixes TestResolveMaxConcurrency exactly as the prior resolved review thread requested: the test now reads the cobra-bound --parallel value via root.Flags().GetInt("parallel") and passes it to resolveMaxConcurrency, rather than the hardcoded 4. The three test-case changes follow directly from that fix:
- "default without environment" (new case): no args, no env →
parallelis-1(the flag default),want: -1. Correct. - "flag value without environment":
--parallel 2→parallel = 2,want: 2. Correct. - "flag wins over environment":
--parallel 2+ env8→ flag wins,parallel = 2,want: 2. Correct (was previouslywant: 4, a hardcoded value that gave false confidence).
No production code is changed. No bugs introduced.
9dbba35 to
46d350b
Compare
validateFlags, the recreate strategies, createOptions.Apply and upOptions.apply encode the incompatible flag combinations of up/create and had no tests. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Pin the mapping from flags to project/build options and the env/flag precedence rules (COMPOSE_ANSI, COMPOSE_PARALLEL_LIMIT, --workdir). Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
46d350b to
6d3ca02
Compare
glours
left a comment
There was a problem hiding this comment.
Changes requested on the ansi row (see inline, cli_helpers_test.go:346); the rest is small.
The description says "the incompatible-flag matrix of up and create" is asserted, but only validateFlags is exercised. The inline checks in create.go:67-72 (--build/--no-build, --force-recreate/--no-recreate), run.go:175 (--service-ports/--publish) and the flag→field bindings (e.g. create.pullChanged = …Changed("pull"), up.go:123) aren't pinned: removing any of them keeps the suite green. Narrow the description to validateFlags, or add a daemon-free test on the real commands (upCommand(...), ParseFlags, PreRunE).
Nits:
- cli_helpers_test.go:198-204 "publish is ignored with service-ports": run's PreRunE (run.go:175) rejects that combination before
apply. The row only pins theapplyguard (run.go:96), name it that way. - cli_helpers_test.go:111
filterByStatus(nil): unreachable,runPsguards onlen(opts.Status) != 0. Pins incidental behaviour. - cli_helpers_test.go:220-222, up_flags_test.go:185, :308-309: assertions on compose-go /
strconvwording break on a dependency bump.ErrorContainson the offending input is enough. - cli_helpers_test.go:158 "by default" uses
interactive=false, the flag default istrue(run.go:252). :435 "unset" sets the var to empty. up_flags_test.go:83 should name--renew-anon-volumes, notno-inherit. - build/run/ps/scale tests belong in
build_test.go/run_test.go(one test file per production file).
| } else { | ||
| t.Setenv("COMPOSE_ANSI", *tt.env) | ||
| } | ||
| got, err := resolveAnsiMode(commandWithFlags(t, "compose", tt.args...), tt.ansi, tt.noAnsi) |
There was a problem hiding this comment.
--ansi is registered on the root only (compose.go:603) and resolveAnsiMode is called with the executed leaf (compose.go:502), so cmd.Flags().Changed("ansi") (compose.go:638) is false there. Passing the root here makes "flag wins over environment" true by construction. In the real command, COMPOSE_ANSI=always docker compose --ansi never ps resolves always.
TestResolveMaxConcurrency already models root + sub (:378-383):
| got, err := resolveAnsiMode(commandWithFlags(t, "compose", tt.args...), tt.ansi, tt.noAnsi) | |
| root := commandWithFlags(t, PluginName, tt.args...) | |
| sub := &cobra.Command{Use: "up"} | |
| root.AddCommand(sub) | |
| got, err := resolveAnsiMode(sub, tt.ansi, tt.noAnsi) |
With this, "flag wins over environment" fails today (got never, want always) because of the production bug, not the test. Either fix compose.go:638 here (look up Changed("ansi") on the compose root, as resolveMaxConcurrency does, which drops the "no production change" claim), or drop the row and fix separately. Merging the row as is pins a guarantee the code doesn't give.
| // unsetenv removes key for the duration of the test, restoring it afterwards | ||
| func unsetenv(t *testing.T, key string) error { | ||
| t.Helper() | ||
| t.Setenv(key, "") | ||
| return os.Unsetenv(key) | ||
| } |
There was a problem hiding this comment.
Duplicates unsetEnv (hooks_test.go:306, same package, name differs only by case). That one is LookupEnv-based and its comment explains why t.Setenv(k, "") isn't unset. Delete this helper:
| // unsetenv removes key for the duration of the test, restoring it afterwards | |
| func unsetenv(t *testing.T, key string) error { | |
| t.Helper() | |
| t.Setenv(key, "") | |
| return os.Unsetenv(key) | |
| } |
Then at :342 and :374 call unsetEnv(t, "COMPOSE_ANSI") / unsetEnv(t, ComposeParallelLimit) directly (no assert.NilError wrapper, it returns nothing), and drop the os import at :20, which this helper was the only user of.
| opts := buildOptions{ | ||
| ProjectOptions: &ProjectOptions{}, | ||
| pull: true, | ||
| push: true, | ||
| noCache: true, | ||
| quiet: true, | ||
| deps: true, | ||
| print: true, | ||
| check: true, | ||
| args: []string{"FOO=bar", "BAZ"}, | ||
| sbom: "true", | ||
| provenance: "mode=max", | ||
| } | ||
| got, err := opts.toAPIBuildOptions([]string{"web"}) | ||
| assert.NilError(t, err) | ||
| assert.DeepEqual(t, got.Services, []string{"web"}) | ||
| assert.Assert(t, got.Pull && got.Push && got.NoCache && got.Quiet && got.Deps && got.Print && got.Check) |
There was a problem hiding this comment.
Every flag is true and they're asserted as a conjunction, so cross-wiring in toAPIBuildOptions passes: swapping Pull/Push, NoCache/Quiet, or Deps<-print all survive. memory is never set, so dropping Memory survives too. Distinct values, one assertion per field:
| opts := buildOptions{ | |
| ProjectOptions: &ProjectOptions{}, | |
| pull: true, | |
| push: true, | |
| noCache: true, | |
| quiet: true, | |
| deps: true, | |
| print: true, | |
| check: true, | |
| args: []string{"FOO=bar", "BAZ"}, | |
| sbom: "true", | |
| provenance: "mode=max", | |
| } | |
| got, err := opts.toAPIBuildOptions([]string{"web"}) | |
| assert.NilError(t, err) | |
| assert.DeepEqual(t, got.Services, []string{"web"}) | |
| assert.Assert(t, got.Pull && got.Push && got.NoCache && got.Quiet && got.Deps && got.Print && got.Check) | |
| opts := buildOptions{ | |
| ProjectOptions: &ProjectOptions{}, | |
| pull: true, | |
| quiet: true, | |
| print: true, | |
| args: []string{"FOO=bar", "BAZ"}, | |
| sbom: "true", | |
| provenance: "mode=max", | |
| } | |
| got, err := opts.toAPIBuildOptions([]string{"web"}) | |
| assert.NilError(t, err) | |
| assert.DeepEqual(t, got.Services, []string{"web"}) | |
| assert.Equal(t, got.Pull, true) | |
| assert.Equal(t, got.Push, false) | |
| assert.Equal(t, got.NoCache, false) | |
| assert.Equal(t, got.Quiet, true) | |
| assert.Equal(t, got.Deps, false) | |
| assert.Equal(t, got.Print, true) | |
| assert.Equal(t, got.Check, false) |
Also set memory to a non-zero cliopts.MemBytes and assert got.Memory (needs the cliopts import used in build.go).
| name: "detach with watch", | ||
| up: upOptions{Detach: true, watch: true}, | ||
| wantErr: "--detach cannot be combined with --abort-on-container-exit, --abort-on-container-failure, --attach, --attach-dependencies or --watch", | ||
| }, |
There was a problem hiding this comment.
The --detach condition (up.go:203) has five terms, only attachDependencies, cascadeFail and watch are pinned. Dropping len(up.attach) > 0 or up.cascadeStop survives. The --wait variant of the message (up.go:204-205) is unreachable from this table: the only way in is wait + cascadeFail/watch (wait sets Detach at :198, and the check at :195 doesn't list them). Four rows, all pass on base:
| }, | |
| }, | |
| { | |
| name: "detach with attach", | |
| up: upOptions{Detach: true, attach: []string{"web"}}, | |
| wantErr: "--detach cannot be combined with --abort-on-container-exit, --abort-on-container-failure, --attach, --attach-dependencies or --watch", | |
| }, | |
| { | |
| name: "detach with abort-on-container-exit", | |
| up: upOptions{Detach: true, cascadeStop: true}, | |
| wantErr: "--detach cannot be combined with --abort-on-container-exit, --abort-on-container-failure, --attach, --attach-dependencies or --watch", | |
| }, | |
| { | |
| name: "wait with abort-on-container-failure", | |
| up: upOptions{wait: true, cascadeFail: true}, | |
| wantErr: "--wait cannot be combined with --abort-on-container-exit, --abort-on-container-failure, --attach, --attach-dependencies or --watch", | |
| }, | |
| { | |
| name: "wait with watch", | |
| up: upOptions{wait: true, watch: true}, | |
| wantErr: "--wait cannot be combined with --abort-on-container-exit, --abort-on-container-failure, --attach, --attach-dependencies or --watch", | |
| }, |
This PR adds table-driven unit tests that pin the flag validation and flag-to-option mapping of the
composeCLI, without needing a Docker daemon.Context
cmd/composesits at roughly a quarter of statement coverage, yet it holds the user-facing flag logic: which combinations are rejected, how flags and environment variables take precedence, and how flags end up in project and build options. That logic is easy to break by accident during refactors and cheap to test, since it is pure and never touches the engine.What the PR brings
The incompatible-flag matrix of
upandcreate(--wait,--detach,--abort-on-container-*,--build/--no-build, the--no-recreatefamily,--watch) is now asserted case by case with its exact error message, along with the implications between flags (--exit-code-fromimplies abort-on-exit,--waitimplies detach). The recreate strategies,--pullpolicy handling,--scaleparsing,up --no-deps/--exit-code-fromand therunoption mapping (tty/stdin,--service-portsversus--publish, volumes,--no-deps) are covered too.Around them, the PR pins the
scaleargument parser,ps --filterparsing and status filtering, the legacy flag-name normalization ofrun, the--ssh/BUILDX_BUILDERhandling ofbuild, and the root-level precedence rules (--ansivs--no-ansivsCOMPOSE_ANSI,--parallelvsCOMPOSE_PARALLEL_LIMIT, deprecated--workdir, env-file path normalization).Guardrails: tests only, no production change, no Docker daemon and no mocks of the API.
cmd/compatibilitywas already at 100% and is untouched.AI disclosure: prepared with GitHub Copilot CLI, orchestrated by Claude Code, on behalf of and under the direction of the submitter.