chore(cockroachDB): use Run function #3326
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Summary by CodeRabbit
WalkthroughReplaces GenericContainerRequest-based startup with testcontainers.Run using assembled With* options; adds a post-assembly configure customizer that refreshes credentials, detects insecure mode, and adjusts/removes TLS wait strategies; removes the old configure(req) method and updates startup error text. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Module as CockroachDBContainer
participant TC as testcontainers.Run
participant Container as Container
participant Wait as WaitStrategies
Client->>Module: Start(ctx, image, opts)
Module->>Module: Build moduleOpts (WithCmd, WithEnv, WithFiles, WithWaitStrategy...)
Module->>Module: Append configure() customizer
Module->>TC: Run(ctx, image, moduleOpts...)
TC-->>Container: Container or error
rect rgba(230,245,255,0.4)
note right of Module: configure() runs after opts are assembled
Module->>Wait: Inspect wait strategies for TLSStrategy
alt insecure mode detected
Wait-->>Module: Remove TLSStrategy
else secure mode
Wait-->>Module: Update TLSStrategy client cert files (per user)
Module->>Module: Store TLSStrategy ref
end
Module->>Module: Refresh credentials from env
end
Container-->>Client: Ready or error ("run cockroachdb" on failure)
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
🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
modules/cockroachdb/cockroachdb.go(1 hunks)modules/cockroachdb/options.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
modules/cockroachdb/cockroachdb.go (1)
options.go (6)
ContainerCustomizer(22-24)WithCmd(462-467)WithExposedPorts(454-459)WithEnv(75-85)WithFiles(524-529)WithWaitStrategy(366-368)
modules/cockroachdb/options.go (4)
modules/cockroachdb/cockroachdb.go (1)
CockroachDBContainer(73-76)generic.go (1)
GenericContainerRequest(21-27)wait/walk.go (3)
Walk(30-43)ErrVisitRemove(18-18)ErrVisitStop(11-11)wait/tls.go (1)
TLSStrategy(16-27)
⏰ 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/cockroachdb) / test: modules/cockroachdb/1.25.x
- GitHub Check: test (1.24.x, modules/cockroachdb) / test: modules/cockroachdb/1.24.x
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
modules/cockroachdb/options.go (1)
5-5: LGTM!Import additions are appropriate for the new configure method functionality.
Also applies to: 7-7, 11-11
modules/cockroachdb/cockroachdb.go (3)
178-209: LGTM with note on panic in wait strategy.The modular options approach correctly assembles the container configuration. The TLS certificate paths use
defaultUserhere but are updated byconfigure()to match the actual user after options are applied.Note: The ForSQL wait strategy panics if
connStringreturns an error (lines 201-207). While this is acceptable during container initialization, consider whether this should log and return an error instead.
211-215: LGTM!The ordering is correct: user options are applied first, then
configure()extracts the final credentials and TLS configuration. The comment clearly explains this dependency.
218-220: LGTM!The migration to
testcontainers.Runfollows the new module pattern. The error message is appropriately updated to be more specific.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
modules/cockroachdb/cockroachdb.go (1)
201-207: Consider more graceful error handling in the ForSQL closure.The closure panics if
connStringfails. While this will cause the container startup to fail, panicking is not idiomatic Go error handling. Consider returning an error or using a sentinel connection string that will cause ForSQL to retry/fail gracefully.However, if the panic is intentional to fail-fast on misconfiguration, consider adding a comment explaining this design choice.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
modules/cockroachdb/cockroachdb.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
modules/cockroachdb/cockroachdb.go (2)
options.go (6)
ContainerCustomizer(22-24)WithCmd(462-467)WithExposedPorts(454-459)WithEnv(75-85)WithFiles(524-529)WithWaitStrategy(366-368)modules/chroma/chroma.go (1)
Run(23-48)
⏰ 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.24.x, modules/cockroachdb) / test: modules/cockroachdb/1.24.x
- GitHub Check: test (1.25.x, modules/cockroachdb) / test: modules/cockroachdb/1.25.x
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
modules/cockroachdb/cockroachdb.go (5)
211-211: LGTM!Appending user-provided options after defaults allows proper customization while maintaining sensible defaults.
213-215: LGTM!Applying
configure()as the final customizer ensures it can inspect and adjust the complete configuration after all user options have been applied, which is essential for TLS strategy detection and credential extraction.
218-218: LGTM!The migration to
testcontainers.Runwith modular options is clean and follows the established pattern for the new API.
220-220: LGTM!The updated error message provides more specific context, making debugging easier.
246-250: LGTM!The nil-check prevents potential nil pointer dereference when
tlsStrategyis nil (e.g., in insecure mode). This ensurestlsConfigis properly set tonilwhen TLS is disabled, which is then correctly handled by the conditional check on line 252.
1 participant
What does this PR do?
Use Run function in the CockroachDB module.
The configure method has been converted into a container customiser option, so that it can be applied last, extracting from the request the credentials and the TLS config.
Why is it important?
Migrate modules to the new API
Related issues