Skip to content

Commit 90b23b0

Browse files
CaptainCarpensirCaptainCarpensir
authored
chore: prevent orchestrator stopTask from returning false errors (#5531)
Made a mistake in #5489 that causes an error message to falsely appear when stopping `run local` because it would call `docker inspect` when it only needed to call `docker ps`. This introduces another function that only runs the latter function, rather than running both, fixing the issue. By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.
1 parent 688b639 commit 90b23b0

File tree

6 files changed

+65
-30
lines changed

6 files changed

+65
-30
lines changed

‎internal/pkg/cli/interfaces.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -706,6 +706,7 @@ type templateDiffer interface {
706706
type dockerEngineRunner interface {
707707
CheckDockerEngineRunning() error
708708
Run(context.Context, *dockerengine.RunOptions) error
709+
DoesContainerExist(context.Context, string) (bool, error)
709710
IsContainerRunning(context.Context, string) (bool, error)
710711
Stop(context.Context, string) error
711712
Rm(string) error

‎internal/pkg/cli/mocks/mock_interfaces.go‎

Lines changed: 15 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎internal/pkg/docker/dockerengine/dockerengine.go‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -351,6 +351,15 @@ func (c DockerCmdClient) Run(ctx context.Context, options *RunOptions) error {
351351
return g.Wait()
352352
}
353353

354+
// DoesContainerExist checks if a specific Docker container exists.
355+
func (c DockerCmdClient) DoesContainerExist(ctx context.Context, name string) (bool, error) {
356+
output, err := c.containerID(ctx, name)
357+
if err != nil {
358+
return false, err
359+
}
360+
return output != "", nil
361+
}
362+
354363
// IsContainerRunning checks if a specific Docker container is running.
355364
func (c DockerCmdClient) IsContainerRunning(ctx context.Context, name string) (bool, error) {
356365
state, err := c.containerState(ctx, name)

‎internal/pkg/docker/dockerengine/dockerenginetest/dockerenginetest.go‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import (
1313
// Double is a test double for dockerengine.DockerCmdClient
1414
type Double struct {
1515
StopFn func(context.Context, string) error
16+
DoesContainerExistFn func(context.Context, string) (bool, error)
1617
IsContainerRunningFn func(context.Context, string) (bool, error)
1718
RunFn func(context.Context, *dockerengine.RunOptions) error
1819
BuildFn func(context.Context, *dockerengine.BuildArguments, io.Writer) error
@@ -27,6 +28,14 @@ func (d *Double) Stop(ctx context.Context, name string) error {
2728
return d.StopFn(ctx, name)
2829
}
2930

31+
// DoesContainerExist calls the stubbed function.
32+
func (d *Double) DoesContainerExist(ctx context.Context, name string) (bool, error) {
33+
if d.IsContainerRunningFn == nil {
34+
return false, nil
35+
}
36+
return d.DoesContainerExistFn(ctx, name)
37+
}
38+
3039
// IsContainerRunning calls the stubbed function.
3140
func (d *Double) IsContainerRunning(ctx context.Context, name string) (bool, error) {
3241
if d.IsContainerRunningFn == nil {

‎internal/pkg/docker/orchestrator/orchestrator.go‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@ type logOptionsFunc func(name string, ctr ContainerDefinition) dockerengine.RunL
4848
// DockerEngine is used by Orchestrator to manage containers.
4949
type DockerEngine interface {
5050
Run(context.Context, *dockerengine.RunOptions) error
51+
DoesContainerExist(context.Context, string) (bool, error)
5152
IsContainerRunning(context.Context, string) (bool, error)
5253
Stop(context.Context, string) error
5354
Build(ctx context.Context, args *dockerengine.BuildArguments, w io.Writer) error
@@ -398,13 +399,13 @@ func (o *Orchestrator) stopTask(ctx context.Context, task Task) error {
398399

399400
// ensure that container is fully stopped before stopTask finishes blocking
400401
for {
401-
running, err := o.docker.IsContainerRunning(ctx, o.containerID(name))
402+
exists, err := o.docker.DoesContainerExist(ctx, o.containerID(name))
402403
if err != nil {
403404
errCh <- fmt.Errorf("polling container %q for removal: %w", name, err)
404405
return
405406
}
406407

407-
if running {
408+
if exists {
408409
select {
409410
case <-time.After(1 * time.Second):
410411
continue

‎internal/pkg/docker/orchestrator/orchestrator_test.go‎

Lines changed: 28 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -85,12 +85,12 @@ func TestOrchestrator(t *testing.T) {
8585
runUntilStopped: true,
8686
test: func(t *testing.T) (test, *dockerenginetest.Double) {
8787
de := &dockerenginetest.Double{
88-
IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) {
89-
if name == "prefix-pause" {
90-
return true, nil
91-
}
88+
DoesContainerExistFn: func(ctx context.Context, s string) (bool, error) {
9289
return false, nil
9390
},
91+
IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) {
92+
return true, nil
93+
},
9494
StopFn: func(ctx context.Context, name string) error {
9595
if name == "prefix-success" {
9696
return nil
@@ -119,12 +119,12 @@ func TestOrchestrator(t *testing.T) {
119119
runUntilStopped: true,
120120
test: func(t *testing.T) (test, *dockerenginetest.Double) {
121121
de := &dockerenginetest.Double{
122-
IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) {
123-
if name == "prefix-pause" {
124-
return true, nil
125-
}
122+
DoesContainerExistFn: func(ctx context.Context, s string) (bool, error) {
126123
return false, errors.New("some error")
127124
},
125+
IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) {
126+
return true, nil
127+
},
128128
StopFn: func(ctx context.Context, name string) error {
129129
return nil
130130
},
@@ -148,12 +148,12 @@ func TestOrchestrator(t *testing.T) {
148148
runUntilStopped: true,
149149
test: func(t *testing.T) (test, *dockerenginetest.Double) {
150150
de := &dockerenginetest.Double{
151-
IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) {
152-
if name == "prefix-pause" {
153-
return true, nil
154-
}
151+
DoesContainerExistFn: func(ctx context.Context, s string) (bool, error) {
155152
return false, nil
156153
},
154+
IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) {
155+
return true, nil
156+
},
157157
}
158158
return func(t *testing.T, o *Orchestrator) {
159159
o.RunTask(Task{
@@ -185,12 +185,12 @@ func TestOrchestrator(t *testing.T) {
185185
runUntilStopped: true,
186186
test: func(t *testing.T) (test, *dockerenginetest.Double) {
187187
de := &dockerenginetest.Double{
188-
IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) {
189-
if name == "prefix-pause" {
190-
return true, nil
191-
}
188+
DoesContainerExistFn: func(ctx context.Context, s string) (bool, error) {
192189
return false, nil
193190
},
191+
IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) {
192+
return true, nil
193+
},
194194
RunFn: func(ctx context.Context, opts *dockerengine.RunOptions) error {
195195
// validate pause container has correct ports and secrets
196196
if opts.ContainerName == "prefix-pause" {
@@ -234,12 +234,12 @@ func TestOrchestrator(t *testing.T) {
234234
test: func(t *testing.T) (test, *dockerenginetest.Double) {
235235
stopPause := make(chan struct{})
236236
de := &dockerenginetest.Double{
237-
IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) {
238-
if name == "prefix-pause" {
239-
return true, nil
240-
}
237+
DoesContainerExistFn: func(ctx context.Context, s string) (bool, error) {
241238
return false, nil
242239
},
240+
IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) {
241+
return true, nil
242+
},
243243
RunFn: func(ctx context.Context, opts *dockerengine.RunOptions) error {
244244
if opts.ContainerName == "prefix-foo" {
245245
return errors.New("some error")
@@ -272,12 +272,12 @@ func TestOrchestrator(t *testing.T) {
272272
test: func(t *testing.T) (test, *dockerenginetest.Double) {
273273
stopPause := make(chan struct{})
274274
de := &dockerenginetest.Double{
275-
IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) {
276-
if name == "prefix-pause" {
277-
return true, nil
278-
}
275+
DoesContainerExistFn: func(ctx context.Context, s string) (bool, error) {
279276
return false, nil
280277
},
278+
IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) {
279+
return true, nil
280+
},
281281
RunFn: func(ctx context.Context, opts *dockerengine.RunOptions) error {
282282
if opts.ContainerName == "prefix-foo" {
283283
return nil
@@ -420,12 +420,12 @@ func TestOrchestrator(t *testing.T) {
420420
runUntilStopped: true,
421421
test: func(t *testing.T) (test, *dockerenginetest.Double) {
422422
de := &dockerenginetest.Double{
423-
IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) {
424-
if name == "prefix-pause" {
425-
return true, nil
426-
}
423+
DoesContainerExistFn: func(ctx context.Context, s string) (bool, error) {
427424
return false, nil
428425
},
426+
IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) {
427+
return true, nil
428+
},
429429
ExecFn: func(ctx context.Context, ctr string, w io.Writer, cmd string, args ...string) error {
430430
if cmd == "aws" {
431431
fmt.Fprintf(w, "Port 61972 opened for sessionId mySessionId\n")

0 commit comments

Comments
 (0)