Skip to content

Conversation

@mdelapenya
Copy link
Member

What does this PR do?

Use Run function in the aerospike 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 September 26, 2025 14:19
@mdelapenya mdelapenya added the chore Changes that do not impact the existing functionality label Sep 26, 2025
@mdelapenya mdelapenya self-assigned this Sep 26, 2025
@coderabbitai
Copy link

coderabbitai bot commented Sep 26, 2025

Summary by CodeRabbit

  • Refactor
    • Reworked Aerospike module setup to use a modular, options-based approach for container configuration, combining port exposure, environment variables, and readiness checks into a unified flow.
    • Streamlined startup by removing redundant request construction and customization loops.
    • Ensures more consistent handling of configuration across environments.
    • Updated startup failure messaging to be clearer and more specific.

Walkthrough

Refactors Aerospike module container startup to use testcontainers.Run with ContainerCustomizer options (WithExposedPorts, WithEnv, WithWaitStrategy), replacing GenericContainerRequest/GenericContainer usage and manual customization. Updates error message on startup failure.

Changes

Cohort / File(s) Summary
Aerospike module startup refactor
modules/aerospike/aerospike.go
Replace GenericContainerRequest + GenericContainer and manual option loop with testcontainers.Run(ctx, img, moduleOpts...) using WithExposedPorts, WithEnv, WithWaitStrategy; adjust error text to "run aerospike: %w".

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant C as Caller
  participant A as Aerospike Module
  participant T as testcontainers.Run
  participant D as Docker Daemon

  C->>A: Start(ctx, img, opts...)
  A->>A: Build moduleOpts (WithExposedPorts, WithEnv, WithWaitStrategy, opts...)
  A->>T: Run(ctx, img, moduleOpts...)
  T->>D: Create & start container
  alt success
    D-->>T: Container started
    T-->>A: Container handle
    A-->>C: Return container
  else failure
    D-->>T: Error
    T-->>A: Error
    A-->>C: wrap error "run aerospike: %w"
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • stevenh

Poem

A carrot-keen coder, I twitch with delight,
Swapping old gears for Run’s cleaner bite.
Ports pop, envs hum, waits align tight—
Fewer knobs to twist, everything right.
Thump-thump goes my paw: refactor in sight! 🥕🐇

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The title succinctly describes the main change by indicating that the Aerospike module is updated to use the Run function, matching the changeset’s core purpose and following conventional commit styling.
Description Check ✅ Passed The description clearly outlines the change to use the Run function in the Aerospike module, explains its motivation to migrate to the new API, and references related issues, making it relevant to the changeset.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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

@netlify
Copy link

netlify bot commented Sep 26, 2025

Deploy Preview for testcontainers-go ready!

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

🧹 Nitpick comments (1)
modules/aerospike/aerospike.go (1)

32-43: Consider waiting on infoPort as well for consistency.

You expose 3003/tcp but don’t wait on it. Optional, but aligning waits with exposed ports can reduce flakiness.

Example (after adding nat import):

 			wait.ForLog("migrations: complete"),
 			wait.ForListeningPort(nat.Port(port)).WithStartupTimeout(10*time.Second),
 			wait.ForListeningPort(nat.Port(fabricPort)).WithStartupTimeout(10*time.Second),
 			wait.ForListeningPort(nat.Port(heartbeatPort)).WithStartupTimeout(10*time.Second),
+			wait.ForListeningPort(nat.Port(infoPort)).WithStartupTimeout(10*time.Second),
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9f0e4cd and 201192c.

📒 Files selected for processing (1)
  • modules/aerospike/aerospike.go (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
modules/aerospike/aerospike.go (4)
options.go (3)
  • ContainerCustomizer (22-24)
  • WithExposedPorts (464-469)
  • WithWaitStrategy (376-378)
wait/all.go (1)
  • ForAll (44-48)
wait/log.go (1)
  • ForLog (118-120)
wait/host_port.go (1)
  • ForListeningPort (67-69)
⏰ 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). (2)
  • GitHub Check: lint (modules/aerospike) / lint: modules/aerospike
  • GitHub Check: Analyze (go)
🔇 Additional comments (3)
modules/aerospike/aerospike.go (3)

45-45: LGTM: module opts + user opts merged in correct order.

Module defaults first, then user overrides.


47-47: Migration to testcontainers.Run looks correct.

Uses Run(ctx, img, opts...) with ContainerCustomizer options as intended.


54-54: Confirm intent to return a possibly non-nil container on error.

Pattern can be useful for cleanup (Terminate), but please confirm this matches other modules’ behavior.

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