Skip to content

Conversation

@mdelapenya
Copy link
Member

What does this PR do?

Use the Run function in scylladb

Why is it important?

Migrate modules to the new API, improving consistency and leveraging the latest testcontainers functionality.

Related issues

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
@mdelapenya mdelapenya requested a review from a team as a code owner October 9, 2025 10:15
@mdelapenya mdelapenya added the chore Changes that do not impact the existing functionality label Oct 9, 2025
@netlify
Copy link

netlify bot commented Oct 9, 2025

Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit b5e56c5
🔍 Latest deploy log https://app.netlify.com/projects/testcontainers-go/deploys/68e78b38f3bff0000812cd5e
😎 Deploy Preview https://deploy-preview-3433--testcontainers-go.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@coderabbitai
Copy link

coderabbitai bot commented Oct 9, 2025

Summary by CodeRabbit

  • Refactor

    • Switched to an option-based configuration flow for starting containers, improving consistency and predictability.
    • Standardized command-flag handling: existing flags are cleanly overridden and remaining flags appended in a stable manner.
    • Improved error messaging when startup fails for clearer diagnostics.
  • Tests

    • Updated tests to align with the new customization pattern, ensuring behavior remains unchanged for commands, overrides, and assertions.

Walkthrough

Refactors ScyllaDB module to compose container configuration via testcontainers option customizers and testcontainers.Run. Introduces setCommandFlags to rebuild command arguments via WithCmd. Updates WithConfig, WithShardAwareness, and WithAlternator to use WithFiles/WithExposedPorts/WithWaitStrategy. Adjusts tests to pass request pointers consistent with new option-based customization.

Changes

Cohort / File(s) Summary of changes
Option-based refactor and command flags
modules/scylladb/scylladb.go
Replace direct GenericContainerRequest mutations with testcontainers.With... customizers; implement setCommandFlags(req, map) to reconstruct Cmd via WithCmd; rework Run to build moduleOpts and call testcontainers.Run; route port exposure and wait strategies via options; update error message text.
Tests alignment with pointer requests
modules/scylladb/scylladb_test.go
Switch tests to use pointer GenericContainerRequest and pass pointer to opt.Customize; preserve assertions on command contents and overrides.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Dev as Developer/Test
  participant Mod as ScyllaDB Module
  participant Opts as Module Options
  participant TC as testcontainers.Run
  participant C as Container Runtime

  Dev->>Mod: Run(ctx, image, opts...)
  Mod->>Opts: Build moduleOpts\n- WithExposedPorts\n- WithWaitStrategy\n- WithCmd
  Opts-->>Mod: Customizers ready
  Mod->>Opts: Apply WithConfig/WithShardAwareness/\nWithAlternator (optional)
  Note right of Opts: Internally uses\nWithFiles/WithExposedPorts/\nWithWaitStrategy
  Mod->>Opts: setCommandFlags(req, flags)\nrebuild Cmd via WithCmd
  Mod->>TC: Run(ctx, image, moduleOpts...)
  TC->>C: Create & start container
  C-->>TC: Container handle
  TC-->>Mod: Container
  Mod-->>Dev: ScyllaDB Container wrapper
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested reviewers

  • stevenh

Poem

I twitched my ears at flags and ports,
Hopped past fields of Cmd cohorts;
Options lined like carrots fine,
Run now serves them, all in line.
With waits and files neatly spun—
Thump! A Scylla starts to run. 🥕🐇

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The title succinctly indicates that the scylladb module is being updated to use the Run function, which matches the primary change in the pull request without extraneous details.
Description Check ✅ Passed The description outlines that the PR updates scylladb to use the Run function and explains the motivation to migrate to the new API, which directly aligns with the changes introduced.
Docstring Coverage ✅ Passed Docstring coverage is 85.71% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d256b4b and b5e56c5.

📒 Files selected for processing (2)
  • modules/scylladb/scylladb.go (5 hunks)
  • modules/scylladb/scylladb_test.go (8 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
modules/scylladb/scylladb.go (3)
options.go (6)
  • WithFiles (524-529)
  • CustomizeRequestOption (28-28)
  • WithExposedPorts (454-459)
  • WithWaitStrategy (366-368)
  • ContainerCustomizer (22-24)
  • WithCmd (462-467)
generic.go (1)
  • GenericContainerRequest (21-27)
wait/host_port.go (1)
  • ForListeningPort (67-69)
modules/scylladb/scylladb_test.go (1)
generic.go (1)
  • GenericContainerRequest (21-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.24.x, modules/scylladb) / test: modules/scylladb/1.24.x
  • GitHub Check: test (1.25.x, modules/scylladb) / test: modules/scylladb/1.25.x
  • GitHub Check: Analyze (go)
🔇 Additional comments (6)
modules/scylladb/scylladb_test.go (1)

200-271: LGTM! Clean alignment with pointer-based API.

The mechanical refactor from value-based to pointer-based GenericContainerRequest instantiation is correct and aligns with the updated API usage in the main module. All test logic remains intact while properly using the new pattern.

modules/scylladb/scylladb.go (5)

25-34: LGTM! Cleaner use of helper options.

The refactor to use testcontainers.WithFiles instead of direct field mutation is cleaner and more consistent with the testcontainers API patterns.


37-44: LGTM! Proper error handling with helper options.

The refactor correctly uses WithExposedPorts and WithWaitStrategy helper options while maintaining proper error propagation.


50-65: LGTM! Consistent pattern with proper error handling.

The refactor correctly chains helper options (WithExposedPorts, WithWaitStrategy) and delegates flag handling to setCommandFlags, maintaining clean separation of concerns.


110-141: LGTM! Successful migration to the new Run API.

The refactoring achieves the PR objective by replacing manual GenericContainerRequest construction with a cleaner moduleOpts collection pattern. Key improvements:

  • Default options are cleanly separated using helper options
  • User options are appended and can override defaults
  • Error message updated to "run scylladb: %w" for better context
  • Container wrapping preserves error-handling pattern

This aligns well with the new testcontainers API and improves code maintainability.


143-178: LGTM! Correct flag handling with cleaner API usage.

The refactored setCommandFlags properly:

  • Overrides existing flags by matching on flag names (before =)
  • Removes processed flags from the map to avoid duplicates
  • Appends remaining flags in any order (map iteration)
  • Uses WithCmd instead of direct field mutation

The logic correctly handles both --flag=value and --flag patterns.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@mdelapenya mdelapenya merged commit 58143a2 into testcontainers:main Oct 9, 2025
17 checks passed
@mdelapenya mdelapenya deleted the use-run-claude-scylladb branch October 9, 2025 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chore Changes that do not impact the existing functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant