chore(opensearch)!: use Run function #3423
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Summary by CodeRabbit
WalkthroughReworks OpenSearch module to build container options via defaults and applied Option functions (now error-returning), injects dynamic credentials into env and wait strategy, and uses testcontainers.Run with an HTTP basic-auth readiness check; updates error wrapping and preserves appended customizers. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Run as OpenSearch.Run
participant TC as testcontainers.Run
participant C as Container
participant OS as OpenSearch
participant WS as WaitStrategy
Caller->>Run: Run(ctx, image, opts...)
Run->>Run: defaultOptions() → moduleOpts
Run->>Run: apply Option(...) (now returns error)
Run->>Run: inject env (USERNAME/PASSWORD), ports, ulimits
Run->>Run: configure WaitStrategy (HTTP GET "/", port 9200, 200, basic-auth, tagline match)
Run->>TC: testcontainers.Run(ctx, image, moduleOpts + opts...)
TC->>C: create & start container
par startup readiness
C->>OS: start process
WS->>OS: HTTP GET "/" with basic auth
OS-->>WS: 200 OK + body
WS->>WS: validate tagline contains "OpenSearch"
end
TC-->>Run: container handle or error
Run-->>Caller: return container or wrapped error ("run opensearch: ...")
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
modules/opensearch/opensearch.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
modules/opensearch/opensearch.go (3)
modules/opensearch/options.go (1)
Option(22-22)options.go (5)
ContainerCustomizer(22-24)WithEnv(75-85)WithExposedPorts(454-459)WithHostConfigModifier(88-94)WithWaitStrategy(366-368)wait/http.go (1)
ForHTTP(149-151)
⏰ 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/opensearch) / test: modules/opensearch/1.24.x
- GitHub Check: test (1.25.x, modules/opensearch) / test: modules/opensearch/1.25.x
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
modules/opensearch/opensearch.go (3)
74-99: Comprehensive wait strategy ensures container readiness.The wait strategy is well-designed with multiple checks:
- HTTP endpoint verification
- Basic authentication
- Response content validation (OpenSearch tagline)
The 120-second timeout is appropriate given the comprehensive checks. The response matcher correctly handles errors by returning
falsewhen body reading or JSON unmarshaling fails.
102-102: Correct placement preserves user customization.Appending incoming
optsaftermoduleOptsensures that user-provided customizers can override module defaults, which is the expected behavior.
104-114: Clean migration to new API.The refactoring successfully migrates to
testcontainers.Runwith proper error handling and container construction. The error message provides clear context.
mdelapenya
force-pushed
the
use-run-claude-opensearch
branch
from
6d2e169 to
6039a3c
Compare
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
mdelapenya
force-pushed
the
use-run-claude-opensearch
branch
from
6039a3c to
0472ff8
Compare
mdelapenya
changed the title
mdelapenya
added
breaking change
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
modules/opensearch/opensearch.go (1)
38-46: Type assertion is valid; past review comment appears outdated.The type assertion
opt.(Option)is correct. Looking atoptions.goline 19, there's a compile-time check confirming thatOptionimplementsContainerCustomizer:var _ testcontainers.ContainerCustomizer = (*Option)(nil)The
Customizemethod is defined on lines 24-28 ofoptions.go. The pattern here extracts settings fromOptioninstances before building the module configuration, which is the intended design. Error wrapping with"apply option: %w"is appropriate.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
modules/opensearch/opensearch.go(2 hunks)modules/opensearch/options.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
modules/opensearch/opensearch.go (4)
modules/opensearch/options.go (1)
Option(22-22)options.go (5)
ContainerCustomizer(22-24)WithEnv(75-85)WithExposedPorts(454-459)WithHostConfigModifier(88-94)WithWaitStrategy(366-368)wait/http.go (1)
ForHTTP(149-151)modules/neo4j/neo4j.go (1)
Run(39-77)
modules/opensearch/options.go (2)
modules/elasticsearch/options.go (2)
Options(10-15)Option(27-27)modules/gcloud/gcloud.go (1)
Option(65-65)
⏰ 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/opensearch) / test: modules/opensearch/1.24.x
- GitHub Check: test (1.25.x, modules/opensearch) / test: modules/opensearch/1.25.x
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
modules/opensearch/options.go (1)
22-43: LGTM! Error-returning options align with other modules.The change from
func(*Options)tofunc(*Options) erroris consistent with the pattern used in other modules (e.g., Elasticsearch). The Option functions currently returnnil, which is appropriate since credential-setting operations don't fail. TheCustomizemethod remains a NOOP because the actual option application happens in theRunfunction (opensearch.go lines 40-46), where options are applied to the settings struct rather than directly customizing the container request.Based on learnings from relevant code snippets showing the Elasticsearch module uses the same pattern.
modules/opensearch/opensearch.go (2)
76-101: Robust wait strategy implementation.The wait strategy is comprehensive:
- HTTP health check on the root path
- Status code validation (HTTP 200)
- Response matcher validates the OpenSearch tagline for correctness
- Reasonable 120-second startup timeout
- TLS explicitly disabled with clear documentation
The basic auth concern was already flagged in the previous comment regarding the security plugin configuration.
104-116: LGTM! Container creation follows established patterns.The implementation correctly:
- Appends all options to
moduleOpts(consistent with the Neo4j module pattern shown in relevant snippets)- Uses
testcontainers.Runwith the composed options- Stores credentials in the container struct for later use
- Wraps errors with
"run opensearch: %w"for clear error contextNote:
Optioninstances are processed twice—once for settings extraction (lines 40-46) and once whenRuncallsCustomize(which is a NOOP). This is intentional and consistent with the design whereOptionimplementsContainerCustomizerbut performs its work during settings extraction rather than inCustomize.Based on learnings from relevant code snippets showing the Neo4j module uses the same pattern.
1 participant
What does this PR do?
Use the Run function in opensearch
Why is it important?
Migrate modules to the new API, improving consistency and leveraging the latest testcontainers functionality.
Related issues