Repository navigation
feat(databases): apply migrations in tracked runs - #365
Conversation
Add `databases migration status`, `apply`, `baseline`, and `runs list|get` over the hosting migration-history and migration-run API. `deploy --with-migrations` now starts a tracked run per database instead of running every file through the database proxy, and reads `volcano/migrations` with the Git deploy layout: files at the top level for one database, or one directory per database. Tracked runs need the `databases.migrations` capability. Where the API does not offer it, apply, baseline, and --with-migrations refuse before anything changes and name the untracked `databases migration up --all`, which stays.
AI review: no P0-P2 findingsAdds tracked migration commands ( ⚪ P3
Not checked: The AI review read the diff, the code, and the PR's text; it ran no tests or builds and read nothing outside this repository.
Human review: needed
The AI review found no P0-P2 problems at 3440982, so the code needs no second review for bugs. Decide the points above: approve to accept them, or request changes. Suggested reviewer: |
- Refuse a migration whose name doesn't end in lowercase .sql before a tracked run or baseline sends it, naming the file to rename. Untracked `up` still runs such files. - Send the baseline prompt to stderr with --json, so stdout stays JSON. - Stop claiming a backup was taken when the run ended in its backup phase. - Name the database's directory when a per-database deploy finds it empty, and in `up`'s "not found" error.
|
Closing as stale: no commits since before 2026-11-10. The branch is kept; reopen if this is still needed. |
…ation-runs # Conflicts: # docs/databases.md # internal/apiclient/client.gen.go # internal/cmd/deploy/helpers_test.go # openapi/openapi.yaml
Rename internal/api/database_migrations.go to database_migration_runs.go and the `migration runs` commands from runs.go to runs_list.go and runs_get.go, and name the in-progress code MigrationRunInProgressCode, so #478 can rebase onto this branch without both PRs adding runs.go. Run reads and a refused run start keep only an error body's own fields, since a run's file errors can quote data.
| development has no branches or backups, so a local run applies directly and | ||
| reports `rehearsal_skipped` and `backup_skipped` warnings. | ||
|
|
||
| A file may hold several statements. It cannot `COMMIT`, `ROLLBACK`, use |
There was a problem hiding this comment.
🟡 P2: Transaction guarantees contradict the current migration API
The checked-in DatabaseMigrationFile.sql schema supports -- volcano:no-transaction on the first line, and requestFiles forwards it unchanged. Such files can use CREATE INDEX CONCURRENTLY, run outside a transaction, and retry from their start. These docs and apply --help incorrectly rule them out and promise transactional, once-only execution. Document the marker, its single-statement constraint, and its rollback/retry semantics in both places.
Found by gpt-6.1-sol
Tracked runs accept a file whose first line is `-- volcano:no-transaction`, which runs alone outside a transaction and again from its start on a retry. docs/databases.md and `migration apply --help` said such statements could not run at all.
| } | ||
| // Git deploy does not follow links, so a migration reached through | ||
| // one would apply from here and be missing there. | ||
| if d.Type()&fs.ModeSymlink != 0 { |
There was a problem hiding this comment.
⚪ P3: Symlink refusal covers files that are not migrations
The symlink check runs before the .sql extension check, so any symlink in volcano/migrations that isn't _- or .-prefixed fails ReadMigrations. That includes a symlinked README.md or notes file. Then up, migrations deploy, status, apply, baseline and deploy --with-migrations all stop with "is a symlink, which Git deploy does not follow", even though none of them would read that file. The stated reason (a migration reached through a link applies here but is missing in Git deploy) only covers .sql files and linked directories. Unless Git deploy also refuses non-SQL links, limit the refusal to symlinks named *.sql and symlinks that point to a directory (check with root.Stat).
Found by claude-opus-5-5
| // available. The list is how a client learns what an environment offers, so a | ||
| // capability it leaves out, or reports with any status but available, is | ||
| // unavailable. | ||
| func (c *Client) CapabilityAvailable(ctx context.Context, id string) (bool, error) { |
There was a problem hiding this comment.
⚪ P3: CapabilityAvailable duplicates the existing ListCapabilities call
internal/api/lavaflow.go:23 already has Client.ListCapabilities, which calls the same ListCapabilitiesWithResponse, and sharing.CheckAvailable looks up its capability on top of it. The new CapabilityAvailable makes a second wrapper for the same route. Building it on ListCapabilities, or doing the lookup in database.Service.RequireMigrationRuns the way sharing does, keeps one place that reads /capabilities.
Found by claude-opus-5-5
…hipped, and test apply under a grant
Adds the CLI surface for tracked database migrations (hosting#2267) and moves
deploy --with-migrationsonto it.Why
deploy --with-migrationsanddatabases migration uprun every file again on each call, so users must write every migration to be safe to repeat or drop the flag after the first deploy. A non-idempotent file breaks the next deploy.api/cli-surface.yamllists all five routes as a gap, Kong/volcano-hosting#2421).volcano/migrationsdirectory behaves the same from a push or a terminal.Merge order
Merge only once the production
database_migrationsflag is on: every merge here cuts a release, and--with-migrationsrefuses while the capability is off. Then, right after this PR, merge hosting#2374, which updates hosting'stests/e2e/clilane for the new behavior. That lane builds CLImain, so hosting'scli-testsfails from the moment this merges until that PR lands.localmode-e2eruns againstkong/volcano:local-nightly, which is built from hostingmainand includes hosting#2267, so the local stack offersdatabases.migrations.Commands and flags
All of them live under
volcano [cloud] databases migration, so they work in both trees.status -d <db> [--json]<db>with the database's migration history:applied,baselined,pending,changed(checksum differs),missing(recorded, no file). Names a queued or running run. Works without the capability.apply -d <db> [--no-wait] [--json]<db>toPOST migration-runswith anIdempotency-Key, then follows the run to its end, printing each phase. Prints the files it applied, every file of a failed run, warnings, and the backup name.baseline -d <db> (--all | -f <file>...) [--yes] [--json]runs list -d <db> [--json]runs get <run-id> -d <db> [--json]up -d <db> (--all | -f <file>)apply. Local mode keepsvolcano migrations deployas its alias.deploy --with-migrationsstarts one tracked run per database and waits for it, even with--no-wait. A failed run stops the deploy at the migrations step, and the resume hint namesdatabases migration apply -d <db>.How client-side migrations and tracked runs relate
apply,baseline, and--with-migrationsreadGET /capabilitiesfirst. Withoutdatabases.migrationsavailable they refuse before sending anything, say why, and namedatabases migration up --all -d <db>. They never fall back to running files client-side.statusandrunsdon't need the capability, because the reads stay ungated.upstays the explicit untracked escape hatch. It runs every file through the database proxy, one statement per file, and writes no history. Nothing calls it implicitly.statusshows them allpending, and when nothing is recorded yet it suggestsbaseline --all. A run that fails withmigration_failedbefore anything applied suggests the same.deploy --helpand the docs say to baseline once before the first tracked deploy.volcano/migrationsis read the same way Git deploy reads it: top-level files for one database, orvolcano/migrations/<database>/per database, never both. Names starting with_or.are skipped. Symlinks, non-regular.sqlfiles, and deeper nesting are refused. Nested files are sent as<database>/<file>.sql, which the API accepts and strips. With the per-database layout,--with-migrationswithout-dmigrates every database that has a directory.upnow reads the same layout, soup -d apptakesvolcano/migrations/app/in a per-database project, and a mixed directory is refused there too..sql, soapply,baseline, and--with-migrationsrefuse001_init.SQLbefore sending anything and name the rename.upstill runs such files.statuscompares local files with the history using the server's normalization: a leading BOM is stripped, CRLF becomes LF, and the result issha256:<hex>.Off-state and errors
apply/deploysaytracked migration runs aren't available on this Volcano API (capability "databases.migrations"), nameup --all, and exit non-zero.deployaddsNothing was deployed.. Afeature_unavailableanswer from the route itself gets the same wording.migration_run_in_progress(409) names the run andruns get <id>. Each command sends a freshIdempotency-Key, so a retried request returns the run it started (a200replay is treated like the202), while running the command again starts a new run.runs get <id>.database_busy,history_too_large,history_missing, storage refusals: the API's message is printed and the exit code follows the status.history_missingandchecksum_mismatchadd what to do next.volcano resetnow suggestsvolcano databases migration apply -d app, because an untracked redeploy followed by--with-migrationswould apply every file again.mainalready vendors the Database Migrations paths and the/capabilitiesblock (since #456), so this PR now takesmain'sopenapi/openapi.yamlunchanged;make openapi-generateproduces no diff.Overlap with #478
#478 (stacked on #476 and #477) moves
upanddeploy --with-migrationsonto migration runs underpg-delegated grants. To let it rebase onto this PR with little conflict:internal/api/database_migration_runs.go, feat(databases): apply migrations through migration runs under a grant #478's file name, with feat(databases): apply migrations through migration runs under a grant #478's names and bodies forCreateDatabaseMigrationRun,BaselineDatabaseMigrations,GetDatabaseMigrationRun,migrationConflictError, andMigrationRunInProgressCode. Run reads and a refused run start keep only an error body's own fields, as feat(databases): apply migrations through migration runs under a grant #478 does, since a run's file errors can quote data.migration runs list|getcommands are inruns_list.goandruns_get.go, so feat(databases): apply migrations through migration runs under a grant #478'sruns.go(the grant run path) adds cleanly. None of its identifiers collide with this package.baseline.go(both add it, with the same flags), its branch inrunUp, and its extraFeatureUnavailableCodeconstant.Tests
app.TestAPIE2ECloudDatabaseMigrations(new, standard cloud-e2e CLI suite): creates a database, then runsstatus(all pending, plus the baseline hint),apply(rehearsed,backup_skippedon HOBBY),applyagain (nothing applied),baseline -fandbaseline --all(nothing left to record),deploy --with-migrations(only the new file runs), a broken file that fails in rehearsal without touching the database, thenruns listandruns get.TestAPIE2ECloudDeployand the starter deploy now expect tracked-run output.TestLocalModeE2EDatabaseMigrations(new, added tomake localmode-e2e): the same round trip on the localappdatabase, withrehearsal_skippedandbackup_skipped, and the broken file failing inapply. The local smoke's deploy checks expect tracked runs, and its reset check expects the new hint.TestCLIE2ESmokeDatabaseMigrations(refusals with the gate off, and reads on a missing database) and changes the--with-migrationscheck inTestCLIE2ESmokeDeployto the refusal.Verification
At head after merging
main(v0.78.0, ffe321c):docs/databases.md(keptmain's regions and resize rows and sections),internal/cmd/deploy/helpers_test.go(kept both the migration-run fakes andmain's function-read fakes), andopenapi/openapi.yamlplusinternal/apiclient/client.gen.go(tookmain's;make openapi-generated-checkpasses).go build ./...,go vet ./tests/... ./cmd/...,gofmt,go mod tidy(no diff),make release-workflow-check.go tool golangci-lint run --new-from-rev=origin/main --whole-fileson the changed packages: 0 issues.go test ./internal/cmd/databases/... ./internal/cmd/deploy/ ./internal/database/ ./internal/cmd/root/ ./internal/api/, and-run Resetin./internal/localmode/ ./internal/cmd/local/: pass.checkjob,localmode-e2e, and the hostingcli-testsrun in Kong/volcano-hosting#2374 (same branch name, so it builds this branch).Earlier, on 13784cc:
cloud-e2e-localrunner at hostingmain2791f39517:go test ./tests/e2e/api -run '^TestAPIE2E(SmokeMigrations|CloudDatabaseMigrations)$'. Both passed (TestAPIE2ECloudDatabaseMigrationstook 70s).main2791f39517:TestLocalModeE2EDatabaseMigrations(52s) andTestLocalModeE2ESmoke(449s) passed.main; the merge touches no migration code path, and the alignment commit only renames files and narrows error bodies.Decisions to confirm
--with-migrationshas no client-side fallback. Following hosting's flag rules, it refuses wherever the capability is off and namesup --all. So this PR merges (and releases) only once the production flag is on.baselinefirst.status,deploy --help, the docs, and themigration_failedhint all point tobaseline --all. The CLI never baselines on its own, because recording a file that didn't run means it never runs. Local mode has no rehearsal, so there a re-run file that fails fails inapply. Each file runs in its own transaction, so the files before it stay applied and recorded.upand localmigrations deploystay. They're the escape hatch the refusals name, so they're kept and not deprecated. Their help now says they're untracked and points toapply.-dnarrows it to one database.baselineprompts unless--yesis passed. A recorded file never runs, so the default asks first.upand localmigrations deploynow read the same files as tracked runs and Git deploy. Before, they read only top-level.sqlfiles, followed symlinks, and ran_- and.-prefixed files. Now they skip_/.names, readvolcano/migrations/<database>/in a per-database project, and refuse symlinks, deeper nesting, and a mixed layout. I chose this sobaseline --allafterup --allrecords exactly the files that ran. Alternative: keepup's old reading and accept that the two can disagree.cli-testsagainst CLImain. It drops hosting's mid-deploy failure check (--with-migrations -d missing_databasefailing at step 2), because that lane can't reach a migration step with the gate off. CLI unit tests still cover a step failing mid-run (TestDeployStopsAtAFailedMigrationRun).🤖 Generated with Claude Code