chore(mysql): use Run function #3323
Conversation
Migrate MySQL module to use testcontainers.Run function instead of GenericContainerRequest/GenericContainer pattern, following the same approach as other modules. Co-authored-by: Ona <[email protected]>
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Summary by CodeRabbit
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
Use container.Inspect() instead of non-existent Env() method to extract environment variables from the running container, following the same pattern as other migrated modules. Co-authored-by: Ona <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
modules/mysql/mysql.go (2)
36-36: Simplify redundant condition.The check
len(password) != 0 && password != ""is redundant since both conditions test for non-empty strings. Consider simplifying to justpassword != "".Apply this diff:
- if len(password) != 0 && password != "" { + if password != "" {
83-83: Simplify redundant condition.The check
len(password) == 0 && password == ""is redundant since both expressions are equivalent for strings. Simplify to justpassword == "".Apply this diff:
- if len(password) == 0 && password == "" && !strings.EqualFold(rootUser, username) { + if password == "" && !strings.EqualFold(rootUser, username) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
modules/mysql/mysql.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
modules/mysql/mysql.go (2)
options.go (4)
ContainerCustomizer(22-24)WithExposedPorts(464-469)WithEnv(75-85)WithWaitStrategy(376-378)modules/mariadb/mariadb.go (1)
WithDefaultCredentials(31-48)
⏰ 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). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
modules/mysql/mysql.go (3)
68-75: LGTM!The defensive check for
container != nilbefore extracting the environment is appropriate, astestcontainers.Runmay return a partial container even on error. The error handling forcontainer.Env(ctx)is also correct.
87-92: LGTM!The MySQLContainer construction correctly uses the resolved environment values. The database field reads from
env["MYSQL_DATABASE"], which should always be set due to the default inmoduleOpts(line 60).
95-97: LGTM!Returning both the container and error allows callers to clean up resources on failure. The error message "run mysql: %w" is more descriptive than the previous "generic container: %w".
| moduleOpts := []testcontainers.ContainerCustomizer{ | ||
| testcontainers.WithExposedPorts("3306/tcp", "33060/tcp"), | ||
| testcontainers.WithEnv(map[string]string{ | ||
| "MYSQL_USER": defaultUser, | ||
| "MYSQL_PASSWORD": defaultPassword, | ||
| "MYSQL_DATABASE": defaultDatabaseName, | ||
| }, | ||
| WaitingFor: wait.ForLog("port: 3306 MySQL Community Server"), | ||
| }), | ||
| testcontainers.WithWaitStrategy(wait.ForLog("port: 3306 MySQL Community Server")), | ||
| WithDefaultCredentials(), | ||
| } | ||
|
|
||
| genericContainerReq := testcontainers.GenericContainerRequest{ | ||
| ContainerRequest: req, | ||
| Started: true, | ||
| } | ||
|
|
||
| opts = append(opts, WithDefaultCredentials()) | ||
| moduleOpts = append(moduleOpts, opts...) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify the order of credential option application.
The WithDefaultCredentials() is applied before user-provided opts. This means WithDefaultCredentials processes the default environment (MYSQL_USER=test) before any user-supplied credential options like WithUsername("root") are applied. This could lead to incorrect credential setup if users override the username/password after the defaults are processed.
Run this script to verify how the mariadb module handles similar credential processing:
Also verify the order of option application in the testcontainers.Run implementation:
🏁 Script executed:
#!/bin/bash
# Check if mariadb module applies WithDefaultCredentials before or after user options
rg -n -A 10 -B 2 'func Run\(' modules/mariadb/mariadb.goLength of output: 565
🏁 Script executed:
#!/bin/bash
# Find how testcontainers.Run applies ContainerCustomizer options
ast-grep --pattern $'func Run($_, $_, $OPTS) {
$$$
}'
# Check if options are applied sequentially
rg -n -A 20 'func Run\(ctx context.Context' --type go -g '!**/test/**' -g '!**/modules/**'Length of output: 148
🏁 Script executed:
#!/bin/bash
echo "MySQL module:"
rg -n -C2 'WithDefaultCredentials' modules/mysql/mysql.go
echo "MariaDB module:"
rg -n -C2 'WithDefaultCredentials' modules/mariadb/mariadb.goLength of output: 800
Move default credential customization after user-provided options
In modules/mysql/mysql.go, apply WithDefaultCredentials() after you append opts… (rather than before) so default credentials run on the final environment and respect any user overrides.
🤖 Prompt for AI Agents
In modules/mysql/mysql.go around lines 55 to 66, the call to
WithDefaultCredentials() is applied before appending user-provided opts, so
defaults can override user-specified env; remove WithDefaultCredentials() from
the initial moduleOpts slice and instead append it after moduleOpts =
append(moduleOpts, opts...) (or append it to the result of that append) so the
default-credential customizer runs last and only fills in missing values while
respecting user overrides.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
modules/mysql/mysql.go (2)
55-66: WithDefaultCredentials must run after user options.The past review correctly identified that
WithDefaultCredentials()at line 63 executes before user-providedoptsare appended at line 66. This means if a user callsWithUsername("root"), the sequence is:
- Default env sets
MYSQL_USER=testWithDefaultCredentials()processesMYSQL_USER=test(no changes since not root)- User's
WithUsername("root")setsMYSQL_USER=root- Container starts with
MYSQL_USER=root(invalid, should be deleted)The correct credential transformation never runs on the user's final choice.
Apply this diff to fix the option order:
moduleOpts := []testcontainers.ContainerCustomizer{ testcontainers.WithExposedPorts("3306/tcp", "33060/tcp"), testcontainers.WithEnv(map[string]string{ "MYSQL_USER": defaultUser, "MYSQL_PASSWORD": defaultPassword, "MYSQL_DATABASE": defaultDatabaseName, }), testcontainers.WithWaitStrategy(wait.ForLog("port: 3306 MySQL Community Server")), - WithDefaultCredentials(), } - moduleOpts = append(moduleOpts, opts...) + moduleOpts = append(append(moduleOpts, opts...), WithDefaultCredentials())
97-99: Container leak on validation failure.This validation runs after
testcontainers.Runhas started the container. If the check fails, the function returns an error without terminating the container, causing a resource leak.Move this validation into
WithDefaultCredentials()where it can reject invalid configurations before the container starts:func WithDefaultCredentials() testcontainers.CustomizeRequestOption { return func(req *testcontainers.GenericContainerRequest) error { username := req.Env["MYSQL_USER"] password := req.Env["MYSQL_PASSWORD"] + + if password == "" && !strings.EqualFold(rootUser, username) { + return errors.New("empty password can be used only with the root user") + } + if strings.EqualFold(rootUser, username) { delete(req.Env, "MYSQL_USER") }Then remove lines 97-99 from the
Runfunction.
🧹 Nitpick comments (1)
modules/mysql/mysql.go (1)
97-97: Simplify redundant password empty check.The condition
len(password) == 0 && password == ""is redundant. Whenpassword == ""is true,len(password) == 0is always true, so the&&makes this unnecessarily verbose.Apply this diff:
- if len(password) == 0 && password == "" && !strings.EqualFold(rootUser, username) { + if password == "" && !strings.EqualFold(rootUser, username) { return nil, errors.New("empty password can be used only with the root user") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
modules/mysql/mysql.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
modules/mysql/mysql.go (3)
options.go (4)
ContainerCustomizer(22-24)WithExposedPorts(464-469)WithEnv(75-85)WithWaitStrategy(376-378)wait/log.go (1)
ForLog(118-120)modules/mariadb/mariadb.go (1)
WithDefaultCredentials(31-48)
⏰ 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). (1)
- GitHub Check: Analyze (go)
When MYSQL_USER is set to 'root', MySQL requires: - MYSQL_USER to be removed (not set) - MYSQL_PASSWORD to be removed (not set) - MYSQL_ROOT_PASSWORD to be set instead This fixes the error: 'MYSQL_USER="root", MYSQL_USER and MYSQL_PASSWORD are for configuring a regular user and cannot be used for the root user' Co-authored-by: Ona <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
modules/mysql/mysql.go (2)
59-70: WithDefaultCredentials must run after user options.
WithDefaultCredentials()is applied before user-providedopts, so it processes the default environment before any user-supplied credential options likeWithUsername("root")are applied. This preventsWithDefaultCredentialsfrom seeing user overrides and applying the correct root-user logic.Move
WithDefaultCredentials()to run after user options:moduleOpts := []testcontainers.ContainerCustomizer{ testcontainers.WithExposedPorts("3306/tcp", "33060/tcp"), testcontainers.WithEnv(map[string]string{ "MYSQL_USER": defaultUser, "MYSQL_PASSWORD": defaultPassword, "MYSQL_DATABASE": defaultDatabaseName, }), testcontainers.WithWaitStrategy(wait.ForLog("port: 3306 MySQL Community Server")), - WithDefaultCredentials(), } - moduleOpts = append(moduleOpts, opts...) + moduleOpts = append(moduleOpts, opts...) + moduleOpts = append(moduleOpts, WithDefaultCredentials())
95-103: Container leak on validation failure.The empty password validation occurs after
testcontainers.Runhas started the container. If validation fails at line 102, the function returnsnilwithout terminating the container, causing a resource leak.Either move validation into
WithDefaultCredentials()before container creation, or terminate the container before returning:if len(password) == 0 && password == "" && !strings.EqualFold(rootUser, username) { + _ = c.Terminate(ctx) return nil, errors.New("empty password can be used only with the root user") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
modules/mysql/mysql.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
modules/mysql/mysql.go (3)
options.go (4)
ContainerCustomizer(22-24)WithExposedPorts(464-469)WithEnv(75-85)WithWaitStrategy(376-378)wait/log.go (1)
ForLog(118-120)modules/mariadb/mariadb.go (2)
WithDefaultCredentials(31-48)Run(130-188)
🪛 GitHub Actions: Main pipeline
modules/mysql/mysql.go
[error] 33-33: golangci-lint: File is not properly formatted (gci)
⏰ 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). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
modules/mysql/mysql.go (4)
34-45: Root user credential handling is correct.The logic properly handles root user authentication by removing
MYSQL_USERandMYSQL_PASSWORD, then either settingMYSQL_ROOT_PASSWORDor allowing empty passwords. The implementation is sound.Note: You may want to clean up the extra blank lines (38, 39) for consistency, though this is a minor formatting concern.
72-80: Container creation and error handling look correct.The call to
testcontainers.Runis properly handled, with the container wrapped inMySQLContainerwhen non-nil and errors appropriately wrapped with context.
82-93: Environment inspection logic is sound.The container inspection retrieves the final environment state correctly, parsing
KEY=VALUEentries withstrings.Cut. This approach properly captures the environment after all options have been applied.
105-109: Field assignments are correct.The username, password, and database are properly extracted from the inspected environment and assigned to the container struct.
modules/mysql/mysql.go
Outdated
| return func(req *testcontainers.GenericContainerRequest) error { | ||
| username := req.Env["MYSQL_USER"] | ||
| password := req.Env["MYSQL_PASSWORD"] | ||
|
|
There was a problem hiding this comment.
Fix formatting issue flagged by linter.
The pipeline reports a gci formatting error on this line. Remove or adjust the whitespace to satisfy the linter.
username := req.Env["MYSQL_USER"]
password := req.Env["MYSQL_PASSWORD"]
-
if strings.EqualFold(rootUser, username) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| username := req.Env["MYSQL_USER"] | |
| password := req.Env["MYSQL_PASSWORD"] | |
| if strings.EqualFold(rootUser, username) { |
🧰 Tools
🪛 GitHub Actions: Main pipeline
[error] 33-33: golangci-lint: File is not properly formatted (gci)
🤖 Prompt for AI Agents
In modules/mysql/mysql.go at line 33 there is a gci formatting error caused by
extra/incorrect whitespace (empty line) — remove the stray blank line or
trailing spaces at line 33 so imports/blocks are properly grouped, then reformat
the file with your formatter (gci -w or gofmt/goimports) to ensure the linter
passes.
Remove extra blank lines to comply with gci linter requirements. Co-authored-by: Ona <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
modules/mysql/mysql.go (2)
33-33: Remove extra blank line flagged by linter.The gci linter is reporting a formatting error on this blank line. Remove it to satisfy the formatter.
Apply this diff:
username := req.Env["MYSQL_USER"] password := req.Env["MYSQL_PASSWORD"] - if strings.EqualFold(rootUser, username) {
59-70: Move WithDefaultCredentials after user options to respect overrides.
WithDefaultCredentials()is applied at line 67 before user-providedoptsare appended at line 70. This means the default credential customizer runs before user customizations, preventing users from properly overriding credentials. The mariadb module applies credential defaults after user options to ensure defaults only fill missing values.Apply this diff to fix the ordering:
moduleOpts := []testcontainers.ContainerCustomizer{ testcontainers.WithExposedPorts("3306/tcp", "33060/tcp"), testcontainers.WithEnv(map[string]string{ "MYSQL_USER": defaultUser, "MYSQL_PASSWORD": defaultPassword, "MYSQL_DATABASE": defaultDatabaseName, }), testcontainers.WithWaitStrategy(wait.ForLog("port: 3306 MySQL Community Server")), - WithDefaultCredentials(), } moduleOpts = append(moduleOpts, opts...) + moduleOpts = append(moduleOpts, WithDefaultCredentials())Based on learnings
🧹 Nitpick comments (2)
modules/mysql/mysql.go (2)
40-40: Simplify redundant password check.The condition
len(password) != 0 && password != ""is redundant since an empty string has length 0. Use justpassword != ""for clarity.Apply this diff:
- if len(password) != 0 && password != "" { + if password != "" {
101-101: Simplify redundant password length check.The condition
len(password) == 0 && password == ""is redundant since an empty string always has length 0. Simplify to justpassword == "".Apply this diff:
- if len(password) == 0 && password == "" && !strings.EqualFold(rootUser, username) { + if password == "" && !strings.EqualFold(rootUser, username) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
modules/mysql/mysql.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
modules/mysql/mysql.go (3)
options.go (4)
ContainerCustomizer(22-24)WithExposedPorts(464-469)WithEnv(75-85)WithWaitStrategy(376-378)wait/log.go (1)
ForLog(118-120)modules/mariadb/mariadb.go (2)
WithDefaultCredentials(31-48)Run(130-188)
⏰ 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/mysql) / test: modules/mysql/1.25.x
- GitHub Check: test (1.24.x, modules/mysql) / test: modules/mysql/1.24.x
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
modules/mysql/mysql.go (2)
82-93: LGTM! Clean environment extraction from inspected container.The use of
container.Inspectto extract the final environment state is appropriate, andstrings.Cutis the idiomatic way to parse environment variables. Error handling is properly implemented.
101-103: Ensure testcontainer cleanup on validation failure.In modules/mysql/mysql.go (around lines 101–103), the early return on invalid passwords skips calling
container.Terminate(ctx), leaking the running container.testcontainers-godoes not auto-cleanup abandoned containers; add adefer container.Terminate(ctx)immediately after thetestcontainers.Runcall or explicitly invokecontainer.Terminate(ctx)in the validation error path.
1 participant
What does this PR do?
Use Run function in the MySQL module
Why is it important?
Migrate modules to the new API
Related issues