cli: one project-resolution story — single name precedence, no swallo… · docker/compose@5c16378 · GitHub
Skip to content

Commit 5c16378

Browse files
committed
cli: one project-resolution story — single name precedence, no swallowed load error, strict service validation
projectOrName and toProjectName resolved the project with opposite precedences, and projectOrName silently swallowed any load error when COMPOSE_PROJECT_NAME was set: a broken compose file sent stop, down, ps... into label-based reconstruction without a word — even when the file was named explicitly with --file. One precedence now, documented on both resolvers and applied identically by compose-go while loading: --project-name, then COMPOSE_PROJECT_NAME, then the model's name. The failure policy becomes explicit: an unreadable explicit --file is a hard error; no file around with COMPOSE_PROJECT_NAME set stays the silent file-less workflow; a present-but-broken implicit file falls back to label-based mode with a warning. Service-name validation follows one rule — strict whenever a model is available: restart and wait no longer silently no-op on a typo (validateServiceNames, profile-disabled services remain legitimate targets), and the hand-rolled checks in ps and volumes are removed as dead code, the load-time selection already rejecting unknown names (pinned by test). Epic #14074, F.4. Rebased onto main, which since merged the jobs work (#14093, #14234): projectOrName's job-target detection (jobTargetErr) is restored ahead of the new explicit-file hard-error branch -- the file loaded fine here, only the target's selection failed -- and validateServiceNames now checks project.AllJobs() too, since restart/wait route their service arguments through it instead of projectOrName's own selection. docker-agent review: the "compose file found but could not be loaded" warning's suppression guard only matched errdefs.IsNotFoundError (compose-go's own ErrNotFound sentinel) -- a raw os.ErrNotExist (e.g. a nonexistent --project-directory) wasn't recognized and would have printed a misleading warning. Added errors.Is(err, os.ErrNotExist) as a fallback, and TestProjectOrNameResolution now asserts the warning's presence/absence in both directions instead of just the fallback name. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
1 parent 4e1cd3f commit 5c16378

9 files changed

Lines changed: 276 additions & 20 deletions

File tree

‎cmd/compose/compose.go‎

