Skip to content

Conversation

@mdelapenya
Copy link
Member

What does this PR do?

Use Run function in the Databend module

Why is it important?

Migrate modules to the new API

Related issues

@mdelapenya mdelapenya requested a review from a team as a code owner October 2, 2025 09:27
@netlify
Copy link

netlify bot commented Oct 2, 2025

Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit b7994d4
🔍 Latest deploy log https://app.netlify.com/projects/testcontainers-go/deploys/68de4c2680675600086d58bf
😎 Deploy Preview https://deploy-preview-3402--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 2, 2025

Warning

Rate limit exceeded

@mdelapenya has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 36 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 026c4d7 and b7994d4.

📒 Files selected for processing (1)
  • modules/databend/databend.go (3 hunks)

Summary by CodeRabbit

  • Refactor

    • Streamlined container startup using modular options with a unified default port.
    • Automatically detects credentials from the running container’s environment.
    • Improved error propagation with clearer messages when startup or inspection fails.
  • Bug Fixes

    • Ensures empty username/password combinations are correctly treated as errors after startup checks.
  • Tests

    • Added coverage for empty-credential scenarios to validate enforcement and error handling.

Walkthrough

Refactors Databend module to use testcontainers.Run with modular options, introduces post-start environment inspection to populate credentials, centralizes default port usage, updates error handling around Run/Inspect, and adds a test to validate erroring on empty credentials.

Changes

Cohort / File(s) Summary
Databend module runtime refactor
modules/databend/databend.go
Replaces GenericContainerRequest with testcontainers.Run using option slice (ports, env, wait strategy); adds defaultPort constant; inspects started container to refresh username/password from env; shifts empty-credential validation post-inspect; updates error returns to include partially built container.
Tests
modules/databend/databend_test.go
Adds TestDatabendWithEmptyCredentials to assert error when username and password are empty; ensures cleanup of started container.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Caller
  participant Module as Databend Module
  participant TC as testcontainers.Run
  participant Ctr as Container
  Caller->>Module: Start Databend(img, opts)
  Module->>TC: Run(ctx, img, moduleOpts...)
  alt Run success
    TC-->>Module: Container handle
    Module->>Ctr: Inspect()
    Ctr-->>Module: Env (QUERY_DEFAULT_USER/PASSWORD)
    Module->>Module: Update credentials from env
    Module-->>Caller: DatabendContainer or error if creds empty
  else Run failure
    TC-->>Module: Error
    Module-->>Caller: Partial DatabendContainer, wrapped error
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested labels

chore

Suggested reviewers

  • stevenh

Poem

I hopped through ports on 8000’s shore,
Swapped old crates for Run—what a score!
Peeked at envs, found keys to the den,
If creds are empty, I’ll thump again.
Inspect, reflect, then bound away—
A tidy warren for tests today. 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% 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 succinctly captures the core change—updating the Databend module to use the Run function API—using clear, specific phrasing and conventional commit style, so it accurately reflects the main intent of the PR.
Description Check ✅ Passed The description clearly states that the PR migrates the Databend module to the new Run function API, explains the rationale for the change, and references the related issue, directly corresponding to the implemented code updates.

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.

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 e3c6250 and 026c4d7.

📒 Files selected for processing (2)
  • modules/databend/databend.go (2 hunks)
  • modules/databend/databend_test.go (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
modules/databend/databend_test.go (2)
modules/databend/databend.go (3)
  • Run (40-93)
  • WithUsername (126-131)
  • WithPassword (134-139)
testing.go (1)
  • CleanupContainer (91-97)
modules/databend/databend.go (3)
options.go (3)
  • ContainerCustomizer (22-24)
  • WithExposedPorts (454-459)
  • WithWaitStrategy (366-368)
wait/host_port.go (1)
  • ForListeningPort (67-69)
modules/clickhouse/clickhouse.go (1)
  • Run (69-123)

@mdelapenya mdelapenya added the chore Changes that do not impact the existing functionality label Oct 2, 2025
@mdelapenya mdelapenya self-assigned this Oct 2, 2025
@mdelapenya mdelapenya merged commit 8350eb9 into testcontainers:main Oct 2, 2025
17 checks passed
@mdelapenya mdelapenya deleted the use-run-databend branch October 2, 2025 10:35
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