Merge pull request moby#51312 from austinvazquez/refactor-client-container-wait

client: refactor ContainerWait to use client defined options/results structs

Author: thaJeztah

client/client_interfaces.go

@@ -76,7 +76,7 @@ type ContainerAPIClient interface {
 	ContainerTop(ctx context.Context, container string, options ContainerTopOptions) (ContainerTopResult, error)
 	ContainerUnpause(ctx context.Context, container string, options ContainerUnPauseOptions) (ContainerUnPauseResult, error)
 	ContainerUpdate(ctx context.Context, container string, updateConfig container.UpdateConfig) (container.UpdateResponse, error)
-	ContainerWait(ctx context.Context, container string, condition container.WaitCondition) (<-chan container.WaitResponse, <-chan error)
+	ContainerWait(ctx context.Context, container string, options ContainerWaitOptions) ContainerWaitResult
 	CopyFromContainer(ctx context.Context, container, srcPath string) (io.ReadCloser, container.PathStat, error)
 	CopyToContainer(ctx context.Context, container, path string, content io.Reader, options CopyToContainerOptions) error
 	ContainersPrune(ctx context.Context, opts ContainerPruneOptions) (ContainerPruneResult, error)

client/container_wait.go

@@ -13,6 +13,17 @@ import (
 
 const containerWaitErrorMsgLimit = 2 * 1024 /* Max: 2KiB */
 
+// ContainerWaitOptions holds options for container wait operations.
+type ContainerWaitOptions struct {
+	Condition container.WaitCondition
+}
+
+// ContainerWaitResult defines the result of a container wait operation.
+type ContainerWaitResult struct {
+	Results <-chan container.WaitResponse
+	Errors  <-chan error
+}
+
 // ContainerWait waits until the specified container is in a certain state
 // indicated by the given condition, either "not-running" ([container.WaitConditionNotRunning])
 // (default),  "next-exit" ([container.WaitConditionNextExit]), or "removed".
@@ -30,26 +41,26 @@ const containerWaitErrorMsgLimit = 2 * 1024 /* Max: 2KiB */
 // synchronize ContainerWait with other calls, such as specifying a
 // "next-exit" condition ([container.WaitConditionNextExit]) before
 // issuing a [Client.ContainerStart] request.
-func (cli *Client) ContainerWait(ctx context.Context, containerID string, condition container.WaitCondition) (<-chan container.WaitResponse, <-chan error) {
+func (cli *Client) ContainerWait(ctx context.Context, containerID string, options ContainerWaitOptions) ContainerWaitResult {
 	resultC := make(chan container.WaitResponse)
 	errC := make(chan error, 1)
 
 	containerID, err := trimID("container", containerID)
 	if err != nil {
 		errC <- err
-		return resultC, errC
+		return ContainerWaitResult{Results: resultC, Errors: errC}
 	}
 
 	query := url.Values{}
-	if condition != "" {
-		query.Set("condition", string(condition))
+	if options.Condition != "" {
+		query.Set("condition", string(options.Condition))
 	}
 
 	resp, err := cli.post(ctx, "/containers/"+containerID+"/wait", query, nil, nil)
 	if err != nil {
 		defer ensureReaderClosed(resp)
 		errC <- err
-		return resultC, errC
+		return ContainerWaitResult{Results: resultC, Errors: errC}
 	}
 
 	go func() {
@@ -80,7 +91,7 @@ func (cli *Client) ContainerWait(ctx context.Context, containerID string, condit
 		resultC <- res
 	}()
 
-	return resultC, errC
+	return ContainerWaitResult{Results: resultC, Errors: errC}
 }
 
 // legacyContainerWait returns immediately and doesn't have an option to wait

client/container_wait_test.go

@@ -21,11 +21,11 @@ import (
 func TestContainerWaitError(t *testing.T) {
 	client, err := NewClientWithOpts(WithMockClient(errorMock(http.StatusInternalServerError, "Server error")))
 	assert.NilError(t, err)
-	resultC, errC := client.ContainerWait(context.Background(), "nothing", "")
+	wait := client.ContainerWait(context.Background(), "nothing", ContainerWaitOptions{})
 	select {
-	case result := <-resultC:
+	case result := <-wait.Results:
 		t.Fatalf("expected to not get a wait result, got %d", result.StatusCode)
-	case err := <-errC:
+	case err := <-wait.Errors:
 		assert.Check(t, is.ErrorType(err, cerrdefs.IsInternal))
 	}
 }
@@ -38,11 +38,11 @@ func TestContainerWaitConnectionError(t *testing.T) {
 	client, err := NewClientWithOpts(WithAPIVersionNegotiation(), WithHost("tcp://no-such-host.invalid"))
 	assert.NilError(t, err)
 
-	resultC, errC := client.ContainerWait(context.Background(), "nothing", "")
+	wait := client.ContainerWait(context.Background(), "nothing", ContainerWaitOptions{})
 	select {
-	case result := <-resultC:
+	case result := <-wait.Results:
 		t.Fatalf("expected to not get a wait result, got %d", result.StatusCode)
-	case err := <-errC:
+	case err := <-wait.Errors:
 		assert.Check(t, is.ErrorType(err, IsErrConnectionFailed))
 	}
 }
@@ -59,11 +59,11 @@ func TestContainerWait(t *testing.T) {
 	}))
 	assert.NilError(t, err)
 
-	resultC, errC := client.ContainerWait(context.Background(), "container_id", "")
+	wait := client.ContainerWait(context.Background(), "container_id", ContainerWaitOptions{})
 	select {
-	case err := <-errC:
+	case err := <-wait.Errors:
 		assert.NilError(t, err)
-	case result := <-resultC:
+	case result := <-wait.Results:
 		assert.Check(t, is.Equal(result.StatusCode, int64(15)))
 	}
 }
@@ -82,11 +82,11 @@ func TestContainerWaitProxyInterrupt(t *testing.T) {
 	}))
 	assert.NilError(t, err)
 
-	resultC, errC := client.ContainerWait(context.Background(), "container_id", "")
+	wait := client.ContainerWait(context.Background(), "container_id", ContainerWaitOptions{})
 	select {
-	case err := <-errC:
+	case err := <-wait.Errors:
 		assert.Check(t, is.ErrorContains(err, expErr))
-	case result := <-resultC:
+	case result := <-wait.Results:
 		t.Fatalf("Unexpected result: %v", result)
 	}
 }
