chore: enable noctx linter #3314
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Summary by CodeRabbit
WalkthroughAdds the Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant T as Test/Caller
participant HTTP as http.Client
participant S as Remote Service
Note over T: Build request bound to ctx
T->>HTTP: NewRequestWithContext(ctx, GET, URL, http.NoBody)
T->>HTTP: Do(req)
alt ctx active
HTTP->>S: HTTP request
S-->>HTTP: Response
HTTP-->>T: resp
Note right of T: defer resp.Body.Close()
else ctx canceled/deadline
HTTP-->>T: error (context canceled/timeout)
end
sequenceDiagram
autonumber
participant T as Caller
participant OS as exec.CommandContext
participant P as Child Process
T->>OS: CommandContext(ctx, cmd...)
alt ctx active
OS->>P: start
P-->>OS: exit/output
OS-->>T: result
else ctx canceled
OS-->>P: kill
OS-->>T: error (killed/canceled)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
🔇 Additional comments (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
modules/ollama/examples_test.go (1)
89-96: Restore JSON Content-Type headerSwitching from
http.Postdropped the automaticContent-Type: application/jsonheader. Without it, the Ollama API can reject or mis-handle the payload, so the example risks failing now.req, err := http.NewRequestWithContext(ctx, http.MethodPost, connectionStr+"/api/generate", strings.NewReader(payload)) if err != nil { log.Printf("failed to create request: %s", err) return } + req.Header.Set("Content-Type", "application/json") resp, err := httpClient.Do(req)modules/k6/k6.go (1)
53-66: Close the HTTP response body to avoid leaking connections
io.Copyreading the entire payload doesn't close the underlying connection; without an explicitresp.Body.Close()(or defer) every download leaves the descriptor open, which can quickly exhaust sockets and prevents connection reuse. Please close the body right after the request succeeds.resp, err := client.Do(req) if err != nil { return err } +defer resp.Body.Close() downloadedFile, err := os.Create(d.getDownloadPath())modules/vault/vault_test.go (1)
63-70: Handle the request-construction error
http.NewRequestWithContextcan return an error (e.g., ifhostAddressis malformed). Ignoring it meansrequestcould be nil andrequest.Header.Add(...)will panic. Please check the error just like we do for the subsequent HTTP call.- request, _ := http.NewRequestWithContext(ctx, http.MethodGet, hostAddress+"/v1/secret/data/test1", http.NoBody) + request, err := http.NewRequestWithContext(ctx, http.MethodGet, hostAddress+"/v1/secret/data/test1", http.NoBody) + require.NoError(t, err)
🧹 Nitpick comments (7)
modules/vearch/vearch_test.go (1)
15-29: Move to a bounded context in the test. Usingcontext.Background()keeps these container + HTTP calls uncancellable; if the Vearch endpoint ever stalls, the test will hang until CI kills it. Now that every call accepts a context, wrap it in a reasonable timeout and cancel to keep the suite resilient.- ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + @@ - resp, err := http.DefaultClient.Do(req) + resp, err := http.DefaultClient.Do(req)(remember to import
timeat the top)modules/mockserver/examples_test.go (1)
86-97: Ensureresp.Bodycloses on all pathsRight now the body is only closed after the successful
ReadAll. If that read fails, we log and return without cleaning up, leaking the response body. Please defer the close immediately afterDoso we release the body even on error paths.resp, err := http.DefaultClient.Do(req) if err != nil { log.Printf("failed to send request: %s", err) return } +defer func() { + if err := resp.Body.Close(); err != nil { + log.Printf("failed to close response body: %s", err) + } +}() buf, err := io.ReadAll(resp.Body) if err != nil { log.Printf("failed to read response: %s", err) return } - resp.Body.Close()modulegen/internal/tools/exec.go (1)
12-30: Let callers provide the execution contextRight now each public wrapper hardcodes
context.Background(), so even with the new context-awarerunCommandwe still lose cancellation/timeouts. Since this package is internal, we can safely extend these functions to accept acontext.Contextand thread it through, letting generator code pass the context it already manages.-func GoModTidy(cmdDir string) error { - if err := runCommand(context.Background(), cmdDir, "go", "mod", "tidy"); err != nil { +func GoModTidy(ctx context.Context, cmdDir string) error { + if err := runCommand(ctx, cmdDir, "go", "mod", "tidy"); err != nil { return fmt.Errorf("mod tidy dependencies: %w", err) } return nil } // GoVet checks the generated code for errors, // running the `go vet ./...` command in the given directory. -func GoVet(cmdDir string) error { - if err := runCommand(context.Background(), cmdDir, "go", "vet", "./..."); err != nil { +func GoVet(ctx context.Context, cmdDir string) error { + if err := runCommand(ctx, cmdDir, "go", "vet", "./..."); err != nil { return fmt.Errorf("go vet generated code: %w", err) } return nil } // MakeLint runs the `make lint` command in the given directory. -func MakeLint(cmdDir string) error { - if err := runCommand(context.Background(), cmdDir, "make", "lint"); err != nil { +func MakeLint(ctx context.Context, cmdDir string) error { + if err := runCommand(ctx, cmdDir, "make", "lint"); err != nil { return fmt.Errorf("make lint module: %w", err) } return nil }modules/localstack/examples_test.go (1)
246-251: Close the HTTP response body to avoid leaks.Without an explicit
resp.Body.Close(), the connection stays open until GC, which can exhaust idle connections when these examples run repeatedly. Please add the close after theDocall.resp, err := httpClient.Do(req) if err != nil { log.Printf("failed to send request to lambda function: %s", err) return } + defer resp.Body.Close()modules/openfga/examples_test.go (1)
77-84: Close the HTTP response bodyWe now create the request manually, so we should also close the response explicitly to avoid leaking the underlying TCP connection.
resp, err := httpClient.Do(req) if err != nil { log.Printf("failed to get playground endpoint: %s", err) return } + defer resp.Body.Close() + fmt.Println(resp.StatusCode)modules/socat/socat_test.go (2)
67-74: Remember to close response bodiesWe read the body but never close it, which leaves the connection hanging until GC kicks in. Adding a
defer resp.Body.Close()right after the success check keeps the transport healthy.resp, err := httpClient.Do(req) require.NoError(t, err) +defer resp.Body.Close() body, err := io.ReadAll(resp.Body)
227-234: Close response bodies inside the loopEach iteration should close its response body; otherwise we leak handles and may starve the transport if the loop grows. Add a
defer(or explicit close) after the error check.resp, err := httpClient.Do(req) require.NoError(t, err) + defer resp.Body.Close() require.Equal(t, 200, resp.StatusCode)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (45)
.golangci.yml(1 hunks)docker_auth_test.go(1 hunks)docker_test.go(10 hunks)docs/features/creating_container.md(1 hunks)examples/nginx/nginx_test.go(1 hunks)generic_test.go(1 hunks)internal/core/docker_host.go(1 hunks)logconsumer_test.go(6 hunks)modulegen/internal/tools/exec.go(2 hunks)modules/artemis/artemis_test.go(2 hunks)modules/chroma/chroma_test.go(1 hunks)modules/cockroachdb/examples_test.go(1 hunks)modules/compose/compose_local.go(1 hunks)modules/consul/consul_test.go(1 hunks)modules/databend/databend_test.go(2 hunks)modules/databend/examples_test.go(1 hunks)modules/dolt/dolt.go(1 hunks)modules/dolt/dolt_test.go(3 hunks)modules/dolt/examples_test.go(1 hunks)modules/elasticsearch/elasticsearch_test.go(2 hunks)modules/k6/k6.go(2 hunks)modules/localstack/examples_test.go(1 hunks)modules/mariadb/mariadb_test.go(4 hunks)modules/meilisearch/meilisearch_test.go(3 hunks)modules/mockserver/examples_test.go(1 hunks)modules/mssql/mssql_test.go(3 hunks)modules/mysql/examples_test.go(1 hunks)modules/mysql/mysql_test.go(3 hunks)modules/ollama/examples_test.go(2 hunks)modules/ollama/ollama_test.go(2 hunks)modules/openfga/examples_test.go(3 hunks)modules/opensearch/opensearch_test.go(1 hunks)modules/postgres/postgres_test.go(4 hunks)modules/pulsar/pulsar_test.go(1 hunks)modules/qdrant/qdrant_test.go(2 hunks)modules/registry/registry_test.go(5 hunks)modules/socat/examples_test.go(2 hunks)modules/socat/socat_test.go(3 hunks)modules/vault/vault_test.go(1 hunks)modules/vearch/vearch_test.go(1 hunks)modules/weaviate/weaviate_test.go(1 hunks)modules/yugabytedb/yugabytedb_test.go(2 hunks)testcontainers_test.go(1 hunks)wait/host_port_test.go(7 hunks)wait/http_test.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
modules/mockserver/examples_test.go (1)
log/logger.go (1)
Printf(47-49)
modulegen/internal/tools/exec.go (1)
modulegen/internal/context/context.go (1)
Context(12-14)
modules/ollama/examples_test.go (1)
log/logger.go (1)
Printf(47-49)
modules/socat/examples_test.go (1)
log/logger.go (1)
Printf(47-49)
modules/dolt/examples_test.go (1)
log/logger.go (1)
Printf(47-49)
modules/localstack/examples_test.go (1)
log/logger.go (1)
Printf(47-49)
modules/registry/registry_test.go (1)
modules/registry/registry.go (1)
Run(275-316)
modules/openfga/examples_test.go (1)
modules/openfga/openfga.go (1)
Run(51-93)
🔇 Additional comments (6)
modules/meilisearch/meilisearch_test.go (3)
26-31: Context-aware request creation looks solidSwitching to
http.NewRequestWithContextwhile usinghttp.NoBodykeeps the semantics the same and satisfies the linter without altering behavior. Nicely done.
49-54: Consistent context usageMirroring the earlier change here keeps every HTTP call in the test bound to the shared context, which is exactly what we want alongside the linter enablement.
61-67: Auth request now context-aware tooCarrying the context through the authenticated request closes the loop—noCtx complaints addressed without affecting the request setup.
testcontainers_test.go (1)
51-60: No action needed: go.mod declares Go 1.24.0 (>=1.22)
t.Context()is supported since Go 1.22 and our minimum is Go 1.24.0.generic_test.go (1)
155-159: Confirmed compatibility: go.mod uses Go 1.24.0 (>=1.22)
testing.T.Context() is available since Go 1.22, so this compiles as written.modules/ollama/ollama_test.go (1)
112-120: No action needed: go.mod declares Go 1.24.0, which supports testing.T.Context.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/vearch/vearch_test.go (1)
16-36: 30 s context budget is too short for container startupThis context wraps the entire test, so the initial image pull / container startup will be aborted after 30 s. On a cold CI runner, pulling
vearch/vearch:3.5.1routinely exceeds that, so the test becomes flaky. Please keep the long-lived test context for container lifecycle and apply the short timeout only around the HTTP call.- ctx, cancel := context.WithTimeout(t.Context(), 30*time.Second) - defer cancel() + ctx := t.Context() @@ - req, err := http.NewRequestWithContext(ctx, http.MethodGet, restEndpoint, http.NoBody) - require.NoError(t, err) - - resp, err := http.DefaultClient.Do(req) + reqCtx, cancel := context.WithTimeout(ctx, 30*time.Second) + defer cancel() + + req, err := http.NewRequestWithContext(reqCtx, http.MethodGet, restEndpoint, http.NoBody) + require.NoError(t, err) + + resp, err := http.DefaultClient.Do(req)
🧹 Nitpick comments (5)
modules/ollama/examples_test.go (2)
96-103: Close response bodies to avoid leaking connectionsNow that we’re manually creating the request, we also need to close the response body so the transport can reuse the connection. Without closing, callers that run this example may eventually exhaust idle connections. Add the close right after the error check.
resp, err := httpClient.Do(req) if err != nil { log.Printf("failed to get response: %s", err) return } +defer resp.Body.Close()
329-348: Close the response body before decodingSame here—once we switch to
Do(req), we own the body and must close it to avoid leaking sockets during example runs. Please defer the close as soon as the response succeeds.resp, err := http.DefaultClient.Do(req) if err != nil { log.Printf("failed to get request: %s", err) return } +defer resp.Body.Close()modules/ollama/ollama_test.go (1)
32-36: Prefer test-scoped context for HTTP request
ctxis stillcontext.Background(), so this call won’t be canceled if the test aborts or times out. Please switch tot.Context()(as you already do inassertLoadedModel) so the HTTP request lifecycle remains tied to the test.- req, err := http.NewRequestWithContext(ctx, http.MethodGet, connectionStr, http.NoBody) + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, connectionStr, http.NoBody)modules/postgres/postgres_test.go (1)
338-345: Close query rows to release connectionsWithout closing
rows, the driver may hold on to the underlying connection until GC, which can leak pooled connections when the test runs repeatedly. Add an explicit close after confirming the query succeeded.rows, err := db.QueryContext(ctx, "SELECT COUNT(*) FROM testdb;") require.NoError(t, err) require.NotNil(t, rows) + defer rows.Close() for rows.Next() {internal/core/docker_host.go (1)
47-54: Add a timeout to the gateway probeNow that we switched to
CommandContext, we can guard the shell probe with a short deadline so a stuckip routecall doesn’t hang forever. Please wrap the background context incontext.WithTimeout(and importtime) before passing it down.- cmd := exec.CommandContext(context.Background(), "sh", "-c", "ip route|awk '/default/ { print $3 }'") + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + cmd := exec.CommandContext(ctx, "sh", "-c", "ip route|awk '/default/ { print $3 }'")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (45)
.golangci.yml(1 hunks)docker_auth_test.go(1 hunks)docker_test.go(10 hunks)docs/features/creating_container.md(1 hunks)examples/nginx/nginx_test.go(1 hunks)generic_test.go(1 hunks)internal/core/docker_host.go(1 hunks)logconsumer_test.go(6 hunks)modulegen/internal/tools/exec.go(2 hunks)modules/artemis/artemis_test.go(2 hunks)modules/chroma/chroma_test.go(1 hunks)modules/cockroachdb/examples_test.go(1 hunks)modules/compose/compose_local.go(1 hunks)modules/consul/consul_test.go(1 hunks)modules/databend/databend_test.go(2 hunks)modules/databend/examples_test.go(1 hunks)modules/dolt/dolt.go(1 hunks)modules/dolt/dolt_test.go(3 hunks)modules/dolt/examples_test.go(1 hunks)modules/elasticsearch/elasticsearch_test.go(2 hunks)modules/k6/k6.go(3 hunks)modules/localstack/examples_test.go(1 hunks)modules/mariadb/mariadb_test.go(4 hunks)modules/meilisearch/meilisearch_test.go(3 hunks)modules/mockserver/examples_test.go(1 hunks)modules/mssql/mssql_test.go(3 hunks)modules/mysql/examples_test.go(1 hunks)modules/mysql/mysql_test.go(3 hunks)modules/ollama/examples_test.go(2 hunks)modules/ollama/ollama_test.go(2 hunks)modules/openfga/examples_test.go(3 hunks)modules/opensearch/opensearch_test.go(1 hunks)modules/postgres/postgres_test.go(4 hunks)modules/pulsar/pulsar_test.go(1 hunks)modules/qdrant/qdrant_test.go(2 hunks)modules/registry/registry_test.go(5 hunks)modules/socat/examples_test.go(2 hunks)modules/socat/socat_test.go(3 hunks)modules/vault/vault_test.go(1 hunks)modules/vearch/vearch_test.go(3 hunks)modules/weaviate/weaviate_test.go(1 hunks)modules/yugabytedb/yugabytedb_test.go(2 hunks)testcontainers_test.go(1 hunks)wait/host_port_test.go(7 hunks)wait/http_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (22)
- modules/meilisearch/meilisearch_test.go
- modules/registry/registry_test.go
- modules/elasticsearch/elasticsearch_test.go
- modules/dolt/examples_test.go
- modulegen/internal/tools/exec.go
- docker_auth_test.go
- modules/localstack/examples_test.go
- modules/k6/k6.go
- modules/vault/vault_test.go
- wait/http_test.go
- modules/opensearch/opensearch_test.go
- modules/mariadb/mariadb_test.go
- modules/weaviate/weaviate_test.go
- generic_test.go
- modules/mockserver/examples_test.go
- docs/features/creating_container.md
- modules/dolt/dolt.go
- modules/yugabytedb/yugabytedb_test.go
- docker_test.go
- modules/socat/examples_test.go
- modules/artemis/artemis_test.go
- modules/socat/socat_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
modules/ollama/examples_test.go (1)
log/logger.go (1)
Printf(47-49)
modules/openfga/examples_test.go (1)
modules/openfga/openfga.go (1)
Run(51-93)
🔇 Additional comments (2)
testcontainers_test.go (1)
51-60: Context propagation aligns with test cancellations.Wiring the
exec.Commandinvocations throught.Context()lets the go tooling respect subtest cancellations and timeouts without altering the execution flow. Nicely keeps the helper runs bounded to the test lifecycle..golangci.yml (1)
12-18:noctxaddition aligns with lint goalsEnabling
noctxhere is consistent with the broader push to enforce context propagation, and the configuration integrates cleanly with the existing linter set.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
modules/ollama/ollama_test.go (2)
20-33:t.Context()requires Go 1.22—project still targets Go 1.21Nice catch replacing
context.Background(), buttesting.T.Context()was only introduced in Go 1.22. The repo is still on Go 1.21 (see go.mod), so this change breaks the build. Please keep usingcontext.Background()(or another 1.21-compatible context source) until we bump the toolchain.
108-119: Same Go-version breakage hereThe helper now grabs
t.Context(), which also depends on Go 1.22. This will not compile under the current Go 1.21 toolchain, so we need to revert to the previous approach (e.g., passing acontext.Contextinto the helper or usingcontext.Background()).
🧹 Nitpick comments (4)
modules/ollama/examples_test.go (1)
89-102: Guard against reusing a closed request bodyAfter
http.NewRequestWithContext, the body is astrings.Reader. OnceDoruns, the transport may fully consume it; deferringresp.Body.Close()is good, but for clarity and to avoid future reuse mistakes, setreq.GetBody(or recreate the reader per retry) if this request ever gets retried. Not blocking, just something to consider.modules/socat/socat_test.go (1)
225-234: Avoid deferringresp.Body.Close()inside the loopEach defer waits until the whole test ends, so four responses remain open for longer than necessary. Close them explicitly after reading, or wrap the loop body in a helper that defers inside that scope.
modules/mysql/examples_test.go (1)
83-94: Consider giving the DB calls a cancellable context
PingContext,PrepareContext, andQueryRowContextwill now inherit thecontext.Background()created earlier, so these examples can still block forever if MySQL hangs. Wrapping the background context in a reasonable timeout keeps the documentation sample resilient while still satisfyingnoctx.-import ( - "context" +import ( + "context" "database/sql" "fmt" "log" "path/filepath" + "time" @@ - ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel()modules/mockserver/examples_test.go (1)
79-97: Add a timeout to the request contextNow that we build the POST with
http.NewRequestWithContext, staying oncontext.Background()means the example has no escape hatch if the mockserver never replies. A short timeout keeps the doc test from hanging indefinitely while still showcasing the context-aware APIs.-import ( - "context" +import ( + "context" "fmt" "io" "log" "net/http" "strings" + "time" @@ - ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (45)
.golangci.yml(1 hunks)docker_auth_test.go(1 hunks)docker_test.go(10 hunks)docs/features/creating_container.md(1 hunks)examples/nginx/nginx_test.go(1 hunks)generic_test.go(1 hunks)internal/core/docker_host.go(2 hunks)logconsumer_test.go(6 hunks)modulegen/internal/tools/exec.go(2 hunks)modules/artemis/artemis_test.go(2 hunks)modules/chroma/chroma_test.go(1 hunks)modules/cockroachdb/examples_test.go(1 hunks)modules/compose/compose_local.go(1 hunks)modules/consul/consul_test.go(1 hunks)modules/databend/databend_test.go(2 hunks)modules/databend/examples_test.go(1 hunks)modules/dolt/dolt.go(1 hunks)modules/dolt/dolt_test.go(3 hunks)modules/dolt/examples_test.go(1 hunks)modules/elasticsearch/elasticsearch_test.go(2 hunks)modules/k6/k6.go(3 hunks)modules/localstack/examples_test.go(1 hunks)modules/mariadb/mariadb_test.go(4 hunks)modules/meilisearch/meilisearch_test.go(3 hunks)modules/mockserver/examples_test.go(1 hunks)modules/mssql/mssql_test.go(3 hunks)modules/mysql/examples_test.go(1 hunks)modules/mysql/mysql_test.go(3 hunks)modules/ollama/examples_test.go(2 hunks)modules/ollama/ollama_test.go(3 hunks)modules/openfga/examples_test.go(3 hunks)modules/opensearch/opensearch_test.go(1 hunks)modules/postgres/postgres_test.go(4 hunks)modules/pulsar/pulsar_test.go(1 hunks)modules/qdrant/qdrant_test.go(2 hunks)modules/registry/registry_test.go(5 hunks)modules/socat/examples_test.go(2 hunks)modules/socat/socat_test.go(3 hunks)modules/vault/vault_test.go(1 hunks)modules/vearch/vearch_test.go(3 hunks)modules/weaviate/weaviate_test.go(1 hunks)modules/yugabytedb/yugabytedb_test.go(2 hunks)testcontainers_test.go(1 hunks)wait/host_port_test.go(7 hunks)wait/http_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (26)
- modules/qdrant/qdrant_test.go
- modules/postgres/postgres_test.go
- modules/databend/examples_test.go
- modules/opensearch/opensearch_test.go
- modulegen/internal/tools/exec.go
- modules/elasticsearch/elasticsearch_test.go
- modules/chroma/chroma_test.go
- modules/mysql/mysql_test.go
- modules/dolt/examples_test.go
- docs/features/creating_container.md
- modules/compose/compose_local.go
- .golangci.yml
- docker_auth_test.go
- modules/artemis/artemis_test.go
- modules/registry/registry_test.go
- modules/pulsar/pulsar_test.go
- wait/host_port_test.go
- modules/openfga/examples_test.go
- wait/http_test.go
- modules/cockroachdb/examples_test.go
- docker_test.go
- modules/dolt/dolt_test.go
- examples/nginx/nginx_test.go
- testcontainers_test.go
- modules/weaviate/weaviate_test.go
- modules/dolt/dolt.go
🧰 Additional context used
🧬 Code graph analysis (4)
modules/socat/examples_test.go (1)
log/logger.go (1)
Printf(47-49)
modules/ollama/examples_test.go (1)
log/logger.go (1)
Printf(47-49)
modules/localstack/examples_test.go (1)
log/logger.go (1)
Printf(47-49)
modules/mockserver/examples_test.go (1)
log/logger.go (1)
Printf(47-49)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint (modules/azurite) / lint: modules/azurite
- GitHub Check: lint (modules/compose) / lint: modules/compose
- GitHub Check: Analyze (go)
🔇 Additional comments (25)
modules/yugabytedb/yugabytedb_test.go (2)
41-42: Context-aware ping keeps tests compliantSwitching to
PingContextwith the existingctxsatisfiesnoctxwhile preserving behavior. Looks good.
69-70: Same improvement for custom-options pathThanks for mirroring the context-aware ping in the custom-options branch; consistent usage will keep the linter quiet. 👍
modules/mssql/mssql_test.go (3)
36-43: Context-aware ping and DDL exec look solidUpdating to
PingContext/ExecContextcleanly propagates the test context and satisfies the new linter mandate. No further adjustments needed.
91-99: Good consistency on the connection-string variantApplying the same context-aware
PingContext/ExecContextpattern here keeps the test aligned with the rest of the suite. Looks good.
121-122: Nice follow-through on pinging with contextThe switch to
PingContextmirrors the earlier changes and ensures all DB touches are context-aware. 👍modules/localstack/examples_test.go (1)
239-252: Nice context-aware HTTP request setup.Using
http.NewRequestWithContext, explicitly setting theContent-Type, and deferringresp.Body.Close()keeps the call compliant withnoctxand prevents response leaks.modules/databend/databend_test.go (1)
35-43: Context-aware DB calls look goodSwitching the ping and table-creation logic to the context-aware
PingContext/ExecContextvariants keeps the linter happy and adds cancellation hooks without altering behavior.modules/mariadb/mariadb_test.go (2)
37-84: Nice upgrade to context-aware DB usageReplacing the raw driver calls with
PingContext/ExecContextacross these setup paths aligns the tests with the newnoctxpolicy while preserving the original flow.
114-164: Great to see full context propagationExtending the context-aware pattern through the prepared-statement and query helpers keeps the whole fetch path cancellable and consistent.
generic_test.go (1)
155-159: Stop runaway subtestsSwitching to
CommandContexttied tot.Context()lets the helper cancel the subprocess when the test finishes or times out—exactly what the newnoctxlint expects and a solid reliability win.internal/core/docker_host.go (1)
47-54: Good timeout guardLayering a 30 s timeout around the
ip routeprobe prevents this deprecated helper from hanging indefinitely while preserving the existing error reporting.logconsumer_test.go (5)
594-638: Close HTTP responses inTest_MultiContainerLogConsumer_CancelledContext.Both containers’ request sequences still leak
resp.Body. Please close every response—this is the same outstanding issue from the prior review.- _, err = http.DefaultClient.Do(req) - require.NoError(t, err) + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) req, err = http.NewRequestWithContext(ctx, http.MethodGet, ep1+"/stdout?echo=there1", http.NoBody) require.NoError(t, err) - _, err = http.DefaultClient.Do(req) - require.NoError(t, err) + resp, err = http.DefaultClient.Do(req) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) … - _, err = http.DefaultClient.Do(req) - require.NoError(t, err) + resp, err = http.DefaultClient.Do(req) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) req, err = http.NewRequestWithContext(ctx, http.MethodGet, ep2+"/stdout?echo=there2", http.NoBody) require.NoError(t, err) - _, err = http.DefaultClient.Do(req) - require.NoError(t, err) + resp, err = http.DefaultClient.Do(req) + require.NoError(t, err) + require.NoError(t, resp.Body.Close())
101-115: Close HTTP responses inTest_LogConsumerGetsCalled.Both of the first two
http.DefaultClient.Docalls drop the response, soBodynever closes and the connection leaks. This was already pointed out in the earlier review—please capture the response and close it just like the last request in this block.- _, err = http.DefaultClient.Do(req) - require.NoError(t, err) + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) req, err = http.NewRequestWithContext(ctx, http.MethodGet, ep+"/stdout?echo=there", http.NoBody) require.NoError(t, err) - _, err = http.DefaultClient.Do(req) - require.NoError(t, err) + resp, err = http.DefaultClient.Do(req) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) - req, err = http.NewRequestWithContext(ctx, http.MethodGet, ep+"/stdout?echo="+lastMessage, http.NoBody) + req, err = http.NewRequestWithContext(ctx, http.MethodGet, ep+"/stdout?echo="+lastMessage, http.NoBody) require.NoError(t, err) - resp, err := http.DefaultClient.Do(req) + resp, err = http.DefaultClient.Do(req) require.NoError(t, err) require.NoError(t, resp.Body.Close())
170-183: Close HTTP responses inTest_ShouldRecognizeLogTypes.Each
Dohere still leaks the response body; same issue as raised previously. Please ensure every request closesresp.Body.- _, err = http.DefaultClient.Do(req) - require.NoError(t, err) + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) req, err = http.NewRequestWithContext(ctx, http.MethodGet, ep+"/stderr?echo=this-is-stderr", http.NoBody) require.NoError(t, err) - _, err = http.DefaultClient.Do(req) - require.NoError(t, err) + resp, err = http.DefaultClient.Do(req) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) req, err = http.NewRequestWithContext(ctx, http.MethodGet, ep+"/stdout?echo="+lastMessage, http.NoBody) require.NoError(t, err) - _, err = http.DefaultClient.Do(req) - require.NoError(t, err) + resp, err = http.DefaultClient.Do(req) + require.NoError(t, err) + require.NoError(t, resp.Body.Close())
229-236: Close HTTP responses inTest_MultipleLogConsumers.Still the same leak: both requests ignore the response. Capture it and close the body.
- _, err = http.DefaultClient.Do(req) - require.NoError(t, err) + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) req, err = http.NewRequestWithContext(ctx, http.MethodGet, ep+"/stdout?echo="+lastMessage, http.NoBody) require.NoError(t, err) - _, err = http.DefaultClient.Do(req) - require.NoError(t, err) + resp, err = http.DefaultClient.Do(req) + require.NoError(t, err) + require.NoError(t, resp.Body.Close())
443-456: Close HTTP responses inTestContainerLogsEnableAtStart.Same unresolved leak: every
Docall must close the response body.- _, err = http.DefaultClient.Do(req) - require.NoError(t, err) + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) req, err = http.NewRequestWithContext(ctx, http.MethodGet, ep+"/stdout?echo=there", http.NoBody) require.NoError(t, err) - _, err = http.DefaultClient.Do(req) - require.NoError(t, err) + resp, err = http.DefaultClient.Do(req) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) req, err = http.NewRequestWithContext(ctx, http.MethodGet, ep+"/stdout?echo="+lastMessage, http.NoBody) require.NoError(t, err) - _, err = http.DefaultClient.Do(req) - require.NoError(t, err) + resp, err = http.DefaultClient.Do(req) + require.NoError(t, err) + require.NoError(t, resp.Body.Close())modules/meilisearch/meilisearch_test.go (3)
26-31: Context-aware Meilisearch readiness ping looks goodUsing
NewRequestWithContextwithhttp.NoBodyties the readiness probe to the test context without altering behaviour, so the new linter rule is satisfied while keeping the test semantics intact.
49-55: Dump-import health check follows the ctx-aware patternApplying the same context-aware request creation here keeps cancellation wired through and the explicit body close maintains the previous resource lifecycle.
61-68: Bearer-authenticated document fetch stays correctHeaders are preserved while threading
ctxthrough the second request, so cancellation propagates and the assertions remain unchanged.modules/consul/consul_test.go (1)
51-58: Consul HTTP probe now respects contextBinding the GET to
ctxand deferringBody.Close()resolves the linter hit and ensures sockets are released even if the subtest aborts early.modules/vault/vault_test.go (1)
63-70: Vault secret fetch honours context cleanlyThe explicit context-aware request plus error check matches the suite’s new pattern and keeps the response body lifecycle correct.
modules/socat/examples_test.go (2)
72-79: Single-target ping example is now cancellableSwitching to
NewRequestWithContextwith early error handling makes the example robust and compliant with the new linter without changing behaviour.
179-186: Multi-target loop mirrors the ctx-aware request flowEach iteration now constructs a context-bound request before dispatching, keeping error handling consistent with the single-target variant.
modules/k6/k6.go (2)
40-67: Downloader adopts context-aware HTTP correctlyUsing
NewRequestWithContextand deferring the body close keeps the timeout/auth logic intact while satisfying the noctx rule.
109-114: WithRemoteTestScript integrates the new signature smoothlyEagerly downloading with
context.Background()preserves previous behaviour while accommodating the ctx-aware helper; no regressions noted.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (14)
docker_test.go (3)
84-88: Close the HTTP response before discarding it.
http.DefaultClient.Dohands back a live*http.Response; dropping it on the floor without closing the body keeps the underlying connection open until GC and can leak descriptors when this test runs repeatedly. Capture the response, check the error, anddefer resp.Body.Close()even when you do not inspect the payload.Apply this diff:
- _, err = http.DefaultClient.Do(req) - require.NoErrorf(t, err, "Expected OK response") + resp, err := http.DefaultClient.Do(req) + require.NoErrorf(t, err, "Expected OK response") + defer resp.Body.Close()
149-153: Close the HTTP response before discarding it (again).Same leak risk as above: keep the response around long enough to
defer resp.Body.Close().Apply this diff:
- _, err = http.DefaultClient.Do(req) - require.NoErrorf(t, err, "Expected OK response") + resp, err := http.DefaultClient.Do(req) + require.NoErrorf(t, err, "Expected OK response") + defer resp.Body.Close()
161-163: Close the HTTP response before discarding it (third instance).Capture the response and close its body to avoid leaking the connection.
Apply this diff:
- _, err = http.DefaultClient.Do(req) - require.NoErrorf(t, err, "Expected OK response") + resp, err := http.DefaultClient.Do(req) + require.NoErrorf(t, err, "Expected OK response") + defer resp.Body.Close()generic_test.go (6)
155-159: Good switch to CommandContext; consider explicit timeout.Using t.Context cancels the subprocess if the test times out/cancels. Optionally bound it with a shorter timeout to avoid hangs if the helper gets stuck.
- cmd := exec.CommandContext(t.Context(), os.Args[0], "-test.run=TestHelperContainerStarterProcess", "-test.v=true") + ctx, cancel := context.WithTimeout(t.Context(), 2*time.Minute) + defer cancel() + cmd := exec.CommandContext(ctx, os.Args[0], "-test.run=TestHelperContainerStarterProcess", "-test.v=true")
23-26: Prefer t.Context() over context.Background() in tests.Aligns with noctx and ensures cleanup on test cancellation.
- ctx := context.Background() + ctx := t.Context()
102-104: Derive timeout context from t.Context().Keeps cancellation semantics tied to the test.
- ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + ctx, cancel := context.WithTimeout(t.Context(), 2*time.Second)
130-138: Pass t.Context() to Docker client ops.Removes noctx lint hits and improves cancellation.
- cli, err := NewDockerClientWithOpts(context.Background()) + cli, err := NewDockerClientWithOpts(t.Context()) - ctrs, err := cli.ContainerList(context.Background(), container.ListOptions{ + ctrs, err := cli.ContainerList(t.Context(), container.ListOptions{ All: true, Filters: f, })
147-149: Propagate test context here too.- nginxC, err := provider.ContainerFromType(context.Background(), ctrs[0]) + nginxC, err := provider.ContainerFromType(t.Context(), ctrs[0])
171-180: Use t.Context() in helper test.Brings the helper in line with noctx and test cancellation.
- ctx := context.Background() + ctx := t.Context()modules/elasticsearch/elasticsearch_test.go (2)
90-91: Use http.NoBody for clarity/consistency.Minor: prefer http.NoBody over nil as done elsewhere in the repo.
Apply this diff:
- req, err := http.NewRequestWithContext(ctx, http.MethodGet, esContainer.Settings.Address, nil) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, esContainer.Settings.Address, http.NoBody)
178-180: Use http.NoBody for clarity/consistency.Same here.
Apply this diff:
- req, err := http.NewRequestWithContext(ctx, http.MethodGet, ctr.Settings.Address, nil) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, ctr.Settings.Address, http.NoBody)modules/postgres/postgres_test.go (2)
242-245: Use QueryContext for SELECT statements (not ExecContext).ExecContext discards rows and is semantically incorrect for SELECT.
Apply this diff:
- result, err := db.ExecContext(ctx, "SELECT * FROM testdb;") - require.NoError(t, err) - require.NotNil(t, result) + rows, err := db.QueryContext(ctx, "SELECT * FROM testdb;") + require.NoError(t, err) + require.NotNil(t, rows) + defer rows.Close()
288-291: Use QueryContext for SELECT statements (not ExecContext).Same issue here.
Apply this diff:
- result, err := db.ExecContext(ctx, "SELECT * FROM testdb;") - require.NoError(t, err) - require.NotNil(t, result) + rows, err := db.QueryContext(ctx, "SELECT * FROM testdb;") + require.NoError(t, err) + require.NotNil(t, rows) + defer rows.Close()modules/k6/k6.go (1)
40-110: Consider exposing a context-aware WithRemoteTestScript variantWe now thread a
ctxintodownloadFileFromDescription, butWithRemoteTestScriptstill hard-codescontext.Background(), so callers cannot cancel or time out the download. A follow-up helper likeWithRemoteTestScriptContext(ctx, d)(with the current function delegating to it) would let consumers propagate their own context without breaking the existing API.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (45)
.golangci.yml(1 hunks)docker_auth_test.go(1 hunks)docker_test.go(10 hunks)docs/features/creating_container.md(1 hunks)examples/nginx/nginx_test.go(1 hunks)generic_test.go(1 hunks)internal/core/docker_host.go(2 hunks)logconsumer_test.go(6 hunks)modulegen/internal/tools/exec.go(2 hunks)modules/artemis/artemis_test.go(2 hunks)modules/chroma/chroma_test.go(1 hunks)modules/cockroachdb/examples_test.go(1 hunks)modules/compose/compose_local.go(1 hunks)modules/consul/consul_test.go(1 hunks)modules/databend/databend_test.go(2 hunks)modules/databend/examples_test.go(1 hunks)modules/dolt/dolt.go(1 hunks)modules/dolt/dolt_test.go(3 hunks)modules/dolt/examples_test.go(1 hunks)modules/elasticsearch/elasticsearch_test.go(2 hunks)modules/k6/k6.go(3 hunks)modules/localstack/examples_test.go(1 hunks)modules/mariadb/mariadb_test.go(4 hunks)modules/meilisearch/meilisearch_test.go(3 hunks)modules/mockserver/examples_test.go(1 hunks)modules/mssql/mssql_test.go(3 hunks)modules/mysql/examples_test.go(1 hunks)modules/mysql/mysql_test.go(3 hunks)modules/ollama/examples_test.go(2 hunks)modules/ollama/ollama_test.go(3 hunks)modules/openfga/examples_test.go(3 hunks)modules/opensearch/opensearch_test.go(1 hunks)modules/postgres/postgres_test.go(4 hunks)modules/pulsar/pulsar_test.go(1 hunks)modules/qdrant/qdrant_test.go(2 hunks)modules/registry/registry_test.go(5 hunks)modules/socat/examples_test.go(2 hunks)modules/socat/socat_test.go(3 hunks)modules/vault/vault_test.go(1 hunks)modules/vearch/vearch_test.go(2 hunks)modules/weaviate/weaviate_test.go(1 hunks)modules/yugabytedb/yugabytedb_test.go(2 hunks)testcontainers_test.go(1 hunks)wait/host_port_test.go(7 hunks)wait/http_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (20)
- modules/artemis/artemis_test.go
- modules/qdrant/qdrant_test.go
- modules/pulsar/pulsar_test.go
- modules/ollama/ollama_test.go
- logconsumer_test.go
- modules/socat/socat_test.go
- modules/mariadb/mariadb_test.go
- modules/registry/registry_test.go
- modules/consul/consul_test.go
- modules/socat/examples_test.go
- testcontainers_test.go
- modules/mysql/examples_test.go
- modules/vearch/vearch_test.go
- modules/mysql/mysql_test.go
- internal/core/docker_host.go
- modules/dolt/examples_test.go
- modules/chroma/chroma_test.go
- modules/weaviate/weaviate_test.go
- modules/meilisearch/meilisearch_test.go
- modules/compose/compose_local.go
🧰 Additional context used
🧬 Code graph analysis (5)
modules/openfga/examples_test.go (2)
modules/openfga/openfga.go (1)
Run(51-93)log/logger.go (1)
Printf(47-49)
modules/localstack/examples_test.go (1)
log/logger.go (1)
Printf(47-49)
modules/mockserver/examples_test.go (1)
log/logger.go (1)
Printf(47-49)
modules/ollama/examples_test.go (1)
log/logger.go (1)
Printf(47-49)
modules/cockroachdb/examples_test.go (1)
log/logger.go (1)
Printf(47-49)
🔇 Additional comments (22)
modules/cockroachdb/examples_test.go (1)
107-121: Context-aware queries LGTMGood call switching these SHOW queries to
QueryRowContextso they honor the surrounding context; matches the linter’s intent without altering behavior.examples/nginx/nginx_test.go (1)
24-31: Good context propagation and resource cleanupSwitching to
NewRequestWithContextwires the test’s context through the HTTP call, and the deferredresp.Body.Close()avoids leaking sockets. Nicely done.modules/ollama/examples_test.go (2)
89-102: Nicely switched to a context-aware POSTThe request now propagates
ctx, sets the proper header, and closes the response body to satisfynoctxand avoid leaks. Looks solid.
324-335: Great follow-up on the tags lookupCreating the GET request with
ctxand deferringresp.Body.Close()resolves the earlier leak and keeps the example compliant with the new linter.modules/yugabytedb/yugabytedb_test.go (1)
41-70: Context-aware ping looks goodSwitching to
PingContextkeeps the test in line with the new linter rules without altering behavior—looks clean.wait/host_port_test.go (1)
22-23: Restore Go 1.21 compatibility: replacet.Context()
testing.T.Context()landed in Go 1.22, but the module still targets Go 1.21. Using it here (and in the similar call sites below) breaks compilation for every supported toolchain. Please derive a context in a helper that works on Go 1.21—e.g. seed fromcontext.Background(), fold int.Deadline(), register the cancel witht.Cleanup()—and reuse it for eachListencall.+func contextForTest(t *testing.T) context.Context { + t.Helper() + + ctx := context.Background() + if deadline, ok := t.Deadline(); ok { + var cancel context.CancelFunc + ctx, cancel = context.WithDeadline(ctx, deadline) + t.Cleanup(cancel) + } + return ctx +} ... - ln := &net.ListenConfig{} - listener, err := ln.Listen(t.Context(), "tcp", "localhost:0") + ln := &net.ListenConfig{} + listener, err := ln.Listen(contextForTest(t), "tcp", "localhost:0")Apply the same fix to every new
t.Context()usage in this file.modules/opensearch/opensearch_test.go (1)
25-29: Context propagation was added correctly.Thanks for reworking the OpenSearch probe to use
NewRequestWithContextand the default client. This satisfiesnoctxwhile keeping behavior the same, and closing the body remains intact.modules/databend/examples_test.go (1)
77-79: LGTM: context-aware DB querySwitching to QueryRowContext propagates cancellation/timeouts correctly.
modules/localstack/examples_test.go (1)
239-247: LGTM: context-aware HTTP request and proper body closingUsing NewRequestWithContext, Do(req), and deferring resp.Body.Close() addresses noctx and leak risks.
Also applies to: 251-251
modules/mssql/mssql_test.go (1)
36-37: LGTM: use of PingContext/ExecContextContext variants are correctly applied across tests; behavior preserved with improved cancellation.
Also applies to: 39-43, 91-93, 94-98, 121-122
wait/http_test.go (1)
257-265: LGTM: context-aware requests and safe response handlingSwitching to NewRequestWithContext + client.Do(req) with error checks and deferred Close is correct and leak-free.
Also applies to: 311-319, 375-383
modules/dolt/dolt.go (1)
126-128: Good: switched to PingContext.Context-aware ping is correct.
modules/postgres/postgres_test.go (2)
131-138: LGTM: context-aware DDL/DML calls.Switching to ExecContext for CREATE/INSERT is correct.
338-342: Good: QueryContext with rows.Close.Proper API usage and resource cleanup.
modules/vault/vault_test.go (1)
63-65: LGTM: context-aware HTTP request and explicit no-body.Matches the noctx policy and improves readability.
modules/openfga/examples_test.go (4)
49-51: LGTM: thread ctx into Run.Aligns example with context-aware patterns.
62-63: LGTM: context-aware endpoint resolution.Consistent with noctx enforcement.
71-75: LGTM: NewRequestWithContext + error check.Good ergonomics and explicit body.
77-83: LGTM: Do(req) and defer Close.Proper lifecycle handling.
modules/mockserver/examples_test.go (1)
79-97: Context-aware HTTP request update looks solidSwitching to
http.NewRequestWithContextand deferring the body close reuses the existingctxand keeps cleanup tight—exactly what the linter wants without changing behavior.modules/databend/databend_test.go (1)
35-73: Database operations now respect the shared contextUsing the
*_Contextvariants wires the DB work into the same cancellation path as the container lifecycle, which is a clean win for these tests.modules/dolt/dolt_test.go (1)
35-155: Context-aware SQL calls look goodThanks for moving the ping, table creation, and prepared statement paths onto the
*_Contextmethods; that keeps the tests compliant while preserving the original intent.
| _, err = db.ExecContext(ctx, fmt.Sprintf("CREATE DATABASE IF NOT EXISTS %s;", c.database)) | ||
| if err != nil { | ||
| return fmt.Errorf("error creating database %s: %w", c.database, err) | ||
| } |
There was a problem hiding this comment.
Quote database identifier to avoid injection and reserved-word breakage.
Interpolating c.database unquoted is unsafe and can fail on special names.
Apply this diff:
- _, err = db.ExecContext(ctx, fmt.Sprintf("CREATE DATABASE IF NOT EXISTS %s;", c.database))
+ // Quote identifier; escape any backticks
+ _, err = db.ExecContext(ctx, "CREATE DATABASE IF NOT EXISTS `"+strings.ReplaceAll(c.database, "`", "``")+"`")Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In modules/dolt/dolt.go around lines 131 to 134, the code interpolates
c.database directly into the CREATE DATABASE SQL which risks SQL injection and
will break for reserved or special names; instead, sanitize and quote the
identifier by escaping any backticks in c.database (replace "`" with "``") and
wrap the result in backticks when building the SQL string (e.g., build CREATE
DATABASE IF NOT EXISTS `escapedName`;), then use that safe string in
db.ExecContext.
There was a problem hiding this comment.
@mmorel-35 wdyt? I suggest we resolve that in a separate PR.
There was a problem hiding this comment.
Indeed, It's better to separate topics
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| _, err = db.ExecContext(ctx, fmt.Sprintf("CREATE USER IF NOT EXISTS '%s' IDENTIFIED BY '%s';", c.username, c.password)) | ||
| if err != nil { | ||
| return fmt.Errorf("error creating user %s: %w", c.username, err) | ||
| } | ||
|
|
||
| q := fmt.Sprintf("GRANT ALL ON %s.* TO '%s';", c.database, c.username) | ||
| // grant user privileges | ||
| _, err = db.Exec(q) | ||
| _, err = db.ExecContext(ctx, q) | ||
| if err != nil { |
There was a problem hiding this comment.
Avoid string interpolation for CREATE USER/GRANT; use placeholders and quote identifiers.
Direct fmt.Sprintf with username/password is injection-prone; GRANT should quote the DB name.
Apply this diff:
- _, err = db.ExecContext(ctx, fmt.Sprintf("CREATE USER IF NOT EXISTS '%s' IDENTIFIED BY '%s';", c.username, c.password))
+ _, err = db.ExecContext(ctx, "CREATE USER IF NOT EXISTS ? IDENTIFIED BY ?", c.username, c.password)
if err != nil {
return fmt.Errorf("error creating user %s: %w", c.username, err)
}
- q := fmt.Sprintf("GRANT ALL ON %s.* TO '%s';", c.database, c.username)
- // grant user privileges
- _, err = db.ExecContext(ctx, q)
+ // grant user privileges
+ _, err = db.ExecContext(ctx, "GRANT ALL ON `"+strings.ReplaceAll(c.database, "`", "``")+"`.* TO ?", c.username)
if err != nil {
return fmt.Errorf("error creating user %s: %w", c.username, err)
}Follow-up: remove now-unused variable q at Line 143.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _, err = db.ExecContext(ctx, fmt.Sprintf("CREATE USER IF NOT EXISTS '%s' IDENTIFIED BY '%s';", c.username, c.password)) | |
| if err != nil { | |
| return fmt.Errorf("error creating user %s: %w", c.username, err) | |
| } | |
| q := fmt.Sprintf("GRANT ALL ON %s.* TO '%s';", c.database, c.username) | |
| // grant user privileges | |
| _, err = db.Exec(q) | |
| _, err = db.ExecContext(ctx, q) | |
| if err != nil { | |
| _, err = db.ExecContext(ctx, "CREATE USER IF NOT EXISTS ? IDENTIFIED BY ?", c.username, c.password) | |
| if err != nil { | |
| return fmt.Errorf("error creating user %s: %w", c.username, err) | |
| } | |
| // grant user privileges | |
| _, err = db.ExecContext(ctx, | |
| "GRANT ALL ON `"+strings.ReplaceAll(c.database, "`", "``")+"`.* TO ?", | |
| c.username, | |
| ) | |
| if err != nil { | |
| return fmt.Errorf("error creating user %s: %w", c.username, err) | |
| } |
🤖 Prompt for AI Agents
In modules/dolt/dolt.go around lines 138 to 146, the code builds SQL for CREATE
USER and GRANT via fmt.Sprintf which risks SQL injection and leaves an unused
variable q; replace string interpolation with parameterized ExecContext calls
for the CREATE USER statement (use placeholders for username and password), and
for the GRANT statement avoid injecting unquoted identifiers by quoting the
database identifier properly (e.g., wrap the database name with backticks or use
a proper identifier-quoting helper) while passing the username as a parameter;
then remove the now-unused variable q on line 143.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
modules/qdrant/qdrant_test.go (1)
56-60: Consider extracting a shared HTTP GET helperThe Web UI branch repeats the same request/
Do/status-check pattern used in the REST test. If more endpoints need this probe, a tiny helper (e.g.,assertGETOK(ctx, t, url)) could keep future touchpoints DRY. Totally optional, but it might ease the inevitable linter-driven copy/paste elsewhere.modules/databend/examples_test.go (1)
16-16: Prefer a timeout-bound context in examplesUsing a finite timeout avoids hanging examples if the DB is slow/unreachable.
Add:
ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel()and import time.
modules/consul/consul_test.go (1)
51-57: Use t.Context() to bind the request lifecycle to the testThis ensures cancellation on test timeout/failure.
Apply:
- req, err := http.NewRequestWithContext(ctx, http.MethodGet, "http://"+host, http.NoBody) + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, "http://"+host, http.NoBody)modules/weaviate/weaviate_test.go (1)
33-39: Bind HTTP request to t.Context()Keeps the request aligned with the subtest’s deadline/cancellation.
Apply:
- req, err := http.NewRequestWithContext(ctx, http.MethodGet, fmt.Sprintf("%s://%s", schema, host), http.NoBody) + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, fmt.Sprintf("%s://%s", schema, host), http.NoBody)modules/chroma/chroma_test.go (1)
28-35: Use tt.Context() for the HTTP requestPropagates the subtest’s timeout/cancel to the GET call.
Apply:
- req, err := http.NewRequestWithContext(ctx, http.MethodGet, restEndpoint+"/docs", http.NoBody) + req, err := http.NewRequestWithContext(tt.Context(), http.MethodGet, restEndpoint+"/docs", http.NoBody)modules/registry/registry_test.go (2)
30-35: Tie the unauthenticated GET to t.Context()Improves cancellation behavior for this test.
Apply:
- req, err := http.NewRequestWithContext(ctx, http.MethodGet, httpAddress+"/v2/_catalog", http.NoBody) + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, httpAddress+"/v2/_catalog", http.NoBody)
61-61: Use http.NoBody consistently for GET requestsMinor consistency/readability tweak; nil works, but http.NoBody is explicit.
Apply:
- req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, httpAddress+"/v2/_catalog", nil) + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, httpAddress+"/v2/_catalog", http.NoBody) - req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, httpAddress+"/v2/_catalog", nil) + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, httpAddress+"/v2/_catalog", http.NoBody) - req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, httpAddress+"/v2/_catalog", nil) + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, httpAddress+"/v2/_catalog", http.NoBody) - req, err := http.NewRequestWithContext(ctx, http.MethodGet, httpAddress+"/v2/_catalog", nil) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, httpAddress+"/v2/_catalog", http.NoBody) - req, err := http.NewRequestWithContext(ctx, http.MethodGet, regA.addr+"/v2/_catalog", nil) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, regA.addr+"/v2/_catalog", http.NoBody) - req, err = http.NewRequestWithContext(ctx, http.MethodGet, regB.addr+"/v2/_catalog", nil) + req, err = http.NewRequestWithContext(ctx, http.MethodGet, regB.addr+"/v2/_catalog", http.NoBody)Also applies to: 72-72, 85-85, 165-165, 213-213, 229-229
modules/compose/compose_local.go (2)
331-339: Unify ExecError.Command to the actual invoked argv.Use cmd.Args consistently instead of constructing “Starting command”/“Reading std …” slices. This simplifies error handling and keeps parity with what was executed.
- return ExecError{ - // add information about the CMD and arguments used - Command: execCmd, + return ExecError{ + // actual argv for the started cmd + Command: cmd.Args, StdoutOutput: stdout.Bytes(), StderrOutput: stderr.Bytes(), Error: err, Stderr: errStderr, Stdout: errStdout, } @@ - return ExecError{ - Command: execCmd, + return ExecError{ + Command: cmd.Args, StdoutOutput: stdout.Bytes(), StderrOutput: stderr.Bytes(), Error: err, Stderr: errStderr, Stdout: errStdout, }Also applies to: 355-360
396-396: Avoid hardcoding context.Background(); plumb a caller-provided ctx.Background removes the ability to cancel or apply deadlines on compose operations. Recommend adding context-aware variants and keeping the current API for back-compat.
Minimal approach:
- Add executeComposeWithContext(ctx, dc, args) and have executeCompose call it with context.Background().
- Optionally add InvokeWithContext/DownWithContext to LocalDockerCompose.
Example additions (outside this hunk):
// New context-aware helper; body matches executeCompose but calls execute(ctx, ...) func executeComposeWithContext(ctx context.Context, dc *LocalDockerCompose, args []string) ExecError { // ... same setup ... execErr := execute(ctx, pwd, environment, dc.Executable, cmds) // ... same error/strategy handling ... return execErr } // Back-compat wrapper func executeCompose(dc *LocalDockerCompose, args []string) ExecError { return executeComposeWithContext(context.Background(), dc, args) } // Optional user-facing helpers (non-breaking; LocalDockerCompose is deprecated anyway) func (dc *LocalDockerCompose) InvokeWithContext(ctx context.Context) ExecError { return executeComposeWithContext(ctx, dc, dc.Cmd) } func (dc *LocalDockerCompose) DownWithContext(ctx context.Context) ExecError { return executeComposeWithContext(ctx, dc, []string{"down", "--remove-orphans", "--volumes"}) }If you’d like, I can prep a follow-up patch threading ctx through call sites.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (45)
.golangci.yml(1 hunks)docker_auth_test.go(5 hunks)docker_test.go(10 hunks)docs/features/creating_container.md(2 hunks)examples/nginx/nginx_test.go(1 hunks)generic_test.go(1 hunks)internal/core/docker_host.go(1 hunks)logconsumer_test.go(6 hunks)modulegen/internal/tools/exec.go(2 hunks)modules/artemis/artemis_test.go(2 hunks)modules/chroma/chroma_test.go(1 hunks)modules/cockroachdb/examples_test.go(1 hunks)modules/compose/compose_local.go(2 hunks)modules/consul/consul_test.go(1 hunks)modules/databend/databend_test.go(2 hunks)modules/databend/examples_test.go(1 hunks)modules/dolt/dolt.go(1 hunks)modules/dolt/dolt_test.go(3 hunks)modules/dolt/examples_test.go(1 hunks)modules/elasticsearch/elasticsearch_test.go(2 hunks)modules/k6/k6.go(3 hunks)modules/localstack/examples_test.go(1 hunks)modules/mariadb/mariadb_test.go(4 hunks)modules/meilisearch/meilisearch_test.go(3 hunks)modules/mockserver/examples_test.go(1 hunks)modules/mssql/mssql_test.go(3 hunks)modules/mysql/examples_test.go(1 hunks)modules/mysql/mysql_test.go(3 hunks)modules/ollama/examples_test.go(2 hunks)modules/ollama/ollama_test.go(5 hunks)modules/openfga/examples_test.go(3 hunks)modules/opensearch/opensearch_test.go(1 hunks)modules/postgres/postgres_test.go(4 hunks)modules/pulsar/pulsar_test.go(1 hunks)modules/qdrant/qdrant_test.go(2 hunks)modules/registry/registry_test.go(5 hunks)modules/socat/examples_test.go(2 hunks)modules/socat/socat_test.go(3 hunks)modules/vault/vault_test.go(1 hunks)modules/vearch/vearch_test.go(2 hunks)modules/weaviate/weaviate_test.go(1 hunks)modules/yugabytedb/yugabytedb_test.go(2 hunks)testcontainers_test.go(1 hunks)wait/host_port_test.go(7 hunks)wait/http_test.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (23)
- modules/socat/examples_test.go
- modules/pulsar/pulsar_test.go
- modules/socat/socat_test.go
- modules/k6/k6.go
- modules/mysql/mysql_test.go
- modules/elasticsearch/elasticsearch_test.go
- modules/vearch/vearch_test.go
- modules/mockserver/examples_test.go
- testcontainers_test.go
- modules/mariadb/mariadb_test.go
- modules/openfga/examples_test.go
- modules/cockroachdb/examples_test.go
- modules/dolt/examples_test.go
- modules/opensearch/opensearch_test.go
- modulegen/internal/tools/exec.go
- internal/core/docker_host.go
- modules/mssql/mssql_test.go
- modules/localstack/examples_test.go
- .golangci.yml
- generic_test.go
- modules/meilisearch/meilisearch_test.go
- modules/databend/databend_test.go
- docker_test.go
🧰 Additional context used
🧬 Code graph analysis (4)
wait/http_test.go (2)
generic.go (1)
Run(122-149)testing.go (1)
CleanupContainer(91-97)
docker_auth_test.go (2)
modules/registry/registry.go (1)
Run(275-316)generic.go (1)
Run(122-149)
modules/registry/registry_test.go (1)
modules/registry/registry.go (1)
Run(275-316)
modules/ollama/ollama_test.go (3)
modules/ollama/ollama.go (2)
Run(76-115)OllamaContainer(21-23)testing.go (1)
CleanupContainer(91-97)options.go (1)
WithReuseByName(129-138)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: lint (modules/dind) / lint: modules/dind
- GitHub Check: lint (modules/neo4j) / lint: modules/neo4j
- GitHub Check: lint (modules/cockroachdb) / lint: modules/cockroachdb
- GitHub Check: lint (modules/mssql) / lint: modules/mssql
- GitHub Check: lint (modules/artemis) / lint: modules/artemis
- GitHub Check: lint (modules/openfga) / lint: modules/openfga
- GitHub Check: lint (modules/nats) / lint: modules/nats
- GitHub Check: lint (modulegen) / lint: modulegen
- GitHub Check: lint (modules/influxdb) / lint: modules/influxdb
- GitHub Check: lint (modules/elasticsearch) / lint: modules/elasticsearch
- GitHub Check: lint (modules/dolt) / lint: modules/dolt
- GitHub Check: lint (examples/nginx) / lint: examples/nginx
- GitHub Check: lint (modules/aerospike) / lint: modules/aerospike
- GitHub Check: lint (modules/pulsar) / lint: modules/pulsar
- GitHub Check: lint (modules/databend) / lint: modules/databend
- GitHub Check: lint (modules/memcached) / lint: modules/memcached
- GitHub Check: lint (modules/dynamodb) / lint: modules/dynamodb
- GitHub Check: lint (modules/arangodb) / lint: modules/arangodb
- GitHub Check: Analyze (go)
🔇 Additional comments (10)
modules/mysql/examples_test.go (1)
83-94: Context-aware DB calls look good.
Switching to the Context variants keeps the example behavior while satisfying the new linter requirement.examples/nginx/nginx_test.go (1)
24-30: Context propagation + body close look goodConstructing the request with the test context and closing the response body resolves the earlier noctx complaint and avoids leaking connections. Nicely done.
modules/qdrant/qdrant_test.go (1)
29-33: Nice context-aware REST probeThanks for swapping the plain
http.Getfor an explicitNewRequestWithContext+Do; this keeps the request cancellable and satisfies the new linter without changing the surrounding assertions.wait/host_port_test.go (1)
22-23: Compile break:testing.T.Contextrequires Go 1.22
(*testing.T).Contextlanded in Go 1.22, while our module still targets Go 1.21, so this file (and every similar call site below) will fail to compile until we either bump the toolchain or stop callingt.Context(). Please root a cancellable context offcontext.Background()/t.Deadline()instead and reuse it. Suggested sketch:ln := &net.ListenConfig{} - listener, err := ln.Listen(t.Context(), "tcp", "localhost:0") + ctx := testContext(t) + listener, err := ln.Listen(ctx, "tcp", "localhost:0")Add once in this file and reuse:
func testContext(t *testing.T) context.Context { t.Helper() ctx := context.Background() if dl, ok := t.Deadline(); ok { ctx, cancel := context.WithDeadline(ctx, dl) t.Cleanup(cancel) return ctx } ctx, cancel := context.WithCancel(ctx) t.Cleanup(cancel) return ctx }(Apply the same swap for every new
t.Context()usage in the file to keep the noctx linter happy without dropping Go 1.21 compatibility.)modules/dolt/dolt.go (1)
131-146: Fix SQL injection risks in CREATE DATABASE/USER flows.We still interpolate
c.database,c.username, andc.passworddirectly into SQL strings. An attacker supplying values with quotes or backticks can break out of the intended command — e.g., a crafted database name causes arbitrary SQL execution or fails on reserved words. We need to quote identifiers and use placeholders for user credentials.- _, err = db.ExecContext(ctx, fmt.Sprintf("CREATE DATABASE IF NOT EXISTS %s;", c.database)) + safeDatabase := "`" + strings.ReplaceAll(c.database, "`", "``") + "`" + _, err = db.ExecContext(ctx, "CREATE DATABASE IF NOT EXISTS "+safeDatabase+";") @@ - _, err = db.ExecContext(ctx, fmt.Sprintf("CREATE USER IF NOT EXISTS '%s' IDENTIFIED BY '%s';", c.username, c.password)) + _, err = db.ExecContext(ctx, "CREATE USER IF NOT EXISTS ? IDENTIFIED BY ?", c.username, c.password) @@ - q := fmt.Sprintf("GRANT ALL ON %s.* TO '%s';", c.database, c.username) - _, err = db.ExecContext(ctx, q) + _, err = db.ExecContext(ctx, "GRANT ALL ON "+safeDatabase+".* TO ?", c.username)modules/databend/examples_test.go (1)
76-79: Switch to QueryRowContext — LGTMThe context-aware query is correct and aligns with the linter changes.
logconsumer_test.go (2)
98-116: Close the response bodies before issuing more probes.We call
http.DefaultClient.Do(req)twice and drop the returned*http.Response, so those sockets/bodies never get closed. That leaks connections in a hot looped test. Capture the response and close it (with error handling) before moving on.- _, err = http.DefaultClient.Do(req) - require.NoError(t, err) + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) req, err = http.NewRequestWithContext(ctx, http.MethodGet, ep+"/stdout?echo=there", http.NoBody) require.NoError(t, err) - _, err = http.DefaultClient.Do(req) - require.NoError(t, err) + resp, err = http.DefaultClient.Do(req) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) req, err = http.NewRequestWithContext(ctx, http.MethodGet, ep+"/stdout?echo="+lastMessage, http.NoBody) require.NoError(t, err) - resp, err := http.DefaultClient.Do(req) + resp, err = http.DefaultClient.Do(req) require.NoError(t, err) require.NoError(t, resp.Body.Close())
167-185: Also close the bodies in this test variant.Same pattern here—the first and last requests drop the response entirely, so the TCP connection never gets reclaimed. Grab the response and close it just like the middle call.
- _, err = http.DefaultClient.Do(req) - require.NoError(t, err) + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) req, err = http.NewRequestWithContext(ctx, http.MethodGet, ep+"/stderr?echo=this-is-stderr", http.NoBody) require.NoError(t, err) - resp, err := http.DefaultClient.Do(req) + resp, err = http.DefaultClient.Do(req) require.NoError(t, err) require.NoError(t, resp.Body.Close()) req, err = http.NewRequestWithContext(ctx, http.MethodGet, ep+"/stdout?echo="+lastMessage, http.NoBody) require.NoError(t, err) - _, err = http.DefaultClient.Do(req) - require.NoError(t, err) + resp, err = http.DefaultClient.Do(req) + require.NoError(t, err) + require.NoError(t, resp.Body.Close())wait/http_test.go (1)
258-262: Nice to see the HTTP request using the test context.Wiring the request through
ctxmeans these tests now bail out promptly when the test context is canceled instead of hanging on flaky environments. Good call.modules/compose/compose_local.go (1)
297-301: Verified no remaining exec.Command calls without context — All Go files (excluding vendor) use exec.CommandContext after the change.
mmorel-35
force-pushed
the
noctx
branch
7 times, most recently
from
a9c07e3 to
2b5f826
Compare
There was a problem hiding this comment.
Having done a partial review, while I agree with the goal, there's some challenges with the current approach.
In the tests it's adding a lot of boilerplate without much benefit in most places. We could solve that with a helper, but it is worth it?
For the library side in the first few places I noticed we're not actually fixing the issue, just hiding it by using context.Background() which is kind of the anti pattern to what the linter is trying to achieve.
I would suggest if we're going to enable this linter, we should actually fix the API's so they all pass in a context.
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, nginxC.URI, http.NoBody) | ||
| require.NoError(t, err) | ||
| resp, err := http.DefaultClient.Do(req) |
There was a problem hiding this comment.
question: this is adding a lot of boiler plate, should we a help to eliminate that?
| // running the `go mod tidy` command in the given directory. | ||
| func GoModTidy(cmdDir string) error { | ||
| if err := runCommand(cmdDir, "go", "mod", "tidy"); err != nil { | ||
| if err := runCommand(context.Background(), cmdDir, "go", "mod", "tidy"); err != nil { |
There was a problem hiding this comment.
suggestion: This is the anti pattern to what the context checker is trying to achieve, if we're going to do this we should expose it and use it properly.
More below
|
Thanks for coming with more eyes @stevenh. I think the changes make sense, although I also considered that sending small chunks with the API updates first, and then enable the linter, would be of interest. |
Signed-off-by: Matthieu MOREL <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
modules/socat/socat_test.go (2)
122-133: Missingdefer resp.Body.Close()statement.The response body should be closed after the HTTP request. Add the defer statement after line 126.
resp, err := httpClient.Do(req) require.NoError(t, err) require.Equal(t, 200, resp.StatusCode) +defer resp.Body.Close() body, err := io.ReadAll(resp.Body)
225-237: Avoiddeferinside loop - potential resource leak.Using
deferinside a loop will accumulate all deferred calls until the function exits, potentially keeping multiple response bodies open. Consider closing the body immediately after reading.resp, err := httpClient.Do(req) require.NoError(t, err) require.Equal(t, 200, resp.StatusCode) -defer resp.Body.Close() body, err := io.ReadAll(resp.Body) require.NoError(t, err) require.Equal(t, "PONG", string(body)) +resp.Body.Close()Alternatively, extract the request logic into a helper function where
deferwould be appropriate.
🧹 Nitpick comments (10)
modules/artemis/artemis_test.go (4)
77-85: Always close response bodies via defer/t.Cleanup to avoid leaks on assertion failuresDefer the close immediately after a successful Do, and drop the explicit Close assertion.
- res, err := http.DefaultClient.Do(req) - require.NoError(t, err, "failed to access console") - - require.Equal(t, http.StatusOK, res.StatusCode, "failed to access console") - require.NoError(t, res.Body.Close()) + res, err := http.DefaultClient.Do(req) + require.NoError(t, err, "failed to access console") + defer func() { _ = res.Body.Close() }() + require.Equal(t, http.StatusOK, res.StatusCode, "failed to access console")
132-149: Defer body close before decoding to ensure cleanup even if decoding/assertions failPlace the defer right after the request succeeds; remove the trailing Close.
- r, err := http.DefaultClient.Do(req) - require.NoError(t, err, "failed to request QueueNames") + r, err := http.DefaultClient.Do(req) + require.NoError(t, err, "failed to request QueueNames") + defer func() { _ = r.Body.Close() }() @@ - require.NoError(t, r.Body.Close())
19-19: Prefer per-test contextTie cancellations to the test deadline by using t.Context().
- ctx := context.Background() + ctx := t.Context()
115-121: Avoid ticker leak; use time.After for one-shot timeoutA Ticker spins a goroutine and should be stopped; a one-shot timeout is simpler here.
- ticker := time.NewTicker(10 * time.Second) - select { - case <-ticker.C: + select { + case <-time.After(10 * time.Second): t.Fatal("timed out waiting for message") case msg := <-sub.C: require.Equal(t, "test", string(msg.Body), "received unexpected message") }modules/opensearch/opensearch_test.go (2)
15-15: Use t.Context() instead of context.Background() in testsLeverage the test’s deadline/cancellation.
- ctx := context.Background() + ctx := t.Context()
21-31: Derive a per-subtest context for request/address callsBind the HTTP request and Address() to the subtest’s lifecycle.
t.Run("Connect to Address", func(t *testing.T) { - address, err := ctr.Address(ctx) + tctx := t.Context() + address, err := ctr.Address(tctx) require.NoError(t, err) - req, err := http.NewRequestWithContext(ctx, http.MethodGet, address, nil) + req, err := http.NewRequestWithContext(tctx, http.MethodGet, address, nil) require.NoError(t, err) resp, err := http.DefaultClient.Do(req) require.NoError(t, err) defer resp.Body.Close() })modules/compose/compose_local.go (1)
396-396: Avoid context.Background() at the call site; thread a caller-provided ctxexecuteCompose hardcodes context.Background(), which defeats cancellation from callers. If changing public APIs is undesirable (deprecated type), consider:
- Adding an optional context to LocalDockerCompose (e.g., a field set via a deprecated option) and use it here; or
- Threading ctx through the deprecated methods (Down/Invoke/determineVersion) without breaking existing signatures by providing new ctx-accepting variants and delegating.
This keeps behavior consistent with the intent of noctx.
modules/vault/vault_test.go (3)
61-66: Prefer t.Context() for subtest-scoped cancellation.Use the per-test context so the HTTP request is canceled if the subtest ends early or times out.
Apply this minimal change:
t.Run("From HTTP request", func(t *testing.T) { // httpRead { - request, err := http.NewRequestWithContext(ctx, http.MethodGet, hostAddress+"/v1/secret/data/test1", http.NoBody) + ctx := t.Context() + request, err := http.NewRequestWithContext(ctx, http.MethodGet, hostAddress+"/v1/secret/data/test1", http.NoBody) require.NoError(t, err) request.Header.Add("X-Vault-Token", token)
67-71: Optionally assert HTTP status code for clearer failures.Adding a status check makes failures more explicit.
response, err := http.DefaultClient.Do(request) // } require.NoError(t, err) +require.Equal(t, http.StatusOK, response.StatusCode) defer response.Body.Close()
25-25: Use t.Context() instead of context.Background()
Go 1.24 (go.mod:go 1.24.0) supportst.Context(), and most tests already use it. Replace at lines 25, 47, and 79 inmodules/vault/vault_test.go.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (45)
.golangci.yml(1 hunks)docker_auth_test.go(5 hunks)docker_test.go(10 hunks)docs/features/creating_container.md(2 hunks)examples/nginx/nginx_test.go(1 hunks)generic_test.go(1 hunks)internal/core/docker_host.go(1 hunks)logconsumer_test.go(6 hunks)modulegen/internal/tools/exec.go(2 hunks)modules/artemis/artemis_test.go(2 hunks)modules/chroma/chroma_test.go(1 hunks)modules/cockroachdb/examples_test.go(1 hunks)modules/compose/compose_local.go(2 hunks)modules/consul/consul_test.go(2 hunks)modules/databend/databend_test.go(2 hunks)modules/databend/examples_test.go(1 hunks)modules/dolt/dolt.go(1 hunks)modules/dolt/dolt_test.go(3 hunks)modules/dolt/examples_test.go(1 hunks)modules/elasticsearch/elasticsearch_test.go(2 hunks)modules/k6/k6.go(3 hunks)modules/localstack/examples_test.go(1 hunks)modules/mariadb/mariadb_test.go(4 hunks)modules/meilisearch/meilisearch_test.go(3 hunks)modules/mockserver/examples_test.go(1 hunks)modules/mssql/mssql_test.go(3 hunks)modules/mysql/examples_test.go(1 hunks)modules/mysql/mysql_test.go(3 hunks)modules/ollama/examples_test.go(2 hunks)modules/ollama/ollama_test.go(6 hunks)modules/openfga/examples_test.go(3 hunks)modules/opensearch/opensearch_test.go(1 hunks)modules/postgres/postgres_test.go(4 hunks)modules/pulsar/pulsar_test.go(1 hunks)modules/qdrant/qdrant_test.go(2 hunks)modules/registry/registry_test.go(5 hunks)modules/socat/examples_test.go(2 hunks)modules/socat/socat_test.go(3 hunks)modules/vault/vault_test.go(1 hunks)modules/vearch/vearch_test.go(2 hunks)modules/weaviate/weaviate_test.go(2 hunks)modules/yugabytedb/yugabytedb_test.go(2 hunks)testcontainers_test.go(1 hunks)wait/host_port_test.go(7 hunks)wait/http_test.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (25)
- modules/consul/consul_test.go
- modules/ollama/examples_test.go
- modules/dolt/examples_test.go
- modules/elasticsearch/elasticsearch_test.go
- modules/chroma/chroma_test.go
- modules/mariadb/mariadb_test.go
- wait/host_port_test.go
- .golangci.yml
- modules/yugabytedb/yugabytedb_test.go
- docs/features/creating_container.md
- internal/core/docker_host.go
- modules/meilisearch/meilisearch_test.go
- modules/databend/databend_test.go
- modules/dolt/dolt.go
- modules/mockserver/examples_test.go
- modules/pulsar/pulsar_test.go
- wait/http_test.go
- docker_test.go
- modules/cockroachdb/examples_test.go
- modules/mysql/examples_test.go
- modules/k6/k6.go
- modules/localstack/examples_test.go
- modules/ollama/ollama_test.go
- docker_auth_test.go
- modules/mysql/mysql_test.go
🧰 Additional context used
🧬 Code graph analysis (4)
modules/openfga/examples_test.go (1)
modules/openfga/openfga.go (1)
Run(51-93)
modules/socat/examples_test.go (1)
log/logger.go (1)
Printf(47-49)
modules/weaviate/weaviate_test.go (2)
modules/weaviate/weaviate.go (1)
Run(29-67)testing.go (1)
CleanupContainer(91-97)
modules/registry/registry_test.go (2)
modules/registry/registry.go (1)
Run(275-316)testing.go (1)
CleanupContainer(91-97)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: lint (modules/mysql) / lint: modules/mysql
- GitHub Check: lint (modules/inbucket) / lint: modules/inbucket
- GitHub Check: lint (modules/pinecone) / lint: modules/pinecone
- GitHub Check: lint (modules/k3s) / lint: modules/k3s
- GitHub Check: lint (modules/cassandra) / lint: modules/cassandra
- GitHub Check: lint (modules/scylladb) / lint: modules/scylladb
- GitHub Check: lint (modules/mariadb) / lint: modules/mariadb
- GitHub Check: lint (modules/weaviate) / lint: modules/weaviate
- GitHub Check: lint (modules/cockroachdb) / lint: modules/cockroachdb
- GitHub Check: lint (modules/redpanda) / lint: modules/redpanda
- GitHub Check: lint (modules/registry) / lint: modules/registry
- GitHub Check: lint (modules/chroma) / lint: modules/chroma
- GitHub Check: lint (modules/toxiproxy) / lint: modules/toxiproxy
- GitHub Check: Analyze (go)
🔇 Additional comments (30)
modules/mssql/mssql_test.go (3)
36-44: Context-aware ping/exec looks goodSwitching to
PingContextandExecContexthere keeps the call sites aligned with the new linter requirements without altering behavior. 👍
91-99: Nice consistencyThanks for mirroring the context-aware database calls in this variant of the test too.
121-123: LGTMThis
PingContextupgrade completes the trio—no issues spotted.modules/socat/examples_test.go (2)
70-76: Good call switching toNewRequestWithContext.
This keeps the example inline with thenoctxpolicy and bails out early if the request cannot be constructed—exactly what we want here.
177-183: Context-aware loop looks solid.
Building each request with the sharedctxand surfacing construction errors ensures the multi-target example now respects cancellation and passes the noctx linter cleanly.examples/nginx/nginx_test.go (1)
27-32: Defer closing the response bodyRight now the body is only closed after the status assertion, so if
require.Equalfails we leak the HTTP connection. Please defer (ort.Cleanup) the close immediately after theDosucceeds, and keep the final status assertion unchanged. Example:resp, err := http.DefaultClient.Do(req) require.NoError(t, err) - require.Equal(t, http.StatusOK, resp.StatusCode) - require.NoError(t, resp.Body.Close()) + defer func() { + require.NoError(t, resp.Body.Close()) + }() + + require.Equal(t, http.StatusOK, resp.StatusCode)modules/vearch/vearch_test.go (1)
14-14: Restore compatibility with our current Go toolchain
testing.Tonly gained aContext()method in Go 1.24, so this line won’t compile on the Go versions we still build and test against unless the toolchain bump ships at the same time. Please keep deriving the test context from a supported parent (for example, keepcontext.WithTimeout(context.Background(), …)), or include the Go 1.24+ upgrade in this PR before callingt.Context(). (betterstack.com)- ctx := t.Context() + ctx := context.Background()testcontainers_test.go (1)
50-61: LGTM: subprocesses now respect test cancellationUsing t.Context() with exec.CommandContext is correct and consistent.
modules/compose/compose_local.go (1)
297-301: Good: command now uses exec.CommandContext(ctx, …)This enables cancellation; no further issues spotted here.
modulegen/internal/tools/exec.go (1)
12-12: Expose context-aware exported variants instead of hardcoding context.Background()Hardcoding Background at call sites undermines cancellation. Provide ctx-accepting exported helpers and have existing funcs delegate to them to avoid breaking changes. This aligns with the prior feedback.
import ( "context" "fmt" "os/exec" ) // GoModTidy synchronizes the dependencies for a module or example, // running the `go mod tidy` command in the given directory. func GoModTidy(cmdDir string) error { - if err := runCommand(context.Background(), cmdDir, "go", "mod", "tidy"); err != nil { + return GoModTidyContext(context.Background(), cmdDir) +} + +// GoModTidyContext is the context-aware variant of GoModTidy. +func GoModTidyContext(ctx context.Context, cmdDir string) error { + if err := runCommand(ctx, cmdDir, "go", "mod", "tidy"); err != nil { return fmt.Errorf("mod tidy dependencies: %w", err) } return nil } // GoVet checks the generated code for errors, // running the `go vet ./...` command in the given directory. func GoVet(cmdDir string) error { - if err := runCommand(context.Background(), cmdDir, "go", "vet", "./..."); err != nil { + return GoVetContext(context.Background(), cmdDir) +} + +// GoVetContext is the context-aware variant of GoVet. +func GoVetContext(ctx context.Context, cmdDir string) error { + if err := runCommand(ctx, cmdDir, "go", "vet", "./..."); err != nil { return fmt.Errorf("go vet generated code: %w", err) } return nil } // MakeLint runs the `make lint` command in the given directory. func MakeLint(cmdDir string) error { - if err := runCommand(context.Background(), cmdDir, "make", "lint"); err != nil { + return MakeLintContext(context.Background(), cmdDir) +} + +// MakeLintContext is the context-aware variant of MakeLint. +func MakeLintContext(ctx context.Context, cmdDir string) error { + if err := runCommand(ctx, cmdDir, "make", "lint"); err != nil { return fmt.Errorf("make lint module: %w", err) } return nil }Also applies to: 21-21, 29-29
generic_test.go (1)
155-159: LGTM: createReuseContainerInSubprocess uses exec.CommandContext(t.Context(), …)Subprocess now obeys test timeouts/cancellation.
modules/vault/vault_test.go (1)
63-65: LGTM: context-aware request + error check.Using http.NewRequestWithContext and http.NoBody addresses noctx and improves clarity. Nicely done.
logconsumer_test.go (4)
101-108: Response bodies still not closed for first two requests.Lines 101-102 and 107-108 ignore the returned
*http.Responseand don't closeresp.Body, which can leak connections and resources.
227-240: LGTM! Proper response body handling.Both HTTP requests correctly capture the response and close the body, preventing connection leaks.
443-463: LGTM! Consistent response body management.All three HTTP requests properly capture the response and close the body, demonstrating consistent resource management throughout the test.
597-649: LGTM! Excellent response body handling across all requests.All four HTTP requests consistently capture the response and close the body properly. This demonstrates good resource management in a complex test scenario with multiple containers and cancellation.
modules/qdrant/qdrant_test.go (2)
29-32: LGTM! Proper context propagation for HTTP request.The change correctly replaces the direct HTTP GET call with a context-aware request pattern, aligning with the noctx linter requirements.
56-61: LGTM! Consistent context propagation pattern.The Web UI test follows the same correct pattern as the REST endpoint test, properly using context-aware HTTP requests.
modules/openfga/examples_test.go (3)
49-50: LGTM! Proper context initialization and usage.Creating a local context variable and reusing it throughout the function is a good practice that aligns with the PR's context propagation pattern.
62-62: LGTM! Consistent context usage.Correctly uses the local context variable instead of creating a new background context.
71-82: LGTM! Complete context-aware HTTP request implementation.The changes properly implement the context-aware HTTP request pattern with appropriate error handling and resource cleanup via
defer resp.Body.Close().modules/socat/socat_test.go (1)
64-75: LGTM! Proper context-aware HTTP request with correct defer placement.The changes correctly implement the context-aware HTTP request pattern. The
defer resp.Body.Close()is properly placed after the error check and before reading the body.modules/weaviate/weaviate_test.go (3)
21-21: LGTM! Proper test context usage.Using
t.Context()is the preferred pattern for tests as it provides proper test lifecycle management and cancellation support.
26-27: LGTM! Consistent subtest context pattern.Each subtest properly creates its own context from
t.Context(), ensuring proper test isolation and lifecycle management.Also applies to: 44-45, 62-63
32-37: LGTM! Well-implemented context-aware HTTP request.The HTTP request correctly uses the subtest's context and follows the established pattern with proper error handling.
modules/registry/registry_test.go (5)
22-35: LGTM! Clean context-aware implementation.The test properly uses
t.Context()and implements the context-aware HTTP request pattern with appropriate error handling.
42-43: LGTM! Consistent test context usage.Properly establishes test context for the authenticated registry test.
61-68: LGTM! Proper subtest context usage.The subtest correctly uses
t.Context()for its HTTP request.
213-219: LGTM! Context-aware request with explicit body closure.The test correctly implements the context-aware HTTP request pattern. The explicit
resp.Body.Close()is appropriate here given the test's structure.
229-235: LGTM! Consistent HTTP request pattern.Maintains the same correct context-aware pattern for the second registry test.
| _, err = http.DefaultClient.Do(req) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
Close response bodies for consistency.
Lines 170-171 and 183-184 ignore the returned *http.Response without closing resp.Body. For consistency with the pattern established in lines 176-179, capture and close these responses as well.
Apply this diff:
- _, err = http.DefaultClient.Do(req)
- require.NoError(t, err)
+ resp, err := http.DefaultClient.Do(req)
+ require.NoError(t, err)
+ require.NoError(t, resp.Body.Close())Also apply to lines 183-184:
- _, err = http.DefaultClient.Do(req)
+ resp, err = http.DefaultClient.Do(req)
require.NoError(t, err)
+ require.NoError(t, resp.Body.Close())Also applies to: 183-184
🤖 Prompt for AI Agents
In logconsumer_test.go around lines 170-171 and 183-184, the test calls
http.DefaultClient.Do(req) and ignores the returned *http.Response without
closing resp.Body; update both call sites to assign the response (e.g., resp,
err := http.DefaultClient.Do(req)), assert no error as before, and ensure you
defer resp.Body.Close() (or call resp.Body.Close() immediately after checking
err) to match the pattern used at lines 176-179 so response bodies are always
closed for consistency and to avoid resource leaks.
3 participants
What does this PR do?
Enables and fixes issues from noctx linter
Why is it important?
Related issues