Lines changed: 61 additions & 1 deletion
Lines changed: 163 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,163 @@
1+
/*
2+
Copyright 2020 Docker Compose CLI authors
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package compose
18+
19+
import (
20+
"os"
21+
"path/filepath"
22+
"strings"
23+
"testing"
24+
25+
"github.com/compose-spec/compose-go/v2/types"
26+
"github.com/docker/cli/cli/streams"
27+
logrustest "github.com/sirupsen/logrus/hooks/test"
28+
"go.uber.org/mock/gomock"
29+
"gotest.tools/v3/assert"
30+
31+
"github.com/docker/compose/v5/pkg/mocks"
32+
)
33+
34+
func projectDir(t *testing.T, composeContent string) string {
35+
t.Helper()
36+
dir := t.TempDir()
37+
if composeContent != "" {
38+
assert.NilError(t, os.WriteFile(filepath.Join(dir, "compose.yaml"), []byte(composeContent), 0o600))
39+
}
40+
return dir
41+
}
42+
43+
func resolutionCli(t *testing.T) *mocks.MockCli {
44+
t.Helper()
45+
ctrl := gomock.NewController(t)
46+
cli := mocks.NewMockCli(ctrl)
47+
cli.EXPECT().Out().Return(streams.NewOut(os.Stdout)).AnyTimes()
48+
cli.EXPECT().Err().Return(streams.NewOut(os.Stderr)).AnyTimes()
49+
return cli
50+
}
51+
52+
const validCompose = "services:\n web:\n image: alpine\n gated:\n image: alpine\n profiles: [debug]\n"
53+
54+
// The resolution matrix of projectOrName: one name precedence
55+
// (--project-name, then COMPOSE_PROJECT_NAME, then the model), a hard error
56+
// for an explicit --file that cannot be read, and a label-based fallback
57+
// only when COMPOSE_PROJECT_NAME provides a name.
58+
func TestProjectOrNameResolution(t *testing.T) {
59+
unsetEnv := func(t *testing.T) {
60+
t.Setenv(ComposeProjectName, "")
61+
assert.NilError(t, os.Unsetenv(ComposeProjectName))
62+
}
63+
64+
t.Run("broken implicit file falls back to COMPOSE_PROJECT_NAME", func(t *testing.T) {
65+
t.Setenv(ComposeProjectName, "fallback")
66+
dir := projectDir(t, "services: {invalid")
67+
opts := ProjectOptions{ProjectDir: dir}
68+
hook := logrustest.NewGlobal()
69+
70+
project, name, err := opts.projectOrName(t.Context(), resolutionCli(t))
71+
assert.NilError(t, err)
72+
assert.Equal(t, name, "fallback")
73+
assert.Assert(t, project == nil)
74+
assert.Equal(t, len(hook.AllEntries()), 1)
75+
assert.Assert(t, strings.Contains(hook.LastEntry().Message, "falling back to label-based mode"), hook.LastEntry().Message)
76+
})
77+
78+
t.Run("broken explicit --file is a hard error even with COMPOSE_PROJECT_NAME", func(t *testing.T) {
79+
t.Setenv(ComposeProjectName, "fallback")
80+
dir := projectDir(t, "services: {invalid")
81+
opts := ProjectOptions{ConfigPaths: []string{filepath.Join(dir, "compose.yaml")}, ProjectDir: dir}
82+
83+
_, _, err := opts.projectOrName(t.Context(), resolutionCli(t))
84+
assert.ErrorContains(t, err, "yaml")
85+
})
86+
87+
t.Run("no file at all with COMPOSE_PROJECT_NAME is the file-less workflow", func(t *testing.T) {
88+
t.Setenv(ComposeProjectName, "labels-only")
89+
dir := projectDir(t, "")
90+
opts := ProjectOptions{ProjectDir: dir}
91+
hook := logrustest.NewGlobal()
92+
93+
project, name, err := opts.projectOrName(t.Context(), resolutionCli(t))
94+
assert.NilError(t, err)
95+
assert.Equal(t, name, "labels-only")
96+
assert.Assert(t, project == nil)
97+
// no file exists at all, so the "found but could not be loaded"
98+
// warning would be factually wrong here — this is the normal
99+
// file-less workflow, not a fallback from a broken file.
100+
assert.Equal(t, len(hook.AllEntries()), 0, hook.AllEntries())
101+
})
102+
103+
t.Run("no file and no name errors", func(t *testing.T) {
104+
unsetEnv(t)
105+
dir := projectDir(t, "")
106+
opts := ProjectOptions{ProjectDir: dir}
107+
108+
_, _, err := opts.projectOrName(t.Context(), resolutionCli(t))
109+
assert.Assert(t, err != nil)
110+
})
111+
112+
t.Run("COMPOSE_PROJECT_NAME overrides the loaded model's name", func(t *testing.T) {
113+
t.Setenv(ComposeProjectName, "from-env")
114+
dir := projectDir(t, validCompose)
115+
opts := ProjectOptions{ProjectDir: dir}
116+
117+
project, name, err := opts.projectOrName(t.Context(), resolutionCli(t))
118+
assert.NilError(t, err)
119+
assert.Equal(t, name, "from-env")
120+
assert.Assert(t, project != nil)
121+
})
122+
123+
t.Run("--project-name without --file skips loading, even a broken file", func(t *testing.T) {
124+
unsetEnv(t)
125+
dir := projectDir(t, "services: {invalid")
126+
opts := ProjectOptions{ProjectName: "explicit", ProjectDir: dir}
127+
128+
project, name, err := opts.projectOrName(t.Context(), resolutionCli(t))
129+
assert.NilError(t, err)
130+
assert.Equal(t, name, "explicit")
131+
assert.Assert(t, project == nil)
132+
})
133+
134+
t.Run("unknown requested service is rejected by the load itself", func(t *testing.T) {
135+
unsetEnv(t)
136+
dir := projectDir(t, validCompose)
137+
opts := ProjectOptions{ProjectDir: dir}
138+
139+
_, _, err := opts.projectOrName(t.Context(), resolutionCli(t), "typo")
140+
assert.ErrorContains(t, err, "no such service")
141+
})
142+
}
143+
144+
// validateServiceNames backs the commands that don't pass their service
145+
// arguments through the load-time selection (restart, wait): strict when a
146+
// model is available, no-op in label-based mode.
147+
func TestValidateServiceNames(t *testing.T) {
148+
project := &types.Project{
149+
Services: types.Services{"web": {Name: "web"}},
150+
DisabledServices: types.Services{"gated": {Name: "gated"}},
151+
Jobs: types.Jobs{"migrate": {Name: "migrate"}},
152+
}
153+
154+
assert.NilError(t, validateServiceNames(nil, []string{"anything"}))
155+
assert.NilError(t, validateServiceNames(project, []string{"web"}))
156+
// profile-disabled services are legitimate targets: restart enables them
157+
assert.NilError(t, validateServiceNames(project, []string{"gated"}))
158+
assert.Error(t, validateServiceNames(project, []string{"typo"}), "no such service: typo")
159+
// restart/wait pass no services to projectOrName, so a job target never
160+
// reaches jobTargetErr there -- validateServiceNames must recognize it
161+
// itself instead of falling through to the generic "no such service".
162+
assert.Error(t, validateServiceNames(project, []string{"migrate"}), `job "migrate" can only be triggered with "docker compose run"`)
163+
}

‎cmd/compose/ps.go‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -99,14 +99,10 @@ func runPs(ctx context.Context, dockerCli command.Cli, backendOptions *BackendOp
9999
}
100100

101101
if project != nil {
102+
// unknown requested services were already rejected while loading the
103+
// project (service selection in ToProject)
102104
names := project.ServiceNames()
103-
if len(services) > 0 {
104-
for _, service := range services {
105-
if !slices.Contains(names, service) {
106-
return fmt.Errorf("no such service: %s", service)
107-
}
108-
}
109-
} else if !opts.Orphans {
105+
if len(services) == 0 && !opts.Orphans {
110106
// until user asks to list orphaned services, we only include those declared in project
111107
services = names
112108
}

‎cmd/compose/restart.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,9 @@ func runRestart(ctx context.Context, dockerCli command.Cli, backendOptions *Back
5959
if err != nil {
6060
return err
6161
}
62+
if err := validateServiceNames(project, services); err != nil {
63+
return err
64+
}
6265

6366
if project != nil && len(services) > 0 {
6467
project, err = project.WithServicesEnabled(services...)

‎cmd/compose/volumes.go‎

Lines changed: 4 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ package compose
1919
import (
2020
"context"
2121
"fmt"
22-
"slices"
2322

2423
"github.com/docker/cli/cli/command"
2524
"github.com/docker/cli/cli/command/formatter"
@@ -57,20 +56,14 @@ func volumesCommand(p *ProjectOptions, dockerCli command.Cli, backendOptions *Ba
5756
}
5857

5958
func runVol(ctx context.Context, dockerCli command.Cli, backendOptions *BackendOptions, services []string, options volumesOptions) error {
60-
project, name, err := options.projectOrName(ctx, dockerCli, services...)
59+
// unknown requested services are rejected while loading the project
60+
// (service selection in ToProject); label-based mode has no model to
61+
// validate against
62+
_, name, err := options.projectOrName(ctx, dockerCli, services...)
6163
if err != nil {
6264
return err
6365
}
6466

65-
if project != nil {
66-
names := project.ServiceNames()
67-
for _, service := range services {
68-
if !slices.Contains(names, service) {
69-
return fmt.Errorf("no such service: %s", service)
70-
}
71-
}
72-
}
73-
7467
backend, err := compose.NewComposeService(dockerCli, backendOptions.Options...)
7568
if err != nil {
7669
return err

‎cmd/compose/wait.go‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,10 +63,13 @@ func waitCommand(p *ProjectOptions, dockerCli command.Cli, backendOptions *Backe
6363
}
6464

6565
func runWait(ctx context.Context, dockerCli command.Cli, backendOptions *BackendOptions, opts *waitOptions) (int64, error) {
66-
_, name, err := opts.projectOrName(ctx, dockerCli)
66+
project, name, err := opts.projectOrName(ctx, dockerCli)
6767
if err != nil {
6868
return 0, err
6969
}
70+
if err := validateServiceNames(project, opts.services); err != nil {
71+
return 0, err
72+
}
7073

7174
backend, err := compose.NewComposeService(dockerCli, backendOptions.Options...)
7275
if err != nil {

‎pkg/e2e/jobs_test.go‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -141,3 +141,21 @@ func TestDownRefusesJobWithProjectNameEnv(t *testing.T) {
141141
StderrContains(`job "migrate" can only be triggered with "docker compose run"`),
142142
ServiceNotCreated("migrate"))
143143
}
144+
145+
// restart and wait don't pass their service arguments to projectOrName (see
146+
// runRestart/runWait), so jobTargetErr never sees them there -- the refusal
147+
// for these two instead comes from validateServiceNames itself recognizing
148+
// project.AllJobs(). A separate code path, so it needs its own coverage.
149+
func TestRestartRefusesJob(t *testing.T) {
150+
NewScenario(t, "restart must refuse a job by name, naming run as the right command").
151+
Step("restart fails naming the job",
152+
ComposeCmd("restart", "migrate").MayFail(),
153+
StderrContains(`job "migrate" can only be triggered with "docker compose run"`))
154+
}
155+
156+
func TestWaitRefusesJob(t *testing.T) {
157+
NewScenario(t, "wait must refuse a job by name, naming run as the right command").
158+
Step("wait fails naming the job",
159+
ComposeCmd("wait", "migrate").MayFail(),
160+
StderrContains(`job "migrate" can only be triggered with "docker compose run"`))
161+
}
Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
services:
2+
web:
3+
image: alpine
4+
command: sleep infinity
5+
jobs:
6+
migrate:
7+
image: alpine
8+
command: sh -c 'echo "migration done"'
9+
triggers:
10+
manual: true
Lines changed: 10 additions & 0 deletions

0 commit comments

Comments
 (0)