chore(artemis): use Run function #3320
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Summary by CodeRabbit
WalkthroughRefactors Artemis module to use testcontainers.Run with functional options, sets default credentials, inspects started container to derive ARTEMIS_USER/ARTEMIS_PASSWORD, and updates tests to use require.Equal with ticker cleanup. Public API unchanged. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant ArtemisRun as modules/artemis.Run
participant TC as testcontainers.Run
participant Container
participant Inspector as Container.Inspect
participant Parser as Env Parser
Caller->>ArtemisRun: Run(ctx, opts...)
ArtemisRun->>TC: Run(ctx, image, moduleOpts...)
alt Run error
TC-->>ArtemisRun: error
ArtemisRun-->>Caller: wrapped start error
else Run success
TC-->>ArtemisRun: Container handle
ArtemisRun->>Inspector: Inspect(ctx)
alt Inspect error
Inspector-->>ArtemisRun: error
ArtemisRun-->>Caller: wrapped inspect error
else Inspect success
Inspector-->>ArtemisRun: Config.Env list
ArtemisRun->>Parser: parse ARTEMIS_USER / ARTEMIS_PASSWORD
Parser-->>ArtemisRun: resolved credentials
ArtemisRun-->>Caller: container + credentials
end
end
rect rgba(46,125,50,0.06)
note right of Parser: credential derivation is post-start
end
rect rgba(211,47,47,0.06)
note right of TC: Run/inspect/parse errors propagate
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/artemis/artemis.go (1)
54-61: Avoid potential nil map writes; reuse WithEnv helperreq.Env may be nil depending on option ordering/usage outside this module. Use testcontainers.WithEnv to ensure initialization and merge behavior.
func WithCredentials(user, password string) testcontainers.CustomizeRequestOption { - return func(req *testcontainers.GenericContainerRequest) error { - req.Env["ARTEMIS_USER"] = user - req.Env["ARTEMIS_PASSWORD"] = password - - return nil - } + return testcontainers.WithEnv(map[string]string{ + "ARTEMIS_USER": user, + "ARTEMIS_PASSWORD": password, + }) } func WithAnonymousLogin() testcontainers.CustomizeRequestOption { - return func(req *testcontainers.GenericContainerRequest) error { - req.Env["ANONYMOUS_LOGIN"] = "true" - - return nil - } + return testcontainers.WithEnv(map[string]string{ + "ANONYMOUS_LOGIN": "true", + }) } func WithExtraArgs(args string) testcontainers.CustomizeRequestOption { - return func(req *testcontainers.GenericContainerRequest) error { - req.Env["EXTRA_ARGS"] = args - - return nil - } + return testcontainers.WithEnv(map[string]string{ + "EXTRA_ARGS": args, + }) }Also applies to: 64-70, 76-82
🧹 Nitpick comments (6)
modules/artemis/artemis.go (4)
113-115: Clarify error message context“generic container” is ambiguous now that Run() is used. Recommend a clearer message.
- return c, fmt.Errorf("generic container: %w", err) + return c, fmt.Errorf("run artemis: %w", err)
117-121: Guard against nil container before dereferenceImprobable, but if ctr == nil with err == nil, c.user assignments will panic. Add a defensive check.
// initialize the credentials -c.user = defaultUser -c.password = defaultPassword +if c == nil { + return nil, fmt.Errorf("run artemis: container is nil") +} +c.user = defaultUser +c.password = defaultPassword
126-139: Break early after both creds foundSmall efficiency/readability tweak: stop scanning once both ARTEMIS_USER and ARTEMIS_PASSWORD are found.
-// refresh the credentials from the environment variables -for _, env := range inspect.Config.Env { - value, ok := strings.CutPrefix(env, "ARTEMIS_USER=") - if ok { - c.user = value - continue - } - - value, ok = strings.CutPrefix(env, "ARTEMIS_PASSWORD=") - if ok { - c.password = value - continue - } -} +foundUser, foundPass := false, false +for _, env := range inspect.Config.Env { + if v, ok := strings.CutPrefix(env, "ARTEMIS_USER="); ok { + c.user, foundUser = v, true + } else if v, ok := strings.CutPrefix(env, "ARTEMIS_PASSWORD="); ok { + c.password, foundPass = v, true + } + if foundUser && foundPass { + break + } +}
98-104: Consider a longer wait deadline for CI stabilityArtemis can take >60s on shared runners. Optionally raise the default deadline.
+ // import "time" - testcontainers.WithWaitStrategy(wait.ForAll( + testcontainers.WithWaitStrategyAndDeadline(120*time.Second, wait.ForAll( ... )),If you prefer keeping 60s, expose a WithWaitDeadline option for callers to override.
modules/artemis/artemis_test.go (2)
95-97: Tighten login conditionOnly set STOMP login if both user and pass are non-empty to avoid accidental empty credentials.
- if test.user != "" || test.pass != "" { + if test.user != "" && test.pass != "" { opt = append(opt, stomp.ConnOpt.Login(test.user, test.pass)) }
110-116: Stop ticker to avoid goroutine leak on early returnsDefer ticker.Stop() so it’s cleaned up if the test fails before receiving.
- ticker := time.NewTicker(10 * time.Second) + ticker := time.NewTicker(10 * time.Second) + defer ticker.Stop()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
modules/artemis/artemis.go(3 hunks)modules/artemis/artemis_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
modules/artemis/artemis.go (5)
options.go (3)
WithExposedPorts(464-469)WithEnv(75-85)WithWaitStrategy(376-378)wait/all.go (1)
ForAll(44-48)wait/host_port.go (1)
ForListeningPort(67-69)wait/log.go (1)
ForLog(118-120)container.go (1)
Container(41-73)
⏰ 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: test (1.25.x, modules/artemis) / test: modules/artemis/1.25.x
- GitHub Check: test (1.24.x, modules/artemis) / test: modules/artemis/1.24.x
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
modules/artemis/artemis_test.go (3)
56-57: LGTM on documenting defaults inlineThe explicit comments improve readability; matches defaultUser/defaultPassword in module.
11-11: Switch to require is appropriate hereUsing require.* short-circuits on fatal conditions and avoids cascaded failures in integration tests.
Based on learnings
66-70: CleanupContainer is nil-safe — no changes required
TerminateContainer (called by CleanupContainer) returns early ifctris nil (cleanup.go around lines 97–104), so there’s no panic risk.modules/artemis/artemis.go (1)
128-137: Go version requirement satisfied; no changes needed
go.mod declaresgo 1.24.0, which is ≥ 1.20—strings.CutPrefixis supported.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
modules/artemis/artemis.go (3)
54-61: Prevent panic: initialize req.Env before writes in WithCredentialsreq.Env may be nil; writing to a nil map panics. Initialize it first, mirroring WithEnv’s behavior.
Apply:
func WithCredentials(user, password string) testcontainers.CustomizeRequestOption { return func(req *testcontainers.GenericContainerRequest) error { - req.Env["ARTEMIS_USER"] = user - req.Env["ARTEMIS_PASSWORD"] = password + if req.Env == nil { + req.Env = map[string]string{} + } + req.Env["ARTEMIS_USER"] = user + req.Env["ARTEMIS_PASSWORD"] = password return nil } }
64-70: Same nil‑map risk in WithAnonymousLoginGuard against nil maps before setting ANONYMOUS_LOGIN.
func WithAnonymousLogin() testcontainers.CustomizeRequestOption { return func(req *testcontainers.GenericContainerRequest) error { - req.Env["ANONYMOUS_LOGIN"] = "true" + if req.Env == nil { + req.Env = map[string]string{} + } + req.Env["ANONYMOUS_LOGIN"] = "true" return nil } }
76-82: Same nil‑map risk in WithExtraArgsInitialize req.Env before setting EXTRA_ARGS.
func WithExtraArgs(args string) testcontainers.CustomizeRequestOption { return func(req *testcontainers.GenericContainerRequest) error { - req.Env["EXTRA_ARGS"] = args + if req.Env == nil { + req.Env = map[string]string{} + } + req.Env["EXTRA_ARGS"] = args return nil } }
🧹 Nitpick comments (1)
modules/artemis/artemis.go (1)
45-51: PortEndpoint returns host:port when proto is blank; use net/url for URL construction
- Verified that calling
PortEndpoint(ctx, _, "")returns justhost:port(no scheme) per the implementations inmodules/ollama/local.goanddocker.go.- Replace the manual
fmt.Sprintfwithnet/urlto ensure proper escaping of credentials and path:@@ -import ( - "context" - "fmt" - "strings" +import ( + "context" + "fmt" + "net/url" + "strings" ) @@ - host, err := c.PortEndpoint(ctx, nat.Port(defaultHTTPPort), "") + hostPort, err := c.PortEndpoint(ctx, nat.Port(defaultHTTPPort), "") if err != nil { return "", err } - return fmt.Sprintf("http://%s:%s@%s/console", c.user, c.password, host), nil + u := url.URL{ + Scheme: "http", + User: url.UserPassword(c.user, c.password), + Host: hostPort, + Path: "/console", + } + return u.String(), nil
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
modules/artemis/artemis.go(3 hunks)modules/artemis/artemis_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- modules/artemis/artemis_test.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-29T15:08:18.668Z
Learnt from: mdelapenya
PR: testcontainers/testcontainers-go#3320
File: modules/artemis/artemis.go:98-103
Timestamp: 2025-09-29T15:08:18.668Z
Learning: In testcontainers-go, nat.Port is a type alias for string, so untyped string constants can be passed directly to functions expecting nat.Port (like wait.ForListeningPort) without explicit type conversion - the Go compiler handles the implicit conversion automatically.
Applied to files:
modules/artemis/artemis.go
📚 Learning: 2025-09-29T13:57:14.629Z
Learnt from: mdelapenya
PR: testcontainers/testcontainers-go#3319
File: modules/arangodb/arangodb.go:46-57
Timestamp: 2025-09-29T13:57:14.629Z
Learning: In testcontainers-go ArangoDB module, the wait strategy combines port listening check with HTTP readiness check using wait.ForAll - both strategies are required and complementary, not redundant.
Applied to files:
modules/artemis/artemis.go
🧬 Code graph analysis (1)
modules/artemis/artemis.go (5)
options.go (4)
ContainerCustomizer(22-24)WithExposedPorts(464-469)WithEnv(75-85)WithWaitStrategy(376-378)wait/all.go (1)
ForAll(44-48)wait/host_port.go (1)
ForListeningPort(67-69)wait/log.go (1)
ForLog(118-120)container.go (1)
Container(41-73)
⏰ 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: test (1.25.x, modules/artemis) / test: modules/artemis/1.25.x
- GitHub Check: test (1.24.x, modules/artemis) / test: modules/artemis/1.24.x
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
modules/artemis/artemis.go (2)
98-103: ForListeningPort with untyped string constants is correctPassing untyped string constants to ForListeningPort (nat.Port) is valid; no explicit nat.Port(...) cast is required.
Based on learnings.
129-135: strings.CutPrefix supported by Go 1.24
go.mod declaresgo 1.24, sostrings.CutPrefixis available; no fallback needed.
1 participant
What does this PR do?
Use Run function in the Artemis module
Why is it important?
Migrate modules to the new API
Related issues