Feat/implement phase e#4
Merged
Merged
Conversation
- Add DSN builder, mysqldump/mysql arg builders, fork detection, and binary-parameterized Backup/Restore subprocess runners. - Mirror the postgres stderr-discard pattern (no StderrPipe race); pass password via MYSQL_PWD env. - Add go-sql-driver/mysql; underscore dir stays out of go build ./....
…equirement go test ./... skips underscore-prefixed dirs, so internal/driver/_mysqlcommon unit tests were silently excluded from make test and CI. Name _*-prefixed packages explicitly so the arg-builder/version tests actually run. Also document that the MariaDB driver requires the renamed mariadb-dump/mariadb binaries.
|
Warning Review limit reached
More reviews will be available in 24 minutes and 37 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughAdds full MySQL and MariaDB driver support (Phase E). A shared ChangesMySQL + MariaDB Driver Implementation
Sequence Diagram(s)sequenceDiagram
rect rgba(173, 216, 230, 0.5)
Note over App,Registry: Driver Registration (init)
App->>Registry: register mysql.Driver
App->>Registry: register mariadb.Driver
end
rect rgba(144, 238, 144, 0.5)
Note over Caller,mysqldump: Backup Flow
Caller->>mysql.Driver: Connect(ctx, profile)
mysql.Driver->>mysqlcommon.NewConn: NewConn(ctx, profile, "mysqldump", "mysql", "mysql.connect")
mysqlcommon.NewConn->>sql.DB: Open(DSN) + PingContext (3 retries)
sql.DB-->>mysqlcommon.NewConn: *sql.DB
mysqlcommon.NewConn-->>Caller: *Conn
Caller->>Conn: Backup(ctx, opts, writer)
Conn->>mysqldump: exec with MYSQL_PWD + BuildDumpArgs
mysqldump-->>Conn: SQL dump stream
Conn-->>Caller: nil or toolErr
end
rect rgba(255, 200, 100, 0.5)
Note over IntegrationTest,MariaDB Container: Integration Test Flow
IntegrationTest->>MariaDB Container: startMariaDB (Testcontainers)
MariaDB Container-->>IntegrationTest: host:port
IntegrationTest->>drivertesting.RunDriverSuite: RunDriverSuite(driver, profile, fixtures)
drivertesting.RunDriverSuite->>Conn: Inspect / Backup / Restore
drivertesting.RunDriverSuite->>MariaDB Container: SELECT COUNT(*) FROM widgets
MariaDB Container-->>drivertesting.RunDriverSuite: 2 rows
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 57-61: Remove the mysql-client package from the apt-get install
command that currently installs postgresql-client, mysql-client, and
mariadb-client. Keep only postgresql-client and mariadb-client, since
mariadb-client provides mysql-compatible binaries and installing both
mysql-client and mariadb-client together can cause nondeterministic CI failures
due to package conflicts on Ubuntu.
In `@internal/driver/_mysqlcommon/args.go`:
- Around line 11-23: The BuildDumpArgs and BuildRestoreArgs functions do not
include the SSLMode from the Profile parameter in the generated CLI arguments,
causing mysqldump and mysql to use default SSL settings instead of the profile's
explicit TLS policy. Add the --ssl-mode flag to the args slice in both functions
by mapping p.SSLMode to the corresponding CLI values (DISABLED, REQUIRED,
VERIFY_CA, VERIFY_IDENTITY, or PREFERRED), ensuring the TLS policy is
consistently applied across all database operations including backup and
restore.
In `@internal/driver/_mysqlcommon/conn.go`:
- Line 129: The issue is that appending MYSQL_PWD to os.Environ() without
removing preexisting MYSQL_PWD entries can result in duplicate environment
variables, causing the tool to read the wrong password value. Fix this by
filtering the environment variables to remove any existing MYSQL_PWD entries
before appending the new one. This filtering needs to be applied at both
locations where cmd.Env is assigned with MYSQL_PWD (at line 129 and line 147),
ensuring only a single, correct MYSQL_PWD entry exists in the command
environment.
In `@internal/driver/_mysqlcommon/dsn.go`:
- Around line 20-23: The DSN function currently uses fmt.Sprintf with manual
string formatting, which fails to properly escape special characters in database
names, usernames, and passwords. Replace the manual string formatting approach
with mysql.Config by constructing a mysql.Config struct with the appropriate
fields from the driver.Profile parameter (User, Password, Host, Port, Database),
setting the ParseTime and TLS parameters appropriately based on p.SSLMode, then
call FormatDSN() on the config object to generate a properly escaped DSN string.
In `@README.md`:
- Line 42: The native-tools sentence on line 42 describes mysqldump and
mariadb-dump support as "in v1.0" (future release), which conflicts with the
current Phase E status indicating these tools already work today. Remove or
modify the "in v1.0" qualifier from the description of mysqldump and
mariadb-dump to accurately reflect that MySQL and MariaDB tooling support is
currently available, not planned for a future version. Update the text to show
these tools are already part of the current functionality rather than a future
feature.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9076bb6f-0486-4b2c-b265-a5cb3f2660ed
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (17)
.github/workflows/ci.ymlCHANGELOG.mdMakefileREADME.mddocs/DRIVERS.mdgo.modinternal/app/drivers.gointernal/driver/_mysqlcommon/args.gointernal/driver/_mysqlcommon/args_test.gointernal/driver/_mysqlcommon/conn.gointernal/driver/_mysqlcommon/dsn.gointernal/driver/_mysqlcommon/version.gointernal/driver/_mysqlcommon/version_test.gointernal/driver/mariadb/driver.gointernal/driver/mariadb/integration_test.gointernal/driver/mysql/driver.gointernal/driver/mysql/integration_test.go
Two CI failures from PR #4: - Windows: ./internal/driver/_*/ is a bash glob PowerShell doesn't expand, so go test got a literal path and failed. Name _mysqlcommon by import path instead (portable across shells); mirror in the Makefile. - Ubuntu: mariadb-client Conflicts with mysql-client-core, so they can't co-install. Install mysql-client from Ubuntu and pull mariadb-client from MariaDB's official APT repo, which is built to coexist.
The Cancel_PropagatesToSubprocess subtest raced a 150ms sleep against the dump duration, then asserted Backup returned non-nil. That's flaky across engines: mysqldump/mariadb-dump dump the 5 MiB seed far faster than pg_dump, so the process finished before cancel() fired and Backup returned nil (CI: TestSuite_MySQL/MariaDB failed, Postgres passed). Cancel the context BEFORE starting Backup instead. exec.CommandContext then refuses to start (or immediately kills) the subprocess, so Backup returns a non-nil error deterministically on every engine — proving ctx is wired to the process without a timing bet. Drops the now-unneeded seedCancelLoad helper.
…l DSN, dedup MYSQL_PWD Addresses CodeRabbit review on PR #4: - BuildDumpArgs/BuildRestoreArgs now pass --ssl-mode (mapped from Profile.SSLMode) so backup/restore honor the same TLS policy the connect DSN does, instead of falling back to the client default (PREFERRED). Adds tests. - DSN now uses mysql.Config.FormatDSN (the driver's canonical builder) instead of hand-formatting. Note: FormatDSN escapes the DBName path but not user/passwd, so the ':'-in-username limitation stands (documented honestly). - withMySQLPwd strips any pre-existing MYSQL_PWD from the child env before setting ours, guaranteeing a single, correct value. - README: drop the stale 'in v1.0' qualifier — MySQL/MariaDB tools work today. Skipped the CI apt-package finding: it targets an outdated diff (the mysql-client + mariadb-client install line it flags no longer exists; CI now uses mysql-client from Ubuntu + mariadb-client from MariaDB's repo, the native-client design).
…orks) The --ssl-mode flag added for CodeRabbit's SSLMode-propagation finding broke CI: mariadb-dump rejects --ssl-mode (MariaDB uses --ssl/--skip-ssl, not MySQL's flag), so both BackupRestore_Roundtrip suites failed with exit 7. SSLMode is still honored by the connect DSN; propagating it to the dump/restore tools needs per-fork flag handling and is tracked as a follow-up (see the NOTE in args.go). Keeps the other review fixes (canonical DSN, MYSQL_PWD dedup, README).
This was referenced Jun 21, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Phase E — MySQL + MariaDB drivers
Adds MySQL and MariaDB as siphon drivers, both built on a shared
_mysqlcommonpackage and integrated with the Phase DRunDriverSuiteharness. This is the payoff for the Phase D investment: each new engine is a ~30-line thin wrapper.What's included
internal/driver/_mysqlcommon/— shared code for the two forks:mysqldump/mariadb-dumparg builder, fork detection (DetectFork).Connimplementing the fulldriver.Conncontract once:Inspect(viainformation_schema),Backup(shell out to the dump tool, stream stdout),Restore(pipe SQL into the client over stdin), sha256Verify, and a bounded connect-probe retry (jobs.Retry, mirroring Postgres). Password passed viaMYSQL_PWD(no argv leak); stderr discarded (avoids theStderrPipe/cmd.Wait()race the Postgres driver documents).internal/driver/mysql/andinternal/driver/mariadb/— thin wrappers (~30 lines each) that inject the fork-specific binary names and declare capabilities honestly (Parallel: false— the dump tools are single-threaded). Each registers itself viainit().internal/app/drivers.go(notcli/root.go, per thecli-may-not-import-domaindepguard boundary).TestSuite_MySQL(mysql:8.0) andTestSuite_MariaDB(mariadb:11) run on the sharedRunDriverSuiteharness via testcontainers, giving each engine the four contract tests for free (connect/inspect, backup→restore round-trip, cancel propagation, bad-credentials sentinel).Notable decisions / deviations from the original plan
Conn(vs the plan's per-driverInspect/Verifycopies) — keeps the fork difference to just the binary names and avoids divergence.app/drivers.go, notcli/root.go— the plan's location would have violated depguard.int(port.Num())for testcontainers v0.42.0 (theMappedPortreturn type exposes.Num(), not.Int()).go test ./...silently skips underscore-prefixed dirs, so the_mysqlcommonunit tests were excluded frommake testand CI. The Makefile and CI now name./internal/driver/_*/explicitly so those tests actually run.Verification
make tidyclean ·make test(14 packages, incl_mysqlcommon) ·make lint0 issues ·make buildok.go build -tags integration ./...compiles all three driver suites; integration-tagged lint clean.profile add --driver mysql/--driver mariadbresolve and attempt real connections;inspectfails at connect (no server), not "driver not supported".Known limitation
The MariaDB driver targets the renamed
mariadb-dump/mariadbbinaries (MariaDB 10.5+). Older MariaDB installs that ship onlymysqldump/mysqlare not yet supported (documented in the README).Deferred to Phase F
binlog incremental, cross-engine sync where MySQL/MariaDB are source or target, CDC, and wiring the
RequireCapabilitygates into verbs (streaming/parallel) once those features exist.Summary by CodeRabbit
Release Notes
New Features
Tests
Documentation
Chores