fix(docker): start containers before attaching #342
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,105 @@ | ||
| /* | ||
| Copyright 2026 The Crossplane Authors. | ||
|
|
||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||
| you may not use this file except in compliance with the License. | ||
| You may obtain a copy of the License at | ||
|
|
||
| http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| Unless required by applicable law or agreed to in writing, software | ||
| distributed under the License is distributed on an "AS IS" BASIS, | ||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| */ | ||
|
|
||
| package docker | ||
|
|
||
| import ( | ||
| "context" | ||
| "errors" | ||
| "strings" | ||
| "testing" | ||
|
|
||
| "github.com/moby/moby/client" | ||
| ) | ||
|
|
||
| func TestStartAndAttach(t *testing.T) { | ||
| errStart := errors.New("start failed") | ||
| errAttach := errors.New("attach failed") | ||
|
|
||
| cases := map[string]struct { | ||
| stdin bool | ||
| startErr error | ||
| attachErr error | ||
| wantCalls []string | ||
| wantErr string | ||
| wantOptions client.ContainerAttachOptions | ||
| }{ | ||
|
Comment on lines
+32
to
+39
Contributor
There was a problem hiding this comment. 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win Use Thanks for covering the success and failure paths. Please group invocation inputs under As per path instructions, “Enforce table-driven test structure: ... args/want pattern” and “Ensure ... reason fields.” 🤖 Prompt for AI AgentsSource: Path instructions |
||
| "Success": { | ||
| stdin: true, | ||
| wantCalls: []string{"start", "attach"}, | ||
| wantOptions: client.ContainerAttachOptions{ | ||
| Stream: true, | ||
| Stdout: true, | ||
| Stderr: true, | ||
| Stdin: true, | ||
| Logs: true, | ||
| }, | ||
| }, | ||
| "StartFailureDoesNotAttach": { | ||
| startErr: errStart, | ||
| wantCalls: []string{"start"}, | ||
| wantErr: "failed to start container: start failed", | ||
| }, | ||
| "AttachFailure": { | ||
| attachErr: errAttach, | ||
| wantCalls: []string{"start", "attach"}, | ||
| wantErr: "failed to attach to container: attach failed", | ||
| wantOptions: client.ContainerAttachOptions{ | ||
| Stream: true, | ||
| Stdout: true, | ||
| Stderr: true, | ||
| Logs: true, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| for name, tc := range cases { | ||
| t.Run(name, func(t *testing.T) { | ||
| calls := []string{} | ||
| var gotOptions client.ContainerAttachOptions | ||
| _, err := startAndAttach(context.Background(), "container-id", tc.stdin, runContainerCalls{ | ||
| start: func(_ context.Context, id string, _ client.ContainerStartOptions) error { | ||
| if id != "container-id" { | ||
| t.Errorf("start id = %q, want container-id", id) | ||
| } | ||
| calls = append(calls, "start") | ||
| return tc.startErr | ||
| }, | ||
| attach: func(_ context.Context, id string, opts client.ContainerAttachOptions) (client.ContainerAttachResult, error) { | ||
| if id != "container-id" { | ||
| t.Errorf("attach id = %q, want container-id", id) | ||
| } | ||
| calls = append(calls, "attach") | ||
| gotOptions = opts | ||
| return client.ContainerAttachResult{}, tc.attachErr | ||
| }, | ||
| }) | ||
|
|
||
| if strings.Join(calls, ",") != strings.Join(tc.wantCalls, ",") { | ||
| t.Errorf("calls = %v, want %v", calls, tc.wantCalls) | ||
| } | ||
| if gotOptions != tc.wantOptions { | ||
| t.Errorf("attach options = %+v, want %+v", gotOptions, tc.wantOptions) | ||
| } | ||
| switch { | ||
| case tc.wantErr == "" && err != nil: | ||
| t.Fatalf("unexpected error: %v", err) | ||
| case tc.wantErr != "" && (err == nil || err.Error() != tc.wantErr): | ||
| t.Fatalf("error = %v, want %q", err, tc.wantErr) | ||
|
Comment on lines
+100
to
+101
Contributor
There was a problem hiding this comment. 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win 🔎 Supported by static analysis🔍 Additional research query:
💡 Result: No relevant context found from external knowledge sources. 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- go.mod error dependencies ---'
rg -n 'go-cmp|pkg/errors|runtime/pkg/errors' go.mod go.sum
printf '%s\n' '--- test imports and assertions ---'
sed -n '1,125p' internal/docker/docker_test.go
printf '%s\n' '--- repository error-comparison patterns ---'
rg -n -g '*_test.go' 'cmp\.Diff|EquateErrors|wantErr|errors\.Is|errors\.Cause' internal/dockerRepository: crossplane/cli Length of output: 4571 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- startAndAttach implementation and related tests ---'
rg -n -A45 -B10 'func startAndAttach|startAndAttach\(' internal/docker
printf '%s\n' '--- error package bindings in the implementation ---'
rg -n '^(import|\s+"[^"]*errors|\s+errors\.)' internal/docker --glob '*.go'
printf '%s\n' '--- cmp usage in nearby repository tests ---'
rg -n -A8 -B4 'cmp\.Diff|cmpopts\.EquateErrors' --glob '*_test.go' internal | head -160Repository: crossplane/cli Length of output: 20526 🌐 Web query:
💡 Result: In go-cmp v0.7.0, the cmpopts.EquateErrors function provides a way to compare error values using the semantic equality defined by the standard library's errors.Is function [1][2]. When used, cmpopts.EquateErrors returns a cmp.Comparer option that determines two errors to be equal if errors.Is reports them to match [1]. Because errors.Is is not symmetric, the implementation ensures correctness by checking both directions: errors.Is(x, y) || errors.Is(y, x) [3][4]. Regarding the specific points in your query: 1. Relationship with errors.Is: cmpopts.EquateErrors is specifically designed to leverage errors.Is [1][2]. It is the idiomatic way to handle error comparisons in go-cmp, particularly for sentinel errors or wrapped errors that support the Is interface [1][5]. 2. github.com/pkg/errors: go-cmp does not have a native dependency on github.com/pkg/errors. However, because errors.Is (introduced in Go 1.13) is designed to be compatible with errors that implement the Is method, any error types—including those from github.com/pkg/errors that support this interface—will work with cmpopts.EquateErrors [2][6]. 3. Documentation and Panics: If you attempt to compare errors directly without an appropriate option (such as cmpopts.EquateErrors), go-cmp may panic if it encounters unexported fields within the error type [7][8]. Since v0.4.0 (and further refined in subsequent versions), the library is designed to detect when a type implements the error interface and will explicitly suggest using cmpopts.EquateErrors in the panic message to guide you toward the correct solution [9][8][10][2]. In summary, cmpopts.EquateErrors is the standard, supported method for comparing errors in go-cmp v0.7.0 and is fully compatible with the standard library's error wrapping semantics [1][4][2]. Citations:
Compare errors with 🤖 Prompt for AI AgentsSource: Path instructions |
||
| } | ||
| }) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the container errors actionable.
A render user can receive these errors through
RunContainer, butfailed to start containerandfailed to attach to containergive no recovery step. Include the render action and guidance such as verifying that the container engine is running and accessible.As per path instructions, “Ensure all error messages are meaningful to end users” and “suggest next steps when possible.”
Also applies to: 529-529
🤖 Prompt for AI Agents
Source: Path instructions