Skip to content

Conversation

@mdelapenya
Copy link
Member

What does this PR do?

Use the Run function in k6, localstack, kafka, and mariadb modules

Why is it important?

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

Related issues

mdelapenya and others added 6 commits October 6, 2025 22:19
🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
🤖 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 6, 2025 21:36
@coderabbitai
Copy link

coderabbitai bot commented Oct 6, 2025

Summary by CodeRabbit

  • New Features
    • Pre-run validation for Kafka KRaft compatibility and LocalStack minimum version, with clearer startup errors.
    • MariaDB now validates credentials at runtime and auto-populates connection details after startup.
  • Refactor
    • Standardized container setup across k3s, k6, Kafka, LocalStack, and MariaDB using higher-level configuration options for more consistent behavior.
  • Bug Fixes
    • More robust startup and readiness handling through centralized lifecycle hooks.
  • Tests
    • Expanded and updated tests to reflect new validation logic and configuration flow.

Walkthrough

Refactors multiple modules to use testcontainers' functional option-based Run flow instead of in-place GenericContainerRequest mutation; adds image/version/credential validations, centralized post-start hooks, docker-host configuration, a deprecated LocalStack request alias, and test updates to match new signatures.

Changes

Cohort / File(s) Summary
K3s option wrapper
modules/k3s/k3s.go
WithManifest now applies testcontainers.WithFiles(...)(req) instead of mutating req.Files in-place, switching to functional-option application and returning any error.
K6 option migration
modules/k6/k6.go
Replaced direct req field edits with testcontainers.WithFiles, testcontainers.WithCmdArgs, and testcontainers.WithMounts; Run now builds moduleOpts and calls testcontainers.Run(...); script/env handling moved to option helpers; deprecated WithCmdOptions delegates to WithCmdArgs.
Kafka refactor + KRaft validation
modules/kafka/kafka.go, modules/kafka/kafka_helpers_test.go
Added validateKRaftVersion(img) pre-check; builds moduleOpts (WithExposedPorts/WithEnv/WithEntrypoint/WithCmd/WithLifecycleHooks) and uses testcontainers.Run; introduced configureControllerQuorumVoters() returning an option; WithClusterID now uses WithEnv; post-start hook copies starter script and waits; tests updated to call the returned customizer.
LocalStack refactor, docker-host & tests
modules/localstack/localstack.go, modules/localstack/localstack_test.go, modules/localstack/types.go
Switched to moduleOpts + testcontainers.Run; added minimum-image version guard (legacy <0.11 rejected); deferred docker-host setup via configureDockerHost(ctx, envVar)/setDockerHost(ctx, req, envVar) customize option using DockerProvider.DaemonHost(ctx); tests renamed/updated to use *testcontainers.GenericContainerRequest; added deprecated LocalStackContainerRequest alias type.
MariaDB optionization & credential validation
modules/mariadb/mariadb.go, modules/mariadb/mariadb_test.go
Replaced manual req field mutation with option helpers (WithExposedPorts, WithEnv, WithFiles, WithWaitStrategy) and testcontainers.Run; added validateCredentials option enforcing empty-password rules; post-run Inspect used to populate username/password/database; test error expectation updated.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Caller
  participant Module as Module.Run
  participant TC as testcontainers.Run
  participant Docker as Docker Engine
  participant Container as Container

  Caller->>Module: Run(ctx, image, opts...)
  Module->>Module: Build moduleOpts (WithEnv/Files/Cmd/Mounts/Wait/Validate...)
  Module->>TC: Run(ctx, image, moduleOpts...)
  TC->>Docker: Create & Start container (apply lifecycle hooks)
  Docker-->>TC: Started
  TC-->>Module: Container handle
  Module->>Container: Inspect (env/network)
  Container-->>Module: Details
  Module-->>Caller: Module-specific wrapper (populated from Inspect)
Loading
sequenceDiagram
  autonumber
  actor Caller
  participant KafkaModule as kafka.Run
  participant TC as testcontainers.Run
  participant Hook as PostStart Hook
  participant Container as Container

  Caller->>KafkaModule: Run(ctx, img, opts...)
  KafkaModule->>KafkaModule: validateKRaftVersion(img)
  KafkaModule->>KafkaModule: Add configureControllerQuorumVoters() option + lifecycle hooks
  KafkaModule->>TC: Run(ctx, img, moduleOpts...)
  TC-->>KafkaModule: Container handle
  KafkaModule->>Hook: Execute PostStart Hook
  Hook->>Container: Copy starter script & wait for readiness
  KafkaModule->>Container: Inspect (CLUSTER_ID)
  Container-->>KafkaModule: CLUSTER_ID
  KafkaModule-->>Caller: KafkaContainer
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Suggested reviewers

  • stevenh