@@ -102,12 +102,12 @@ func TestContainerWaitProxyInterruptLong(t *testing.T) {
 	}))
 	assert.NilError(t, err)
 
-	resultC, errC := client.ContainerWait(context.Background(), "container_id", "")
+	wait := client.ContainerWait(context.Background(), "container_id", ContainerWaitOptions{})
 	select {
-	case err := <-errC:
+	case err := <-wait.Errors:
 		// LimitReader limiting isn't exact, because of how the Readers do chunking.
 		assert.Check(t, len(err.Error()) <= containerWaitErrorMsgLimit*2, "Expected error to be limited around %d, actual length: %d", containerWaitErrorMsgLimit, len(err.Error()))
-	case result := <-resultC:
+	case result := <-wait.Results:
 		t.Fatalf("Unexpected result: %v", result)
 	}
 }
@@ -134,12 +134,12 @@ func TestContainerWaitErrorHandling(t *testing.T) {
 				}, nil
 			}))
 			assert.NilError(t, err)
-			resultC, errC := client.ContainerWait(ctx, "container_id", "")
+			wait := client.ContainerWait(ctx, "container_id", ContainerWaitOptions{})
 			select {
-			case err := <-errC:
+			case err := <-wait.Errors:
 				assert.Check(t, is.Equal(err.Error(), test.exp.Error()))
 				return
-			case result := <-resultC:
+			case result := <-wait.Results:
 				t.Fatalf("expected to not get a wait result, got %d", result.StatusCode)
 				return
 			}
