feat(azure): reduce time/memory by running specific sub-services #3451
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Summary by CodeRabbitRelease Notes
Summary by CodeRabbit
WalkthroughRun accepts Azurite-specific Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Caller
participant Run as azurite.Run
participant Opts as Option funcs
participant Builder as module opts builder
participant TC as testcontainers.Run
participant Container as started container
Dev->>Run: Call Run(opts...)
Run->>Opts: Iterate & apply Option funcs (Customize)
Note right of Run #bfe1d6: validate enabled services\n(default blob,queue,table)
Run->>Builder: Build entrypoint/cmd, per-service flags, exposed ports, wait strategies
Builder-->>Run: module customizers (WithCmd, WithExposedPorts, WithWaitStrategy)
Run->>TC: Call testcontainers.Run(module customizers + remaining opts)
TC-->>Container: Return running container or error
Run->>Dev: Return container or wrapped error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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)
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: 0
🧹 Nitpick comments (1)
modules/azure/azurite/options.go (1)
32-46: Consider validating against empty service list.
WithEnabledServices()can be called with no arguments, resulting in an emptyEnabledServicesslice. This would cause the container to start without any services enabled, which is likely a configuration error.Consider adding validation:
func WithEnabledServices(services ...service) Option { return func(o *options) error { + if len(services) == 0 { + return fmt.Errorf("at least one service must be enabled") + } + for _, s := range services { switch s { case blobService, queueService, tableService: default: return fmt.Errorf("unknown service: %s", s) } } o.EnabledServices = services return nil } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
modules/azure/azurite/azurite.go(1 hunks)modules/azure/azurite/azurite_test.go(1 hunks)modules/azure/azurite/examples_test.go(3 hunks)modules/azure/azurite/options.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
modules/azure/azurite/options.go (4)
options.go (1)
ContainerCustomizer(22-24)modules/azure/servicebus/options.go (1)
Option(30-30)modules/azure/eventhubs/options.go (1)
Option(30-30)generic.go (1)
GenericContainerRequest(21-27)
modules/azure/azurite/azurite.go (4)
modules/azure/azurite/options.go (1)
Option(24-24)options.go (5)
ContainerCustomizer(22-24)WithEntrypoint(438-443)WithCmd(462-467)WithExposedPorts(454-459)WithWaitStrategy(366-368)wait/wait.go (1)
Strategy(17-19)wait/host_port.go (1)
ForListeningPort(67-69)
modules/azure/azurite/examples_test.go (2)
modules/azure/azurite/options.go (1)
WithEnabledServices(33-46)modules/azure/azurite/services.go (3)
BlobService(9-9)QueueService(16-16)TableService(23-23)
modules/azure/azurite/azurite_test.go (3)
modules/azure/azurite/azurite.go (1)
Run(74-134)modules/azure/azurite/options.go (1)
WithEnabledServices(33-46)testing.go (1)
CleanupContainer(91-97)
⏰ 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/azure) / test: modules/azure/1.24.x
- GitHub Check: test (1.25.x, modules/azure) / test: modules/azure/1.25.x
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
modules/azure/azurite/examples_test.go (1)
60-60: Excellent demonstration of the service-selection feature.The examples clearly show how to enable specific services using
WithEnabledServices, which helps users understand the new API and its benefits (reduced resource usage, faster startup).Also applies to: 202-202, 297-297
modules/azure/azurite/azurite_test.go (1)
39-64: Comprehensive test coverage for service selection.The test validates both positive cases (valid services) and negative cases (invalid service), and confirms that
ServiceURLcorrectly returns errors for disabled services. The nested loop at lines 54-61 effectively verifies isolation between services.modules/azure/azurite/options.go (1)
20-30: Clean Option API design following testcontainers patterns.The
Optiontype implementsContainerCustomizerwith a NOOPCustomizemethod, which is correct since the actual option processing happens inazurite.go:Run(). This allows options to be processed twice (once for settings, once by testcontainers) without side effects.modules/azure/azurite/azurite.go (4)
77-83: Correct option processing with type-safe filtering.The implementation correctly processes only
Option-type options to build settings, while passing all options (including non-Option types likeWithInMemoryPersistence) through totestcontainers.Run. This design allows mixing azurite-specific options with generic testcontainers options.
85-90: Smart entrypoint selection based on enabled services.Using specialized entrypoints (
azurite-blob,azurite-queue) for single-service configurations is a good optimization. The comment aboutazurite-tablehaving slow shutdown is helpful context for excluding it from this optimization.
94-118: Well-structured dynamic configuration per enabled service.The implementation correctly builds command arguments, exposed ports, and wait strategies based on the enabled services. This ensures that only necessary resources are allocated and reduces startup time.
100-100: Trim/tcpsuffix from Azurite port flags
BlobPort, QueuePort, and TablePort include/tcp, but Azurite’s--blobPort,--queuePort, and--tablePortflags expect plain numeric ports. Confirm whether Azurite accepts the/tcpsuffix; if not, wrap each port constant withstrings.TrimSuffix(port, "/tcp")when building the command.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
modules/azure/azurite/options.go (2)
38-44: Consider adding a comment for the empty switch case.The validation logic is correct, but the empty case for valid services could benefit from a brief comment explaining that no action is needed for valid services, making the intent explicit.
Apply this diff if desired:
for _, s := range services { switch s { - case blobService, queueService, tableService: + case blobService, queueService, tableService: + // Valid service, continue default: return fmt.Errorf("unknown service: %s", s) } }
42-42: Suggest implementing fmt.Stringer on service for custom error output
serviceis currently an alias ofstring, so%sprints its raw value. Add afunc (s service) String() stringif you need more descriptive names in the error.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
modules/azure/azurite/options.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
modules/azure/azurite/options.go (3)
modules/azure/servicebus/options.go (1)
Option(30-30)modules/azure/eventhubs/options.go (1)
Option(30-30)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/azure) / test: modules/azure/1.24.x
- GitHub Check: test (1.25.x, modules/azure) / test: modules/azure/1.25.x
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
modules/azure/azurite/options.go (1)
23-30: LGTM! Option pattern correctly implements ContainerCustomizer.The dual-purpose design (functional option + ContainerCustomizer) is appropriate here. The NOOP Customize method satisfies the interface while actual option processing happens in the Run function, allowing module-specific logic to execute before container creation.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
modules/azure/azurite/options.go (1)
20-21: Comment correctly reflects the verified interface.The comment accurately states that the code satisfies the
testcontainers.ContainerCustomizerinterface, matching the assertion on line 21. The issue raised in the previous review has been resolved.
🧹 Nitpick comments (1)
modules/azure/azurite/options.go (1)
32-51: Consider adding duplicate service detection.The validation logic correctly checks for valid service types, but it does not detect duplicate services. If a user calls
WithEnabledServices(blobService, blobService), the function will accept it, potentially leading to duplicate port configurations or wait strategies downstream.Consider adding duplicate detection:
func WithEnabledServices(services ...service) Option { return func(o *options) error { if len(services) == 0 { services = []service{blobService, queueService, tableService} } else { + seen := make(map[service]bool) for _, s := range services { + if seen[s] { + return fmt.Errorf("duplicate service: %s", s) + } + seen[s] = true + switch s { case blobService, queueService, tableService: // valid service, continue default: return fmt.Errorf("unknown service: %s", s) } } } o.EnabledServices = services return nil } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
modules/azure/azurite/options.go(1 hunks)modules/azure/eventhubs/options.go(1 hunks)modules/azure/servicebus/options.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
modules/azure/azurite/options.go (3)
modules/azure/eventhubs/options.go (1)
Option(30-30)modules/azure/servicebus/options.go (1)
Option(30-30)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/azure) / test: modules/azure/1.24.x
- GitHub Check: test (1.25.x, modules/azure) / test: modules/azure/1.25.x
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
modules/azure/eventhubs/options.go (1)
26-26: LGTM! Comment now accurately reflects the interface.The comment correctly identifies
ContainerCustomizeras the interface being satisfied, matching the assertion on line 27.modules/azure/servicebus/options.go (1)
26-26: LGTM! Comment now accurately reflects the interface.The comment correctly identifies
ContainerCustomizeras the interface being satisfied, matching the assertion on line 27.modules/azure/azurite/options.go (1)
23-30: LGTM! Option type correctly implements ContainerCustomizer.The
Optionfunction type with a NOOPCustomizemethod properly satisfies thetestcontainers.ContainerCustomizerinterface, following the same pattern as eventhubs and servicebus modules.
mdelapenya
changed the title
|
Goes without saying, but if this PR is accepted then the service constants in |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
modules/azure/azurite/options.go (1)
32-55: Avoid slice aliasing when storing EnabledServices.Store a copy to prevent unexpected mutations from the caller’s slice.
Apply this minimal change:
- o.EnabledServices = services + // copy to avoid retaining caller's backing array + o.EnabledServices = append([]Service(nil), services...)modules/azure/azurite/azurite.go (1)
36-44: Optional: return a clearer error if a service wasn’t enabled.Currently this fails deep in PortEndpoint. Guard early for better DX.
Example change:
func (c *Container) ServiceURL(ctx context.Context, srv Service) (string, error) { + // make a quick enabled-set + enabled := false + for _, s := range c.opts.EnabledServices { + if s == srv { + enabled = true + break + } + } + if !enabled { + return "", fmt.Errorf("service %q not enabled", srv) + } port, err := servicePort(srv) if err != nil { return "", err } return c.PortEndpoint(ctx, port, "http") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
modules/azure/azurite/azurite.go(1 hunks)modules/azure/azurite/azurite_test.go(1 hunks)modules/azure/azurite/options.go(1 hunks)modules/azure/azurite/services.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- modules/azure/azurite/azurite_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
modules/azure/azurite/options.go (2)
modules/azure/azurite/services.go (4)
Service(15-15)BlobService(5-5)QueueService(8-8)TableService(11-11)options.go (1)
ContainerCustomizer(22-24)
modules/azure/azurite/azurite.go (6)
modules/azure/azurite/services.go (4)
BlobService(5-5)QueueService(8-8)TableService(11-11)Service(15-15)modules/azurite/services.go (4)
BlobService(12-12)QueueService(18-18)TableService(24-24)Service(31-31)modules/azurite/azurite.go (3)
BlobPort(15-15)QueuePort(18-18)TablePort(21-21)options.go (4)
WithEntrypoint(438-443)WithCmd(462-467)WithExposedPorts(454-459)WithWaitStrategy(366-368)modules/azure/azurite/options.go (1)
Option(24-24)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). (3)
- GitHub Check: test (1.24.x, modules/azure) / test: modules/azure/1.24.x
- GitHub Check: test (1.25.x, modules/azure) / test: modules/azure/1.25.x
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
modules/azure/azurite/services.go (1)
5-11: Public Service type and constants look good.Names, types, and values are clear and consistent with usage elsewhere.
Also applies to: 15-15
modules/azure/azurite/options.go (1)
20-21: Comment fix matches the interface assertion.Accurate and tidy.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
modules/azure/azurite/azurite.go (1)
95-108: Ports/CLI flags now correct; resolves earlier critical issueUsing port.Port() for the CLI (“--Port ”) and string(port) for exposed ports is exactly right. The wait strategy uses nat.Port, also correct.
🧹 Nitpick comments (2)
modules/azure/azurite/azurite.go (2)
61-72: Service-to-port mapping OK; minor optional refactorSwitch is fine. Optionally, use a map[Service]nat.Port to simplify and make additions easier.
117-129: Error handling pattern LGTMReturning a possibly-initialized container alongside the error is helpful for cleanup.
Optionally, document in the function comment that on error a non-nil container may be returned for caller-managed termination.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
modules/azure/azurite/azurite.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
modules/azure/azurite/azurite.go (5)
modules/azurite/services.go (4)
BlobService(12-12)QueueService(18-18)TableService(24-24)Service(31-31)modules/azurite/azurite.go (3)
BlobPort(15-15)QueuePort(18-18)TablePort(21-21)options.go (4)
WithEntrypoint(438-443)WithCmd(462-467)WithExposedPorts(454-459)WithWaitStrategy(366-368)modules/azure/azurite/options.go (1)
Option(24-24)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: test (1.24.x, modules/azure) / test: modules/azure/1.24.x
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
modules/azure/azurite/azurite.go (5)
38-44: ServiceURL centralization looks goodUsing servicePort + PortEndpoint(ctx, nat.Port, "http") is correct and avoids hardcoding ports in multiple places.
46-59: Helper delegation LGTMBlobServiceURL, QueueServiceURL, and TableServiceURL delegating to ServiceURL reduces duplication.
78-85: Verify Option wiring into RunYou type-assert azurite Option from opts (ContainerCustomizer). This requires Option to satisfy testcontainers.ContainerCustomizer. Please confirm it does; otherwise WithEnabledServices(...) won’t compile when passed to Run.
If needed, add a no-op Customize method so Option satisfies the interface:
--- a/modules/azure/azurite/options.go +++ b/modules/azure/azurite/options.go @@ - type Option func(*options) error + type Option func(*options) error + + // Ensure Option also satisfies testcontainers.ContainerCustomizer so it can be passed to Run. + func (o Option) Customize(req *testcontainers.GenericContainerRequest) error { + return nil + }
86-91: Entrypoint selection is sensibleChoosing azurite-blob/azurite-queue for a single enabled service and avoiding table due to shutdown hangs is a pragmatic default.
Consider a short comment linking to the azurite-table shutdown issue for future re-evaluation.
110-114: Aggregating module options is fineWithCmd, WithExposedPorts, and WithWaitStrategy applied together is coherent. 60s default wait deadline via WithWaitStrategy is reasonable.
|
Looks good to me thanks! Could you update the docs/azure file for this new option? |
|
Done. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/modules/azure.md (1)
64-68: Add usage example for consistency with existing options documentation.The section explains the purpose but doesn't show how to invoke the option. For consistency with the
WithInMemoryPersistenceoption documented below, add the function signature and a concrete usage example (e.g.,azurite.WithEnabledServices("blob")).Update lines 66-68 to include the actual function signature and usage pattern:
- The default Azurite container entrypoint runs all three storage services: blob, queue, and table. Use this option to specify the required services for fewer exposed ports and slightly reduced container resources. + The default Azurite container entrypoint runs all three storage services: blob, queue, and table. Use `WithEnabledServices(services ...string)` to specify the required services for fewer exposed ports and slightly reduced container resources. E.g. `azurite.WithEnabledServices("blob", "queue")`.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/modules/azure.md(1 hunks)
⏰ 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.25.x, modules/azure) / test: modules/azure/1.25.x
- GitHub Check: test (1.24.x, modules/azure) / test: modules/azure/1.24.x
- GitHub Check: Analyze (go)
mdelapenya
added
the
feature
2 participants
What does this PR do?
The default MS "azurite" container entrypoint runs the blob, queue and table services internally. The azurite Testcontainers module has an option to specify these services but it's not exposed to consumers and therefore always contains all 3 services.
This PR uses the options pattern established in the Redpanda module to allow consumers to choose the services they actually need. If only "blob" or "queue" are chosen, then the special
azurite-bloborazurite-queueentrypoint is used respectively. Note thatazurite-tableis supported in the official container but is relatively new and appears to hang on shutdown for me so I'm not using that.This also means fewer exposed ports and fewer wait strategies. FWIW, I'm seeing 30% faster startup time on my machine when running
azurite-blobvs the fullazurite.Why is it important?
Faster startup and lower memory usage, especially for the common scenario where only blob storage is needed.