Repository navigation
feat(git): turn applying database migrations on push on or off - #490
Conversation
Adds 'volcano git migrations on|off', which sets the project's apply_migrations Git deploy setting. Turning it on asks for confirmation (--yes skips it), since anyone who can push to the production branch can then run any SQL as the database owner. 'volcano git status' and the deploy summary now report it.
AI review: no P0-P2 findingsAdds 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 7456139, so the code needs no second review for bugs. Decide the points above: approve to accept them, or request changes. Suggested reviewer: |
There was a problem hiding this comment.
🟡 Changes recommended
A repository or production-branch change during confirmation can enable database-owner SQL execution for an unconfirmed binding; the write needs post-confirmation revalidation and a regression test.
1 open issue
What changed in this PR
Adds CLI control over database migrations during Git push deployments, complementing the existing auto-deploy, functions, and frontend settings.
Changes:
- Adds
volcano git migrations on|offwith confirmation when enabling and--yesfor automation. - Shows migration settings in Git status and deployment summaries.
- Documents the command and adds coverage for confirmation, settings preservation, API refusal, and local-mode rejection.
| File | Description |
|---|---|
tests/e2e/localmode/git_test.go |
Adds local-mode rejection coverage for migrations. |
internal/output/git.go |
Displays migration state and migration-aware deploy summaries. |
internal/gitconnect/gitconnect.go |
Adds the migration-setting service operation. |
internal/cmd/git/status_test.go |
Tests migration state and ordering in status output. |
internal/cmd/git/migrations.go |
Defines the command, confirmation, and automation flag. |
internal/cmd/git/migrations_test.go |
Covers command behavior and failure paths. |
internal/cmd/git/helpers_test.go |
Models migration settings in the test API. |
internal/cmd/git/git.go |
Registers the migrations subcommand. |
internal/cmd/git/deploy_settings.go |
Adds an optional confirmation step before saving. |
docs/git.md |
Documents migration controls, risks, and local-mode behavior. |
Review effort: Balanced
| confirmed, err := setting.confirm(connection) | ||
| if err != nil || !confirmed { | ||
| return err | ||
| } |
| switch { | ||
| case !settings.ApplyMigrations: | ||
| Note(w, "Nothing was undone. Applied migrations stay; pushes no longer apply new ones.") | ||
| case settings.AutoDeployEnabled: |
There was a problem hiding this comment.
⚪ P3: Turning migrations on promises the next push applies them while the summary below says a push deploys nothing
With auto-deploy on but functions off and no frontend selected, GitMigrationsSet prints "Nothing was applied. The next push to main applies its pending migrations before it deploys." Then gitDeploySettings immediately warns "Auto-deploy is on, but neither functions nor a frontend is selected, so a push deploys nothing." One of the two lines is misleading, whichever way the platform handles a push with nothing to deploy. GitAutoDeploySet avoids exactly this contradiction by checking len(deployTargets(settings)) == 0 first. Add the same case here, or make the no-target warning say whether migrations still run, so the two lines agree.
Found by claude-opus-5-5
| } | ||
|
|
||
| if setting.confirm != nil { | ||
| confirmed, err := setting.confirm(connection) |
There was a problem hiding this comment.
⚪ P3: The migrations confirmation is not re-checked against the binding after the prompt
The prompt names the production branch and repository from the binding read before asking, then setting.change saves without checking that binding again. If the connection is replaced or its production branch changes while the prompt is open, the user's "yes" turns on SQL-on-push for a branch or repository they never confirmed. The rendered output then shows the stale binding. git disconnect passes the binding it showed to service.Disconnect, which re-checks it with confirmUnchanged for this reason. Re-checking the binding after the prompt (for example with confirmUnchanged, or comparing it with sameBinding) would close the window. The race is narrow, but this is the confirmation for a security-sensitive setting.
Found by claude-opus-5-5
| } | ||
|
|
||
| if setting.confirm != nil { | ||
| confirmed, err := setting.confirm(connection) |
There was a problem hiding this comment.
🟡 P2: Revalidate the confirmed Git binding before enabling migrations
The enabling path needs to verify that the current binding still matches the connection shown in the confirmation. SetApplyMigrations rereads only deploy settings, so a concurrent repository or production-branch update leaves approval tied to stale connection details. Pass the expected connection into the service and reuse confirmUnchanged before saving, as Connect and Disconnect already do.
Found by gpt-6.1-sol
| strings.Join(targets, ", "), | ||
| ) | ||
| lead := fmt.Sprintf("A push to %s deploys: ", connection.ProductionBranch) | ||
| if settings.ApplyMigrations { |
There was a problem hiding this comment.
🟡 P2: Include migration-only pushes in the deployment summary
With auto-deploy and migrations enabled, functions disabled, and no frontend selected, the empty-target branch skips the new migration summary. Consequently, git functions off reports that a push deploys nothing without mentioning that it still applies migrations; the API's GitDeploymentRunStatus contract explicitly supports migration-only runs. Handle this case separately and report that pushes still apply migrations without deploying code.
Found by gpt-6.1-sol
'volcano git migrations on' now refuses to save when the repository connection changed while the confirmation was open, so a yes for one repository and branch never enables SQL-on-push for another. The deploy summary also says that a push with neither functions nor a frontend selected still applies its migrations, instead of saying it deploys nothing.
|
|
||
| func newMigrations(deps cliruntime.Deps) *cobra.Command { | ||
| var yes bool | ||
| baseline := cliruntime.CommandPath(deps, "databases migration baseline") |
There was a problem hiding this comment.
🟡 P2: Point the baseline prerequisite at the cloud command
This produces volcano databases migration baseline, but internal/cmd/root/root.go registers top-level database commands through local.NewResourceCommands, whose configuration targets the local server. Following the prerequisite with the required database/file flags therefore fails without a local stack or records local history, leaving the cloud database unprepared. Use volcano cloud databases migration baseline in the prompt and help, and correct the matching guidance in docs/git.md.
Found by gpt-6.1-sol
The top-level 'volcano databases' commands run against the local server, so following 'volcano databases migration baseline' would record history locally and leave the cloud databases unprepared. The prompt, help and docs now name 'volcano cloud databases migration baseline'.

Tracking
apply_migrationsis on)Why
volcano git auto-deploy,functionsandfrontendcover every other Git deploy setting;apply_migrationscould only be set from the dashboard or a rawPUT.Summary
volcano git migrations on|off [--yes], which sets onlyapply_migrationsand sends the other settings back as read (the API keepsapply_migrationswhen it is omitted, so the other setting commands still never touch it)volcano cloud databases migration baselinefirst (the cloud command: top-levelvolcano databasestargets the local server).--yesskips the prompt; with no terminal and no--yesit refuses, asgit connectandgit disconnectdo. Turning it off asks nothingrunDeploySettinggains an optionalconfirmstep, asked after the repository is known (so the prompt names the branch and repository, and an unconnected project is refused before anyone confirms) and before anything is savedvolcano git statuslistsMigrations: on|off, and the deploy summary reads "A push to main applies its migrations, then deploys: …" while it is onSetApplyMigrationsre-reads the binding (confirmUnchanged, asgit disconnectdoes) and refuses to save if the repository or production branch changed while the prompt was open, so a yes for one repository never turns it on for another400 feature_unavailable; the command fails with that and saves nothingdocs/git.mdgets a section, a table row, and the local-mode listVerification
go test ./internal/cmd/git/ ./internal/output/ ./internal/gitconnect/ ./internal/cmd/root/ ./internal/docs/...go tool golangci-lint run --new-from-rev=origin/main --whole-fileson the changed packages: 0 issuesgo vet ./tests/e2e/localmode/; the local-mode E2E list now includesgit migrations on --yes(refused there: no GitHub App), run in CIvolcano git migrations --helpchecked by handlocalmode-e2e🤖 Generated with Claude Code