chore(gcloud): use Run function #3411
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Warning Rate limit exceeded@mdelapenya has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 26 minutes and 23 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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. 📒 Files selected for processing (3)
Summary by CodeRabbit
WalkthroughRefactors all gcloud emulator modules to replace direct GenericContainerRequest/GenericContainer usage with a modular options pipeline built from testcontainers.ContainerCustomizer and testcontainers.Run. Introduces defaultPort constants per module, migrates command, files, and wait strategies into moduleOpts, updates newGCloudContainer and applyOptions signatures, and aligns tests with the new option types. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Caller
participant M as Module.Run(...)
participant AO as applyOptions(opts)
participant TC as testcontainers.Run
participant Ctr as Container
Note over M: New flow (options-based)
C->>M: Run(ctx, img, opts...)
M->>M: Build moduleOpts (ports, wait, cmd, files)
M->>AO: applyOptions(opts)
AO-->>M: updated settings / moduleOpts
M->>TC: Run(ctx, img, moduleOpts...)
TC-->>M: Container
M->>Ctr: Configure wrapper (endpoint, etc.)
M-->>C: Return Container
sequenceDiagram
autonumber
participant C as Caller
participant M as Module.Run(...)
participant R as (old) GenericContainerRequest
participant GC as (old) GenericContainer
Note over M: Old flow (request-based)
C->>M: Run(ctx, reqOpts...)
M->>R: Build ContainerRequest (ports, wait, cmd, files)
M->>R: opt.Customize(&req)
M->>GC: GenericContainer(ctx, req)
GC-->>M: Container
M-->>C: Return Container
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/gcloud/spanner/spanner.go (1)
45-61: User options are not applied to the container.The code extracts settings from
opts(lines 45-52) but never appendsoptstomoduleOptsbefore callingtestcontainers.Run(line 54). This means user-providedContainerCustomizeroptions are silently ignored.Compare with bigquery.go, which appends opts on line 63 before calling Run on line 67.
Apply this diff to append opts before container creation:
settings := defaultOptions() for _, opt := range opts { if apply, ok := opt.(Option); ok { if err := apply(&settings); err != nil { return nil, err } } } + moduleOpts = append(moduleOpts, opts...) + ctr, err := testcontainers.Run(ctx, img, moduleOpts...)
♻️ Duplicate comments (2)
modules/gcloud/pubsub.go (1)
19-40: Same option ordering issue as bigtable.go.This deprecated function has the same pattern where user opts are appended before
WithCmd, leading to the default command overriding user-provided commands. Since this is deprecated in favor ofpubsub.Run, consider aligning it with whatever pattern is chosen for the new implementation.modules/gcloud/datastore.go (1)
19-37: Same option ordering issue.User opts appended before
WithCmdat line 24, causing the default command to override user commands. Same issue asbigtable.goandpubsub.go.
🧹 Nitpick comments (2)
modules/gcloud/bigquery.go (1)
21-48: Option ordering is safer here due to WithCmdArgs.Unlike other modules that use
WithCmd(which replaces the entire command), this module usesWithCmdArgs(line 33, 39), which appends arguments. This means the order of options is less critical. However, this creates an inconsistency across gcloud modules—some useWithCmd, others useWithCmdArgs.Consider standardizing on
WithCmdArgsacross all gcloud modules where possible, or clearly document when to use each.#!/bin/bash # Description: Identify which gcloud modules use WithCmd vs WithCmdArgs # Expected: document the pattern for consistency echo "=== Modules using WithCmd ===" rg -n 'testcontainers\.WithCmd\(' modules/gcloud/ --type go echo "" echo "=== Modules using WithCmdArgs ===" rg -n 'testcontainers\.WithCmdArgs\(' modules/gcloud/ --type gomodules/gcloud/firestore.go (1)
19-25: Introduce port constants for consistency.Other modules in this PR (bigquery, spanner) define
defaultPortanddefaultPortNumberconstants to avoid hardcoding port strings. This file hardcodes"8080/tcp"in multiple locations (lines 20, 22, 37, 40).Apply this diff to introduce constants:
+const ( + defaultPortNumber = "8080" + defaultPort = defaultPortNumber + "/tcp" +) + func RunFirestore(ctx context.Context, img string, opts ...testcontainers.ContainerCustomizer) (*GCloudContainer, error) { moduleOpts := []testcontainers.ContainerCustomizer{ - testcontainers.WithExposedPorts("8080/tcp"), + testcontainers.WithExposedPorts(defaultPort), testcontainers.WithWaitStrategy(wait.ForAll( - wait.ForListeningPort("8080/tcp"), + wait.ForListeningPort(defaultPort), wait.ForLog("running"), )), }Then update line 37 and line 40 to use
defaultPortNumberand the numeric8080respectively:- "gcloud beta emulators firestore start --host-port 0.0.0.0:8080 --project="+settings.ProjectID, + "gcloud beta emulators firestore start --host-port 0.0.0.0:"+defaultPortNumber+" --project="+settings.ProjectID,- return newGCloudContainer(ctx, img, 8080, settings, "", moduleOpts...) + return newGCloudContainer(ctx, img, defaultPortNumber, settings, "", moduleOpts...)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
modules/gcloud/bigquery.go(1 hunks)modules/gcloud/bigquery/bigquery.go(2 hunks)modules/gcloud/bigquery/bigquery_test.go(1 hunks)modules/gcloud/bigquery/options.go(1 hunks)modules/gcloud/bigtable.go(1 hunks)modules/gcloud/bigtable/bigtable.go(3 hunks)modules/gcloud/datastore.go(1 hunks)modules/gcloud/datastore/datastore.go(3 hunks)modules/gcloud/firestore.go(1 hunks)modules/gcloud/firestore/firestore.go(3 hunks)modules/gcloud/gcloud.go(2 hunks)modules/gcloud/pubsub.go(1 hunks)modules/gcloud/pubsub/pubsub.go(3 hunks)modules/gcloud/spanner.go(1 hunks)modules/gcloud/spanner/spanner.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (15)
modules/gcloud/bigquery/options.go (2)
options.go (2)
WithCmdArgs(470-475)WithFiles(524-529)container.go (1)
ContainerFile(110-115)
modules/gcloud/spanner.go (4)
options.go (3)
ContainerCustomizer(22-24)WithExposedPorts(454-459)WithWaitStrategy(366-368)wait/all.go (1)
ForAll(44-48)wait/host_port.go (1)
ForListeningPort(67-69)wait/log.go (1)
ForLog(118-120)
modules/gcloud/bigquery/bigquery.go (7)
options.go (4)
ContainerCustomizer(22-24)WithExposedPorts(454-459)WithWaitStrategy(366-368)WithCmdArgs(470-475)wait/all.go (1)
ForAll(44-48)wait/host_port.go (1)
ForListeningPort(67-69)wait/http.go (1)
ForHTTP(149-151)modules/gcloud/bigquery/options.go (1)
Option(16-16)modules/gcloud/spanner/spanner.go (2)
Run(36-71)Container(19-22)modules/gcloud/pubsub/pubsub.go (2)
Run(36-77)Container(19-22)
modules/gcloud/firestore.go (4)
options.go (4)
ContainerCustomizer(22-24)WithExposedPorts(454-459)WithWaitStrategy(366-368)WithCmd(462-467)wait/all.go (1)
ForAll(44-48)wait/host_port.go (1)
ForListeningPort(67-69)wait/log.go (1)
ForLog(118-120)
modules/gcloud/gcloud.go (3)
options.go (1)
ContainerCustomizer(22-24)modules/gcloud/spanner/spanner.go (2)
Run(36-71)Container(19-22)modules/etcd/etcd.go (1)
Run(62-131)
modules/gcloud/bigtable/bigtable.go (5)
options.go (4)
ContainerCustomizer(22-24)WithExposedPorts(454-459)WithWaitStrategy(366-368)WithCmd(462-467)wait/all.go (1)
ForAll(44-48)wait/host_port.go (1)
ForListeningPort(67-69)wait/log.go (1)
ForLog(118-120)modules/gcloud/spanner/spanner.go (2)
Run(36-71)Container(19-22)
modules/gcloud/datastore.go (2)
options.go (4)
ContainerCustomizer(22-24)WithExposedPorts(454-459)WithWaitStrategy(366-368)WithCmd(462-467)wait/http.go (1)
ForHTTP(149-151)
modules/gcloud/firestore/firestore.go (5)
options.go (4)
ContainerCustomizer(22-24)WithExposedPorts(454-459)WithWaitStrategy(366-368)WithCmd(462-467)wait/all.go (1)
ForAll(44-48)wait/host_port.go (1)
ForListeningPort(67-69)wait/log.go (1)
ForLog(118-120)modules/gcloud/spanner/spanner.go (2)
Run(36-71)Container(19-22)
modules/gcloud/pubsub.go (4)
options.go (4)
ContainerCustomizer(22-24)WithExposedPorts(454-459)WithWaitStrategy(366-368)WithCmd(462-467)wait/all.go (1)
ForAll(44-48)wait/host_port.go (1)
ForListeningPort(67-69)wait/log.go (1)
ForLog(118-120)
modules/gcloud/bigtable.go (4)
options.go (4)
ContainerCustomizer(22-24)WithExposedPorts(454-459)WithWaitStrategy(366-368)WithCmd(462-467)wait/all.go (1)
ForAll(44-48)wait/host_port.go (1)
ForListeningPort(67-69)wait/log.go (1)
ForLog(118-120)
modules/gcloud/pubsub/pubsub.go (6)
options.go (4)
ContainerCustomizer(22-24)WithExposedPorts(454-459)WithWaitStrategy(366-368)WithCmd(462-467)wait/all.go (1)
ForAll(44-48)wait/host_port.go (1)
ForListeningPort(67-69)wait/log.go (1)
ForLog(118-120)modules/gcloud/spanner/spanner.go (2)
Run(36-71)Container(19-22)modules/gcloud/firestore/firestore.go (2)
Run(36-84)Container(19-22)
modules/gcloud/spanner/spanner.go (5)
options.go (3)
ContainerCustomizer(22-24)WithExposedPorts(454-459)WithWaitStrategy(366-368)wait/all.go (1)
ForAll(44-48)wait/host_port.go (1)
ForListeningPort(67-69)wait/log.go (1)
ForLog(118-120)modules/gcloud/pubsub/pubsub.go (2)
Run(36-77)Container(19-22)
modules/gcloud/bigquery.go (3)
options.go (5)
ContainerCustomizer(22-24)WithExposedPorts(454-459)WithWaitStrategy(366-368)WithCmdArgs(470-475)WithFiles(524-529)wait/http.go (1)
ForHTTP(149-151)container.go (1)
ContainerFile(110-115)
modules/gcloud/bigquery/bigquery_test.go (2)
modules/gcloud/bigquery/bigquery.go (1)
Run(43-84)options.go (2)
ContainerCustomizer(22-24)WithCmdArgs(470-475)
modules/gcloud/datastore/datastore.go (4)
options.go (4)
ContainerCustomizer(22-24)WithExposedPorts(454-459)WithWaitStrategy(366-368)WithCmd(462-467)wait/all.go (1)
ForAll(44-48)wait/host_port.go (1)
ForListeningPort(67-69)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/gcloud) / test: modules/gcloud/1.24.x
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (go)
🔇 Additional comments (20)
modules/gcloud/spanner.go (1)
19-34: LGTM! Clean migration to modular options pattern.The refactor correctly:
- Builds moduleOpts with default port exposure and composite wait strategy
- Applies module-specific Options to settings via applyOptions
- Appends all user opts to moduleOpts for container creation
- Passes accumulated options to newGCloudContainer
The pattern aligns with the broader PR migration and maintains backward compatibility.
modules/gcloud/bigtable/bigtable.go (1)
37-79: LGTM! Well-structured migration to Run pattern.The implementation correctly:
- Uses defaultPort constant for consistent port management
- Applies module-specific Options to settings before building WithCmd
- Orders customizers appropriately (defaults → WithCmd → user opts)
- Handles errors and wraps the container for cleanup on failure
- Updates error messages to reflect the new run path
The pattern is consistent with the broader refactor.
modules/gcloud/datastore/datastore.go (1)
37-79: LGTM! Consistent implementation of modular options pattern.The refactor properly:
- Introduces and uses defaultPort constants throughout
- Builds moduleOpts in the correct order (defaults → WithCmd → user opts)
- Applies Option-type customizers to settings
- Uses testcontainers.Run with accumulated moduleOpts
- Updates error messages to "run datastore"
Implementation aligns with the pattern in bigtable and other modules.
modules/gcloud/gcloud.go (2)
30-48: LGTM! Core refactor to delegate to testcontainers.Run.The changes correctly:
- Update newGCloudContainer to accept img and opts instead of constructing a request
- Delegate container creation to testcontainers.Run
- Update error message to reflect the new run path
- Maintain the same port endpoint resolution logic
This is the foundational change that enables the modular options pattern across all gcloud modules.
100-111: LGTM! Simplified applyOptions signature and implementation.The refactor correctly:
- Changes signature to accept only []ContainerCustomizer (no longer needs request)
- Extracts Option-type customizers and applies them to settings
- Wraps errors consistently with "apply option: %w"
- Returns (options, error) for use by callers
This aligns with the modular pattern where options are accumulated and applied, not mutated in place.
modules/gcloud/bigquery/options.go (1)
37-46: LGTM! Clean migration to functional option helpers.The refactor correctly:
- Replaces direct req.Cmd mutation with testcontainers.WithCmdArgs
- Replaces direct req.Files mutation with testcontainers.WithFiles
- Propagates errors from both helper calls
- Maintains the duplicate YAML guard (line 33-35)
The pattern delegates to testcontainers option helpers instead of mutating the request directly, aligning with best practices.
modules/gcloud/firestore/firestore.go (3)
37-43: LGTM: Clean moduleOpts initialization.The introduction of
moduleOptswithWithExposedPortsand composite wait strategy (ForListeningPort+ForLog) follows the new pattern consistently.
67-74: LGTM: Consistent Run and error handling.The switch to
testcontainers.Run(ctx, img, moduleOpts...)with the updated error message "run firestore" aligns with the PR's refactoring objective.
76-79: LGTM: Port endpoint resolution uses constant.Using
defaultPortinstead of a hard-coded string improves maintainability.modules/gcloud/bigquery/bigquery_test.go (2)
79-79: LGTM: More flexible error checking.Changing from
EqualErrortoErrorContainsmakes the test more resilient to error message formatting changes while still validating the key error condition.
83-85: LGTM: Test updated for new API.The
noValueOptionhelper now correctly returns aContainerCustomizerusingtestcontainers.WithCmdArgs, aligning with the new options pipeline pattern.modules/gcloud/pubsub/pubsub.go (2)
37-43: LGTM: Clean moduleOpts initialization.Port constants and composite wait strategy follow the established pattern.
62-67: LGTM: Consistent Run and error handling.Container wrapping and error messages align with the refactoring objective.
modules/gcloud/bigquery/bigquery.go (5)
18-23: LGTM!The port constant definitions are clear and well-structured, with explicit numeric suffixes to distinguish the two ports.
44-52: LGTM!The wait strategy correctly combines port listening with an HTTP readiness check on the BigQuery discovery endpoint, ensuring the emulator is fully operational before returning.
54-62: LGTM!The option application order is correct: settings are extracted and modified before being used in subsequent operations. Error wrapping provides clear context.
63-74: LGTM!Container creation follows best practices: wrapping the container even on error allows proper cleanup, and error messages provide clear module context.
76-84: LGTM!Port endpoint retrieval uses the correct port constant and protocol, with proper error handling.
modules/gcloud/spanner/spanner.go (2)
37-43: LGTM!The wait strategy correctly combines port listening with log-based readiness detection specific to the Spanner emulator.
63-71: LGTM!Port endpoint retrieval correctly uses the defaultPort constant with an empty protocol string as documented on line 35.
1 participant
What does this PR do?
Use Run function in the GCloud module
Why is it important?
Migrate modules to the new API
Related issues