chore(modulegen): use Run function when generating modules #3445
Conversation
Enhance the module development documentation with comprehensive best practices learned from migrating all 49 modules to use the Run function: - Container struct design: enforce Container naming (not ModuleContainer), use private fields for state management - Run function pattern: detailed 5-step implementation guide with code examples - Option types: clarify when to use built-in options vs CustomizeRequestOption vs custom Option types, emphasize returning struct types not interfaces - Error handling patterns in options with practical examples - Option ordering: explain defaults → user options → post-processing - Container state inspection: best practices using strings.CutPrefix with early exit optimization - All examples follow patterns from successfully migrated modules 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Summary by CodeRabbit
WalkthroughReplaces module-specific GenericContainerRequest flows with a unified public Container type and a new Run(ctx, img, opts ...testcontainers.ContainerCustomizer) (*Container, error); updates ContainerCustomizer to return error; generator templates and tests now build option slices, call Run, wrap the returned ctr, and perform post-run inspection. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Module function
participant RunAPI as testcontainers.Run
participant Ctr as returned ctr
participant Wrapper as Module Container
participant Inspect as ctr.Inspect
Caller->>RunAPI: Run(ctx, img, moduleOpts...)
note right of RunAPI #E8F4FF: moduleOpts = defaults + user options
RunAPI-->>Caller: ctr or error
alt success
Caller->>Wrapper: wrap ctr → &Container{Container: ctr, ...}
Wrapper->>Inspect: ctr.Inspect(ctx)
Inspect-->>Wrapper: env/ports/state
Wrapper-->>Caller: populated Container (dbName,user,password)
else error
RunAPI-->>Caller: error
Caller-->>Caller: return nil, fmt.Errorf("run %s: %w", lower, err)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ 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: 1
🧹 Nitpick comments (1)
docs/modules/index.md (1)
328-344: Clarify the "wrong" example.The section shows the correct way to call built-in options and warns against using
.Customize(), but the explanation could be clearer about why this is wrong.Consider adding a brief explanation:
// ❌ Wrong: Don't use .Customize() method func WithConfigFile(cfg string) testcontainers.CustomizeRequestOption { return func(req *testcontainers.GenericContainerRequest) error { - return testcontainers.WithFiles(cfgFile).Customize(req) // Wrong! + return testcontainers.WithFiles(cfgFile).Customize(req) // Wrong: adds unnecessary indirection } }The direct call pattern
testcontainers.WithFiles(cfgFile)(req)is cleaner becauseCustomizeRequestOptionis already a function that takes*GenericContainerRequest, so calling.Customize()is redundant.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/modules/index.md(2 hunks)modulegen/_template/module.go.tmpl(1 hunks)modulegen/main_test.go(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). (6)
- GitHub Check: test (1.25.x, modulegen) / test: modulegen/1.25.x
- GitHub Check: test (1.25.x, modulegen) / test: modulegen/1.25.x
- GitHub Check: test (1.24.x, modulegen) / test: modulegen/1.24.x
- GitHub Check: test (1.24.x, modulegen) / test: modulegen/1.24.x
- GitHub Check: test (1.25.x, modulegen) / test: modulegen/1.25.x
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
modulegen/_template/module.go.tmpl (1)
22-32: LGTM!The
testcontainers.Run()invocation and error handling follow the documented pattern correctly:
- Properly handles the case where
ctrmight be nil- Wraps the container even on error (allows inspection of partial state)
- Uses consistent error message format:
"run {{ $lower }}: %w"modulegen/main_test.go (1)
429-436: Tests verify incorrect template behavior.These assertions correctly verify the current template output, but they also verify the incorrect option ordering identified in
modulegen/_template/module.go.tmpl. Specifically:
- Line 429 expects an empty
moduleOptsslice- Line 430 expects the misleading "Add default options" comment
- Line 431 expects user options to be appended immediately
When the template is fixed to follow the documented best practice (defaults → user options → post-processing), these assertions will need to be updated to:
require.Contains(t, data[16], "moduleOpts := []testcontainers.ContainerCustomizer{") require.Contains(t, data[17], "// Add default options here") // ... verify default options in slice ... require.Equal(t, "\t// Step 2: Append user-provided options", data[X]) require.Equal(t, "\tmoduleOpts = append(moduleOpts, opts...)", data[X+1])This comment is contingent on fixing the template issue flagged in
modulegen/_template/module.go.tmpl.docs/modules/index.md (5)
115-135: Excellent guidance on Container struct design.This section clearly establishes the naming convention and composition pattern for module containers. Key strengths:
- Enforces consistent
Containernaming across modules- Shows proper embedding of
testcontainers.Container- Explains when to use private vs public fields
- Includes helpful warning about legacy module names
136-186: Clear and comprehensive Run function pattern.The 5-step pattern documented here is exactly what modules should follow:
- Process custom options
- Build moduleOpts with defaults
- Append user options
- Call testcontainers.Run
- Return with proper error wrapping
The example code demonstrates best practices including conditional option handling, proper error wrapping (
fmt.Errorf("run redis: %w", err)), and correct nil container handling.Note: The template in
modulegen/_template/module.go.tmplcurrently doesn't follow this pattern (specifically steps 2-3 are out of order). See my comment on that file.
206-282: Excellent guidance on option types and patterns.This section provides clear, actionable guidance on when to use each option pattern:
- Built-in options for simple configuration
CustomizeRequestOptionfor complex multi-operation logic- Custom
Optiontype for transferring internal stateThe emphasis on returning struct types rather than interfaces is particularly valuable and prevents common API design mistakes.
347-365: Critical guidance on option ordering.This section clearly establishes the correct option ordering pattern:
- Module defaults
- User-provided options
- Post-processing options
The example code correctly demonstrates this pattern with helpful comments.
Important: The template in
modulegen/_template/module.go.tmplcurrently does NOT follow this ordering (it appends user options before defaults). This should be fixed to match the documented pattern. See my comment on that file.
367-406: Exemplary guidance on container state inspection.This section demonstrates best practices for reading container state after creation:
- Uses
strings.CutPrefix(idiomatic Go standard library approach)- Shows early exit optimization with individual
foundflags- Provides complete working example with error handling
- Summarizes best practices clearly
The pattern of checking all
foundflags together before breaking is particularly elegant:if foundDB && foundUser && foundPass { break }
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/modules/index.md(2 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). (2)
- GitHub Check: test (1.24.x, modulegen) / test: modulegen/1.24.x
- GitHub Check: Analyze (go)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/modules/index.md(2 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, modulegen) / test: modulegen/1.25.x
- GitHub Check: test (1.24.x, modulegen) / test: modulegen/1.24.x
- GitHub Check: Analyze (go)
1 participant
What does this PR do?
Updates the modulegen module template to use the
testcontainers.Run()function with themoduleOptspattern, and improves module development documentation with comprehensive best practices.Changes:
Module Template (
_template/module.go.tmpl):testcontainers.Run()instead of directly passingopts...moduleOptsslice pattern for consistent option handlingTest Updates (
main_test.go):Documentation (
docs/modules/index.md):Containernaming convention (not module-specific names)CustomizeRequestOptionvs customOptiontypesstrings.CutPrefixwith early exit optimizationWhy is it important?
Run()API patternRelated issues
testcontainers.Run()API