[fernandezcuesta]

Description of your changes

Add support for running tests with custom versions for mariadb and mssql.

Will not wire it up yet (as we have for Pg, which checks the version on every connect) until this is actually used.

Fixes: #374

I have:

• Read and followed Crossplane's [contribution process].
• Run make reviewable to ensure this PR is ready for review.

How has this code been tested

n/a, same logic as for Postgres.

[chlunde]

"14.2.1" → 140201 — I think PG would report 140002 here? server_version_num is major*10000 + minor since PG 10:

$ docker run -d --rm --name pg -e POSTGRES_PASSWORD=x postgres:14.2
$ docker exec pg psql -U postgres -Atc \
    "SELECT current_setting('server_version'), current_setting('server_version_num')"
14.2 (Debian 14.2-1.pgdg110+1)|140002

# and on postgres:18
18.6 (Debian 18.6-1.pgdg13+2)|180006

So the 160200 in the postgres client's doc comment is wrong and has been for a while, and this matches the comment rather than the server 🤔

[fernandezcuesta]

yeah good catch

[chlunde]

For #162 I think we need to tell MariaDB from MySQL, not just how new it is — their database-level ALL differs:

$ docker run -d --rm --name my -e MYSQL_ROOT_PASSWORD=x mysql:8.4
$ docker exec my mysql -uroot -px -e \
    "CREATE DATABASE d3; CREATE USER u3@'%' IDENTIFIED BY 'p';
     GRANT ALL ON d3.* TO u3@'%'; REVOKE TRIGGER ON d3.* FROM u3@'%';
     SHOW GRANTS FOR u3@'%';"
8.4.11
GRANT SELECT, ..., CREATE ROUTINE, ALTER ROUTINE, EVENT ON `d3`.* TO `u3`@`%`

# mariadb:12 — same statements
12.3.3-MariaDB-ubu2404
GRANT SELECT, ..., CREATE ROUTINE, ALTER ROUTINE, EVENT,
      DELETE HISTORY, SHOW CREATE ROUTINE ON `d3`.* TO `u3`@`%`

MariaDB 12 has DELETE HISTORY and SHOW CREATE ROUTINE on top. This cuts at the first -, which is exactly where MariaDB sits in that version string. Could we keep the raw string around too?

[fernandezcuesta]

I guess something we can do is instead of returning the version as a number, return a struct.

I'd prefer to keep this PR focused on the version plumbing and defer the MariaDB detection to #162, where we'll actually need it.

When that lands, the plan would be to change GetServerVersion to return a VersionInfo struct:

type VersionInfo struct {
    Number int    // e.g. 101106
    Raw    string // e.g. "10.11.6-MariaDB"
    Engine string // "mysql", "mariadb", "postgresql", "mssql"
  }

That way ParseVersion stays a pure number parser, and engine detection lives in each client's GetServerVersion where it belongs. The interface change touches
all engines, so it makes more sense as part of the PR that actually consumes it, or in a separate one due to the large number of files affected.

Does that work, or would you rather we implement this here?

[chlunde]

I don't think MSSQL_VERSION reaches anything — setup_mssql still seds the image line and replaces it with ${MSSQL_IMAGE}. Could we pick one? envsubst like postgres/mariadb perhaps, but it'd need a ${MSSQL_VERSION:-2019-CU32-ubuntu-20.04} default the way they have one — CI passes it empty on push/PR (see the ci.yml comment), so otherwise the tag ends up blank.

[chlunde]

This is a second sql.Open per reconcile if anyone follows it — a mysql grant reconcile with no drift is one query today, so it would double. Same concern I had on #345, except now it'd be in twelve places. Worth caching it per DSN, or waiting for one of the pooling PRs, before we recommend it? (also, no db in scope here, it's built inline in the return)

[fernandezcuesta]

removed the comment from all 12 reconcilers, but the only option we've is a kind of per-DSN caching, but that comes with a cost too. Shall we track this in a separate issue?

[chlunde]

worth a {version: "8", wantErr: true} here too — both cases below error via strconv anyway, so nothing actually exercises the len(parts) < 2 guard: relaxing it to < 1 still passes the whole table, and it panics on "8".

[fernandezcuesta]

done