feat: TASK-02 GitHub Actions CI#5
Conversation
- Add .github/workflows/ci.yml with lint-and-test and integration-test jobs - Add internal/testutil/containers.go with MustStartContainer helper - Add internal/testutil/smoke_integration_test.go to verify Testcontainers Cloud Co-authored-by: German Arutyunov <gaarutyunov@users.noreply.github.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an internal test helper to start Testcontainers ( Changes
Sequence Diagram(s)sequenceDiagram
participant GH as "GitHub Actions"
participant Lint as "lint-and-test job"
participant Int as "integration-test job"
participant Helper as "testutil.MustStartContainer"
participant TCCloud as "Testcontainers Cloud"
participant Container as "nginx container"
participant Test as "Integration Test (http.Client)"
GH->>Lint: start lint-and-test
Lint-->>GH: succeed
GH->>Int: start integration-test (needs lint-and-test)
Int->>Helper: MustStartContainer(ctx, req)
Helper->>TCCloud: request container start (nginx:alpine)
TCCloud->>Container: provision & start
Container-->>Helper: started, host & mapped port
Helper-->>Int: return container
Int->>Test: perform HTTP GET http://host:port/
Test->>Container: GET /
Container-->>Test: 200 OK
Int->>Container: c.Terminate() (via t.Cleanup)
Container-->>TCCloud: stop & remove
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (1)
internal/testutil/smoke_integration_test.go (1)
26-34: UseMustStartContainerhere instead of reimplementing startup/cleanup.Lines [26-34] duplicate logic already centralized in
internal/testutil/containers.go. Reusing the helper avoids drift and keeps cleanup/error behavior consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/testutil/smoke_integration_test.go` around lines 26 - 34, Replace the manual GenericContainer startup/cleanup with the centralized helper MustStartContainer from internal/testutil/containers.go: remove the explicit testcontainers.GenericContainer call, error check, and t.Cleanup Terminate call, and instead call MustStartContainer (passing ctx, t and the ContainerRequest/req) to obtain the container; this reuses the existing startup/error-handling/cleanup behavior and prevents duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/testutil/containers.go`:
- Line 14: The helper MustStartContainer currently has signature
MustStartContainer(t *testing.T, ctx context.Context, req
testcontainers.ContainerRequest); change it so context.Context is the first
parameter (e.g., MustStartContainer(ctx context.Context, t *testing.T, req
testcontainers.ContainerRequest)), update all call sites and any tests to pass
ctx first, and ensure any documentation or usages that rely on the old order are
adjusted; keep the function name and behavior unchanged besides the parameter
reorder.
In `@internal/testutil/smoke_integration_test.go`:
- Around line 45-49: Replace the plain http.Get call with a context-bounded
request: create a timeout context (e.g., ctx, cancel := context.WithTimeout(ctx,
10*time.Second); defer cancel()), build the request with
http.NewRequestWithContext(ctx, "GET", url, nil) and execute it with
http.DefaultClient.Do(req) instead of http.Get; update error handling around
resp, err (from http.Get) and ensure resp.Body.Close() stays deferred; remove
the nolint noctx comment.
---
Nitpick comments:
In `@internal/testutil/smoke_integration_test.go`:
- Around line 26-34: Replace the manual GenericContainer startup/cleanup with
the centralized helper MustStartContainer from internal/testutil/containers.go:
remove the explicit testcontainers.GenericContainer call, error check, and
t.Cleanup Terminate call, and instead call MustStartContainer (passing ctx, t
and the ContainerRequest/req) to obtain the container; this reuses the existing
startup/error-handling/cleanup behavior and prevents duplication.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 83eb35fb-b11f-4a16-a68a-d78baadbe272
📒 Files selected for processing (2)
internal/testutil/containers.gointernal/testutil/smoke_integration_test.go
Removed concurrency setting for Testcontainers Cloud Client.
|
@claude fix org to gaarutyunov Error: internal/testutil/containers.go:9:2: missing go.sum entry for module providing package github.com/testcontainers/testcontainers-go (imported by github.com/your-org/mcp-anything/internal/testutil); to add: |
|
Claude finished @gaarutyunov's task in 5m 33s —— View job Fixing module path and go.sum entry
What was doneFixed Blocker: missing
|
|
@claude I added the tools |
|
Claude finished @gaarutyunov's task in 2m 24s —— View job Fixing module path and code review issues
Still needed:
|
- Change module path from github.com/your-org/mcp-anything to github.com/gaarutyunov/mcp-anything - Update .golangci.yml local-prefixes to match - Fix context.Context parameter order in MustStartContainer (ctx first, per conventions) - Replace http.Get with context-bounded request in smoke test - Use MustStartContainer helper in smoke test to remove duplication Co-authored-by: German Arutyunov <gaarutyunov@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/testutil/containers.go (1)
23-26: Bound cleanup termination with a timeout.Line [24] uses
context.Background()for container teardown. If Docker/Testcontainers is slow or unreachable, cleanup can block too long. Prefer a timeout-bound context.Proposed change
import ( "context" "testing" + "time" "github.com/testcontainers/testcontainers-go" ) @@ t.Cleanup(func() { - if err := c.Terminate(context.Background()); err != nil { + termCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + if err := c.Terminate(termCtx); err != nil { t.Logf("terminate container %q: %v", req.Image, err) } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/testutil/containers.go` around lines 23 - 26, The cleanup currently calls c.Terminate(context.Background()) which can block indefinitely; change the t.Cleanup closure to create a timeout-bound context (e.g., ctx, cancel := context.WithTimeout(context.Background(), <reasonable duration>) and defer cancel()) and pass that ctx to c.Terminate; update imports to include time if needed and ensure the cancel is deferred inside the closure so the termination is bounded and resources are released.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/testutil/containers.go`:
- Around line 23-26: The cleanup currently calls
c.Terminate(context.Background()) which can block indefinitely; change the
t.Cleanup closure to create a timeout-bound context (e.g., ctx, cancel :=
context.WithTimeout(context.Background(), <reasonable duration>) and defer
cancel()) and pass that ctx to c.Terminate; update imports to include time if
needed and ensure the cancel is deferred inside the closure so the termination
is bounded and resources are released.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8c3c1170-d538-48bc-8f97-f3d6948af01e
📒 Files selected for processing (4)
.golangci.ymlgo.modinternal/testutil/containers.gointernal/testutil/smoke_integration_test.go
✅ Files skipped from review due to trivial changes (3)
- .golangci.yml
- go.mod
- internal/testutil/smoke_integration_test.go
|
@claude fixed it |
|
Claude finished @gaarutyunov's task in 1m 42s —— View job Fixes applied
Still needed:
|
CodeRabbit nitpick: c.Terminate(context.Background()) could block indefinitely; use a timeout-bound context instead. Co-authored-by: German Arutyunov <gaarutyunov@users.noreply.github.com>
- Add go.sum (was never committed, causing CI vet failure) - Fix incorrect reggen pseudo-version hash in go.mod - Update .golangci.yml for golangci-lint v2 (move gofmt/goimports to formatters, remove gosimple merged into staticcheck) - Add package comment to cmd/proxy/main.go (revive) - Suppress contextcheck on cleanup closure that intentionally uses fresh context - Run go mod tidy to prune unused dependencies Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 participant
Implements TASK-02: GitHub Actions CI
internal/testutil/containers.gowith MustStartContainer helperinternal/testutil/smoke_integration_test.gosmoke test for Testcontainers.github/workflows/ci.ymlmust be added manually (GitHub App lacks workflows permission)Closes #4
Generated with Claude Code
Summary by CodeRabbit