Make this a real fork: module path github.com/block/mysql, driver name block-mysql - #3
Conversation
Coverage Report for CI Build 34057393127Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage decreased (-0.2%) to 84.439%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions9 previously-covered lines in 5 files lost coverage.
Coverage Stats
💛 - Coveralls |
d00fcaa to
c0ef5e0
Compare
The two capabilities this fork carries are reached from strata through
`(*sql.Conn).Raw` and a structural interface assertion, which means a
consumer that builds without the `replace` compiles clean and then fails at
runtime on every statement. `replace` is not inherited across module
boundaries, so any downstream module importing a library built on this fork
silently links upstream instead — and a distinct module path is the only
mechanism Go has for expressing a dependency that is not substitutable.
So: `module github.com/block/mysql`, and the driver registers as
`block-mysql`. The rename is required, not cosmetic — a dependency graph that
still reaches upstream go-sql-driver anywhere links both packages, and two
`sql.Register` calls under one name panic at init.
Edits to upstream files are held to the module path and driver name so that
merging upstream stays mechanical. Three `sql.Open("mysql", ...)` call sites
in driver_test.go move to `driverNameTest` like every other site in the file,
which is what upstream's own `-ldflags` override already assumed.
The README gains a preamble stating what the fork adds, what it changes and
why, the `errors.As` hazard when a binary links both drivers, and how to merge
upstream forward.
Verified: full suite green against MySQL 8.0.44, gofmt and staticcheck clean.
State plainly that we depend on these features and keep a tracking fork until upstream carries them, rather than characterising upstream's responsiveness or roadmap. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Linux with MySQL LTS (9.7, 8.4, 8.0), plus Go 1.26 and 1.25 against the newest
MySQL. Drops the macOS and Windows runners and the four MariaDB versions:
Block deploys none of them, so the jobs cost CI time without telling us
anything we act on. 25 test jobs become 5.
It also removes exposure to a Windows-runner TCP dial flake ("connectex: A
connection attempt failed...") that hits a different matrix cell each run.
That flake is not ours — it reproduces on this fork's master without any of
these changes, and on upstream go-sql-driver's own CI.
This is a deliberate divergence in an upstream-owned file, so the README now
lists CI alongside the module path as the second and last place the fork edits
upstream, and states the narrower support scope explicitly.
The previous wording ruled connect/TLS/packet-path changes out of the fork entirely. The actual constraint is merge cost, not subject matter, so say that: additions are cheapest as new files or new methods. Leaves room for features that need a different shape. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
dff4014 to
6346a1e
Compare
There was a problem hiding this comment.
🟡 Changes recommended
README.md still contains later sections that reference the upstream import path/godoc URLs (e.g., import "github.com/go-sql-driver/mysql"), which conflicts with the new module path and can misdirect users.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR turns the repository into a distinct Go module fork (github.com/block/mysql) and avoids runtime conflicts by registering the SQL driver under a new name (block-mysql), with accompanying documentation/test/CI updates to match.
Changes:
- Updated the Go module path to
github.com/block/mysqland adjusted references accordingly. - Changed the default registered driver name to
block-mysqland updated tests/examples to use the appropriate driver name variable(s). - Narrowed the GitHub Actions CI matrix to Linux + selected MySQL versions, and expanded the README with fork-specific context.
File summaries
| File | Description |
|---|---|
| utils.go | Updates TLS config example to use block-mysql driver name. |
| README.md | Adds fork overview and updates install/usage examples for new module path/driver name. |
| go.mod | Changes module path to github.com/block/mysql and documents rationale. |
| driver.go | Updates package docs and changes default driver registration name to block-mysql. |
| driver_test.go | Updates -ldflags path and uses driverNameTest in remaining hardcoded sql.Open call sites. |
| .github/workflows/test.yml | Narrows CI matrix to Linux and MySQL-only versions. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
🤖 Adversarial correctness review — The argument for the module path is the right one and it's worth restating in review terms, because it is the whole justification for the churn: The mechanical side is complete. Every ⭐ What I'd want changed before merge is documentation of one silent failure mode, and a test for the two lines this PR is entirely about. Both are cheap, and both are about the thing the fork exists to prevent.
1 — the silent case in "Linking both drivers" is the one that isn't listed (low-med)The section names the Every registry in this package is a package-level global — Those are migration chores, not traps. The trap is db, err := sql.Open("mysql", dsn) // resolves — to upstreamsucceeds, connects, and behaves correctly right up until something reaches for 2 — nothing pins either of the two lines this PR consists of (low-med)Both mutations survive the full suite:
The second is unsurprising (no file imports the module path, so nothing observes it) and the first follows from Ordinarily "add a test asserting a constant" is not worth the line. Here it is, because of the workflow this PR documents three sections above: git merge upstream/master
One test file covers both, needs no server, and killed both mutations when I tried it: package mysql
import (
"database/sql"
rtdebug "runtime/debug" // aliased: const.go:14 already declares `debug`
"slices"
"testing"
)
func TestForkIdentity(t *testing.T) {
bi, ok := rtdebug.ReadBuildInfo()
if !ok || bi.Main.Path != "github.com/block/mysql" { ... }
if !slices.Contains(sql.Drivers(), "block-mysql") { ... }
}
3 — one code sample still on the upstream path, in the section that can't use a blank import (low)
The Installation and Usage blocks were both repointed; this one wasn't, and it is the worst one to miss. Every other remaining mention of the upstream path in the README is a link — godoc, wiki, issues — where pointing at upstream is correct. This is a copy-pasteable import, in the one section whose whole premise is that the reader needs the package, not just the driver. Follow it and you get upstream's
4 — the job counts (low)The narrowing is a good call and the shape of it checks out: So 5 is right and 25 isn't — it's 23 (26 vs 8 if you count Verified — the local run, and five attacks that dissolvedLocal: Attack that dissolved: a missed Attack that dissolved: Attack that dissolved: dangling README links. Attack that dissolved: coveralls flag collisions from the narrowed matrix. Leak check: clean. No internal identifiers in the diff. The PR body names strata and The matrix expansion and the fork-identity probe were run with throwaway files that have since been removed, and both mutations were restored from backup; the worktree is clean at This review was generated by Claude Code (claude-opus-5). |
aparajon
left a comment
There was a problem hiding this comment.
🤖 Approving — the module-path argument is the right one (replace isn't inherited across module boundaries, so a library built on this fork hands its consumers upstream silently), the driver rename follows from it necessarily, and the mechanical side is complete: all 19 sql.Open sites go through driverNameTest, one sql.Register, and the suite's only failure is a pre-existing local_infile one that fails identically on the merge base.
Findings in my review comment — two low-med worth acting on: the one silent both-linked failure mode that "Linking both drivers" doesn't list (a missed sql.Open("mysql", …) resolves to upstream), and the fact that both of this PR's substantive lines survive mutation, which matters given the documented merge-forward workflow. Plus a stale import snippet and a job count.
This stamp was left by Claude Code (claude-opus-5).
Four items from the review on #3: 1. "Linking both drivers" documented the errors.As type-identity hazard but not the one that mirrors this PR's own thesis: with both drivers linked, a call site still saying sql.Open("mysql", ...) resolves to upstream and keeps working, until something reaches for QueryResultContext or Warnings() through (*sql.Conn).Raw. Everything else in the both-linked surface fails loudly (the registries are package-level globals and name the cause on use), so the one silent case is worth naming. 2. Nothing pinned the two lines this PR consists of. Both mutations — the driver name back to "mysql", the module path back to upstream's — build clean and pass the full suite, and they are exactly the two places every `git merge upstream/master` has a conflict to resolve. fork_test.go asserts both: debug.ReadBuildInfo().Main.Path reads the real go.mod rather than a copy, and sql.Drivers() reads what init actually registered. Verified it kills both mutations. 3. The LOAD DATA LOCAL INFILE section still had `import "github.com/go-sql-driver/mysql"` — the one copy-pasteable import left on the old path, in the section whose premise is needing the package directly. Following it registers a local file in upstream's fileRegister while connections come from block-mysql. Its godoc link now points at the fork, which documents the fork's additions too; the remaining upstream links are provenance and stay. 4. "5 CI jobs rather than 25" was wrong. Ran the workflow's matrix generator at both revisions: 21 base combinations + 2 appended include entries = 23 test jobs before, 3 + 2 = 5 after.
* Use the block-mysql driver strata links github.com/block/mysql — Block's fork of go-sql-driver/mysql — for capabilities upstream does not carry, and that fork is moving from a `replace` directive to its own module path (block/mysql#3), because `replace` is not inherited across module boundaries and so cannot reach consumers of a library. Once the path differs the two packages declare distinct types, and strata hands spirit a `*mysql.Config` directly (applier.Target.Config), so this is not optional: without it strata does not compile. A binary linking both would also carry two `*mysql.MySQLError` types, across which `errors.As` silently returns false. Mechanical: the import path, and `sql.Open("mysql", ...)` -> `"block-mysql"`. The DSN format, Config, and the rest of the API are unchanged, since the fork tracks upstream. Comments describing upstream driver behaviour still say go-sql-driver, which remains accurate — the fork inherits it. gtid_test.go also gets a sadscan:disable for a pre-existing false positive: the GTID source id 11111111-2222-3333-4444-555555555555 contains a 16-digit run starting with 4, which the Visa PAN rule matches. It is a MySQL server UUID, not card data. The line is unrelated to this change but the file is staged by it, which is what surfaced the finding. The dependency is pinned to the block/mysql PR branch and must be re-pointed at its master commit before this merges. Verified: build, vet and gofmt clean; full test suite passes against MySQL 8.0.44 (33 packages, 0 failures). * Pin block/mysql at merged master (block/mysql#3) * dbconn: export DriverName, and say what a returned DSN is bound to From review of #1219. The type-identity argument the PR body makes covers the surfaces the compiler guards; EnhanceDSNWithTLS is the one that escapes it, because it takes a string and returns a string. initRDSTLS registers "rds" in block/mysql's package-global TLS registry, and EnhanceDSNWithTLS hands back a DSN that names that entry. A consumer that opens the result with upstream go-sql-driver gets: invalid value / unknown config name: rds at connect time, with nothing in it about drivers. block/schemabot is the live case: pkg/mysqlconn.ConnectionDSN calls EnhanceDSNWithTLS and then opens with sql.Open("mysql", ...) — and it imports none of the driver-typed API, so the compile-time canary never fires for it. It would build clean, deploy, and fail on the first RDS dial after bumping spirit. const DriverName = "block-mysql" in pkg/dbconn makes the consumer-side fix sql.Open(dbconn.DriverName, ...), which stays correct through any future move without every consumer knowing the name. The doc comments on both DriverName and EnhanceDSNWithTLS state the coupling. Production call sites now use it, including pkg/lint/load.go — the one production sql.Open in the module with no test behind it (the reviewer mutated it back to "mysql" and all 33 packages still passed). A constant cannot be half-replaced. pkg/testutils keeps a private copy: dbconn's own tests import testutils, so importing dbconn there is a cycle. It is one const instead of nine literals. Also confirmed the sadscan directive rather than assuming it: `sadscan pkg/change/gtid_test.go` with the comment removed reports rule_id sq.pii.cc.visa at line 283, and reports nothing with it in place. 33 packages, 0 failures against MySQL 8.0.44.
* Use the block-mysql driver block/mysql is Block's fork of go-sql-driver/mysql, moving to its own module path (block/mysql#3) because a `replace` directive is not inherited across module boundaries. polt reaches the driver through spirit's dbconn, and spirit is moving with it (block/spirit#1219), so polt follows to keep one driver — and one set of driver types — in the binary. Mechanical: the import path, and `sql.Open("mysql", ...)` -> `"block-mysql"`. Nothing crosses a type boundary here; pkg/test's SetupDB reads fields off *mysql.Config to build a DSN string rather than handing the struct to spirit, so no API had to change. go-sql-driver leaves go.mod entirely, including as an indirect dependency. The block/mysql and block/spirit pins point at unmerged PR branches and must be re-pointed at merged commits before this merges. Verified: build, vet and gofmt clean; all 8 packages pass against MySQL 8.0.44. * Pin block/mysql at merged master (block/mysql#3) * Pin block/spirit at merged main (block/spirit#1219); README: name the fork block/spirit#1219 squash-merged as dc3d4c9f, so the pin moves off the PR branch and onto a commit that is an ancestor of spirit main. The Dependencies section still named go-sql-driver, which after this change is not in polt's module graph at all.
* mysqltopo: use the block-mysql driver strata links github.com/block/mysql — Block's fork of go-sql-driver/mysql — for capabilities upstream does not carry. That fork is moving from a `replace` directive to its own module path (block/mysql#3), because `replace` is not inherited across module boundaries and so cannot reach consumers of a library. Once the path differs, a binary linking both packages gets two distinct `*mysql.MySQLError` types, and `errors.As` across that boundary silently returns false. convertError here depends on exactly that assertion to map 1062 to NodeExists and 1213/1205 to Timeout, and strata's own test helpers assert on the errors this package returns — so mysqltopo and strata must agree on which package the type comes from. Scope is deliberately mysqltopo only: it is Block-added, so editing it costs nothing when merging upstream vitess. The remaining go-sql-driver imports live in upstream-owned endtoend tests, which keep using upstream — the module stays in go.mod and both can coexist. The dependency is pinned to the block/mysql PR branch and must be re-pointed at its master commit before this merges. Verified: go/vt/topo/mysqltopo tests pass against MySQL 8.0.44. * go mod tidy: block/mysql is a direct dependency now * Pin block/mysql at merged master (block/mysql#3) * mysqltopo: state the driver invariant this package actually holds The import comment claimed one driver in the binary; eight end-to-end test files still import go-sql-driver, so that is false. The local invariant is both true and sufficient: every *sql.DB this package opens uses the driver whose *mysql.MySQLError convertError asserts on. Add a package const for the name and use it at all five sql.Open sites, so that invariant is one grep rather than five, and fix the convertError comment to credit block-mysql.
Why
This fork's two capabilities —
QueryResultContext(#1) andWarnings()(#2) — are consumed through(*sql.Conn).Rawand a structural interface assertion. That's good for decoupling, but it has a sharp consequence: a consumer that builds without thereplacedirective compiles clean and then fails at runtime on every statement.And
replacecannot fix that for consumers, because it isn't inherited across module boundaries. Any downstream module that imports a library built on this fork silently links upstream go-sql-driver instead. A distinct module path is the only mechanism Go has for expressing a dependency that is not substitutable.We rely on both features today, so we need a tracking fork to carry them until such a time as they are merged upstream.
QueryResultContextis raised as go-sql-driver/mysql#1793 and remains open; if upstream adopts these, the fork can be retired and consumers move back to the upstream path.What changes
module github.com/block/mysqlin go.mod. Consumers drop thereplace.block-mysql, notmysql. This is required rather than cosmetic: now that the module path differs, a dependency graph that still reaches upstream go-sql-driver anywhere links both packages into one binary, and twosql.Registercalls under the same name panic at init.errors.Ashazard when both drivers are linked, and how to merge upstream forward.sql.Open("mysql", ...)call sites indriver_test.gomove todriverNameTest, matching every other site in the file — which is what upstream's own-ldflagsoverride already assumed.Edits to upstream-owned files are held to exactly two things: the module path / driver name, and the CI matrix. The capabilities themselves live in files upstream doesn't have, so merging
upstream/masterstays near-mechanical — this branch rebased onto the latest upstream merge (5 commits touching dsn.go, connector.go, connection.go and packets.go) with zero conflicts.Note for consumers linking both drivers
Because the module path differs, this package's types are distinct from upstream's even though the source is identical. Most importantly, an
*mysql.MySQLErrorfrom this package will not satisfy anerrors.Asagainst upstream's, and the check returns false silently rather than failing loudly. Anyone with error-inspection sites should audit which package each one imports. For strata specifically that means migrating block/vitess'smysqltopoontoblock-mysql; block/spirit turns out not to need it, since no strata error-inspection site sees a spirit-originated error.Verification
-race -parallel 10,local_infile=1)gofmt -lclean,staticcheck ./...clean🤖 Generated with Claude Code
Review follow-up (
e3ae89d): addedfork_test.go, pinning the two lines this PR consists of — the module path (read from the realgo.modviadebug.ReadBuildInfo) and the registered driver name (read fromsql.Drivers()). Both reverts previously built clean and passed the full suite, and both are what everygit merge upstream/masterhas a conflict on. Also documented the one silent both-linked failure mode (a missedsql.Open("mysql", …)resolves to upstream), repointed theLOAD DATA LOCAL INFILEimport and its godoc link, and corrected the job count above.