@@ -153,8 +153,8 @@ func ExampleClient_ContainerWait_withTimeout() {
 	defer cancel()
 
 	client, _ := NewClientWithOpts(FromEnv)
-	_, errC := client.ContainerWait(ctx, "container_id", "")
-	if err := <-errC; err != nil {
+	wait := client.ContainerWait(ctx, "container_id", ContainerWaitOptions{})
+	if err := <-wait.Errors; err != nil {
 		log.Fatal(err)
 	}
 }

integration-cli/docker_api_containers_test.go

@@ -820,12 +820,12 @@ func (s *DockerAPISuite) TestContainerAPIWait(c *testing.T) {
 	assert.NilError(c, err)
 	defer apiClient.Close()
 
-	waitResC, errC := apiClient.ContainerWait(testutil.GetContext(c), name, "")
+	wait := apiClient.ContainerWait(testutil.GetContext(c), name, client.ContainerWaitOptions{})
 
 	select {
-	case err = <-errC:
+	case err = <-wait.Errors:
 		assert.NilError(c, err)
-	case waitRes := <-waitResC:
+	case waitRes := <-wait.Results:
 		assert.Equal(c, waitRes.StatusCode, int64(0))
 	}
 }

integration/container/create_test.go

@@ -498,7 +498,7 @@ func TestCreateTmpfsOverrideAnonymousVolume(t *testing.T) {
 	// Normally an anonymous volume would, except now tmpfs should prevent that.
 	assert.Assert(t, is.Len(inspect.Container.Mounts, 0))
 
-	chWait, chErr := apiClient.ContainerWait(ctx, id, container.WaitConditionNextExit)
+	wait := apiClient.ContainerWait(ctx, id, client.ContainerWaitOptions{Condition: container.WaitConditionNextExit})
 	_, err = apiClient.ContainerStart(ctx, id, client.ContainerStartOptions{})
 	assert.NilError(t, err)
 
@@ -508,13 +508,13 @@ func TestCreateTmpfsOverrideAnonymousVolume(t *testing.T) {
 	select {
 	case <-timeout.C:
 		t.Fatal("timeout waiting for container to exit")
-	case status := <-chWait:
+	case status := <-wait.Results:
 		var errMsg string
 		if status.Error != nil {
 			errMsg = status.Error.Message
 		}
 		assert.Equal(t, int(status.StatusCode), 0, errMsg)
-	case err := <-chErr:
+	case err := <-wait.Errors:
 		assert.NilError(t, err)
 	}
 }

integration/container/daemon_linux_test.go

@@ -216,10 +216,10 @@ func TestRestartDaemonWithRestartingContainer(t *testing.T) {
 
 	ctxTimeout, cancel := context.WithTimeout(ctx, 30*time.Second)
 	defer cancel()
-	chOk, chErr := apiClient.ContainerWait(ctxTimeout, id, containertypes.WaitConditionNextExit)
+	wait := apiClient.ContainerWait(ctxTimeout, id, client.ContainerWaitOptions{Condition: containertypes.WaitConditionNextExit})
 	select {
-	case <-chOk:
-	case err := <-chErr:
+	case <-wait.Results:
+	case err := <-wait.Errors:
 		assert.NilError(t, err)
 	}
 }

integration/container/wait_test.go

@@ -45,11 +45,11 @@ func TestWaitNonBlocked(t *testing.T) {
 			containerID := container.Run(ctx, t, cli, container.WithCmd("sh", "-c", tc.cmd))
 			poll.WaitOn(t, container.IsInState(ctx, cli, containerID, containertypes.StateExited), poll.WithTimeout(30*time.Second))
 
-			waitResC, errC := cli.ContainerWait(ctx, containerID, "")
+			wait := cli.ContainerWait(ctx, containerID, client.ContainerWaitOptions{})
 			select {
-			case err := <-errC:
+			case err := <-wait.Errors:
 				assert.NilError(t, err)
-			case waitRes := <-waitResC:
+			case waitRes := <-wait.Results:
 				assert.Check(t, is.Equal(tc.expectedCode, waitRes.StatusCode))
 			}
 		})
@@ -85,15 +85,15 @@ func TestWaitBlocked(t *testing.T) {
 			// t.Parallel()
 			ctx := testutil.StartSpan(ctx, t)
 			containerID := container.Run(ctx, t, cli, container.WithCmd("sh", "-c", tc.cmd))
-			waitResC, errC := cli.ContainerWait(ctx, containerID, "")
+			wait := cli.ContainerWait(ctx, containerID, client.ContainerWaitOptions{})
 
 			_, err := cli.ContainerStop(ctx, containerID, client.ContainerStopOptions{})
 			assert.NilError(t, err)
 
 			select {
-			case err := <-errC:
+			case err := <-wait.Errors:
 				assert.NilError(t, err)
-			case waitRes := <-waitResC:
+			case waitRes := <-wait.Results:
 				assert.Check(t, is.Equal(tc.expectedCode, waitRes.StatusCode))
 			case <-time.After(2 * time.Second):
 				t.Fatal("timeout waiting for `docker wait`")
@@ -150,11 +150,11 @@ func TestWaitConditions(t *testing.T) {
 
 			_, err = cli.ContainerStart(ctx, containerID, client.ContainerStartOptions{})
 			assert.NilError(t, err)
-			waitResC, errC := cli.ContainerWait(ctx, containerID, tc.waitCond)
+			wait := cli.ContainerWait(ctx, containerID, client.ContainerWaitOptions{Condition: tc.waitCond})
 			select {
-			case err := <-errC:
+			case err := <-wait.Errors:
 				t.Fatalf("ContainerWait() err = %v", err)
-			case res := <-waitResC:
+			case res := <-wait.Results:
 				t.Fatalf("ContainerWait() sent exit code (%v) before ContainerStart()", res)
 			default:
 			}
@@ -166,9 +166,9 @@ func TestWaitConditions(t *testing.T) {
 			assert.NilError(t, err)
 
 			select {
-			case err := <-errC:
+			case err := <-wait.Errors:
 				assert.NilError(t, err)
-			case waitRes := <-waitResC:
+			case waitRes := <-wait.Results:
 				assert.Check(t, is.Equal(int64(99), waitRes.StatusCode))
 			case <-time.After(StopContainerWindowsPollTimeout):
 				ctr, _ := cli.ContainerInspect(ctx, containerID, client.ContainerInspectOptions{})
@@ -213,7 +213,7 @@ func TestWaitRestartedContainer(t *testing.T) {
 			defer cli.ContainerRemove(ctx, containerID, client.ContainerRemoveOptions{Force: true})
 
 			// Container is running now, wait for exit
-			waitResC, errC := cli.ContainerWait(ctx, containerID, tc.waitCond)
+			wait := cli.ContainerWait(ctx, containerID, client.ContainerWaitOptions{Condition: tc.waitCond})
 
 			timeout := 10
 			// On Windows it will always timeout, because our process won't receive SIGTERM
@@ -229,11 +229,11 @@ func TestWaitRestartedContainer(t *testing.T) {
 			assert.NilError(t, err)
 
 			select {
-			case err := <-errC:
+			case err := <-wait.Errors:
 				t.Fatalf("Unexpected error: %v", err)
 			case <-time.After(time.Second * 3):
 				t.Fatalf("Wait should end after restart")
-			case waitRes := <-waitResC:
+			case waitRes := <-wait.Results:
 				expectedCode := int64(5)
 
 				if !isWindowDaemon {