Poem

In burrows of code I nibble and run,
Options now hop where fields once were done.
KRaft checked, hosts set, scripts snug and right —
Containers wake up tidy in light. 🥕🐇

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 57.14% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title clearly summarizes the primary change by indicating that the specified modules are now using the new Run function, matching the PR’s intent and scope in a concise manner.
Description Check ✅ Passed The pull request description directly relates to using the Run function in the k6, localstack, kafka, and mariadb modules and explains the motivation and context, aligning with the changeset.
✨ 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 898306a and 92404b0.

📒 Files selected for processing (2)
  • modules/kafka/kafka.go (5 hunks)
  • modules/kafka/kafka_helpers_test.go (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
modules/kafka/kafka.go (4)
options.go (7)
  • ContainerCustomizer (22-24)
  • WithExposedPorts (454-459)
  • WithEnv (75-85)
  • WithEntrypoint (438-443)
  • WithCmd (462-467)
  • WithLifecycleHooks (489-494)
  • CustomizeRequestOption (28-28)
container.go (1)
  • Container (41-73)
wait/log.go (1)
  • ForLog (118-120)
modules/localstack/localstack.go (1)
  • Run (66-103)
⏰ 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). (1)
  • GitHub Check: Analyze (go)
🔇 Additional comments (8)
modules/kafka/kafka.go (6)

49-51: Past issue resolved: validation now handles images without tags.

The validation call here previously would panic on images lacking tags, as noted in the past review. Lines 202-208 now properly guard against this by checking for missing colons and returning nil to skip validation in such cases.


53-92: LGTM! Clean migration to functional options pattern.

The moduleOpts setup correctly uses the testcontainers options API. The consolidated PostStarts hook (lines 76-91) is well-designed—combining script copy and readiness wait ensures the wait doesn't proceed if the copy fails.


99-122: LGTM! Proper error handling and container inspection.

The Run invocation and error handling are correct—the container is returned even when errors occur, allowing cleanup. The ClusterID population from inspect.Config.Env (lines 114-119) is appropriate; if the CLUSTER_ID environment variable isn't set, the field remains empty, which appears to be acceptable given the field's optional nature.


152-156: LGTM! Correctly updated to functional options pattern.

WithClusterID now returns testcontainers.WithEnv, consistent with the new API design throughout this module.


169-193: LGTM! Well-designed functional option.

The refactored configureControllerQuorumVoters correctly returns a CustomizeRequestOption and intelligently determines the controller host based on network aliases or falls back to localhost. The logic properly initializes req.Env and respects any pre-existing KAFKA_CONTROLLER_QUORUM_VOTERS value.


202-208: Past issue fully resolved: missing tag handling is now safe.

The fix properly addresses the critical issue raised in the previous review. By checking for missing or empty tags (idx == -1 or idx == len(fqName)-1) and returning nil to skip validation in those cases, the function now gracefully handles images without tags instead of panicking.

modules/kafka/kafka_helpers_test.go (2)

58-59: LGTM! Test correctly updated for new functional options pattern.

The test now properly calls configureControllerQuorumVoters() to obtain the option function, then invokes it with test.req, matching the new higher-order function signature.


97-101: LGTM! Valuable test coverage for the missing tag fix.

This new test case validates that images without tags (e.g., "my-kafka") are handled gracefully, providing coverage for the fix implemented in lines 202-208 of kafka.go.


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 self-assigned this Oct 6, 2025
@mdelapenya mdelapenya added the chore Changes that do not impact the existing functionality label Oct 6, 2025
@netlify
Copy link

netlify bot commented Oct 6, 2025

Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit 92404b0
🔍 Latest deploy log https://app.netlify.com/projects/testcontainers-go/deploys/68e43d04fad8690008e09e93
😎 Deploy Preview https://deploy-preview-3414--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.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e1c8e7b and 898306a.

📒 Files selected for processing (1)
  • modules/kafka/kafka.go (4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
modules/kafka/kafka.go (4)
options.go (7)
  • ContainerCustomizer (22-24)
  • WithExposedPorts (454-459)
  • WithEnv (75-85)
  • WithEntrypoint (438-443)
  • WithCmd (462-467)
  • WithLifecycleHooks (489-494)
  • CustomizeRequestOption (28-28)
lifecycle.go (2)
  • ContainerLifecycleHooks (43-55)
  • ContainerHook (38-38)
modules/mariadb/mariadb.go (1)
  • Run (122-177)
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/localstack) / test: modules/localstack/1.24.x
  • GitHub Check: test (1.24.x, modules/k3s) / test: modules/k3s/1.24.x
  • GitHub Check: Analyze (go)

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