Skip to content

feat(git): turn applying database migrations on push on or off - #490

Merged
swkeever merged 3 commits into
mainfrom
skeever/git-migrations-setting
Nov 13, 2026
Merged

swkeever merged 3 commits into
mainfrom
skeever/git-migrations-setting

Conversation

@swkeever

@swkeever swkeever commented Nov 13, 2026 •

Copy link
Copy Markdown
Collaborator

Tracking

  • Related: feat(databases): apply migrations in tracked runs #365 (tracked migration commands, released in v0.79.0)
  • Related: Kong/volcano-hosting#2287 (Git deploy applies migrations when apply_migrations is on)
  • Related: Kong/volcano-web#1034 (the same switch in the dashboard)

Why

  • Database migrations on push went live in production on 2026-11-13, but the CLI could not turn them on. volcano git auto-deploy, functions and frontend cover every other Git deploy setting; apply_migrations could only be set from the dashboard or a raw PUT.
  • CI jobs and agents set these settings with a project access token, so they need a non-interactive way to do it.

Summary

  • add volcano git migrations on|off [--yes], which sets only apply_migrations and sends the other settings back as read (the API keeps apply_migrations when it is omitted, so the other setting commands still never touch it)
  • turning it on asks for confirmation first, like the dashboard: anyone who can push to the production branch can then run any SQL as the database owner, and files already applied another way need volcano cloud databases migration baseline first (the cloud command: top-level volcano databases targets the local server). --yes skips the prompt; with no terminal and no --yes it refuses, as git connect and git disconnect do. Turning it off asks nothing
  • runDeploySetting gains an optional confirm step, 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 saved
  • volcano git status lists Migrations: on|off, and the deploy summary reads "A push to main applies its migrations, then deploys: …" while it is on
  • after the prompt, SetApplyMigrations re-reads the binding (confirmUnchanged, as git disconnect does) 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 another
  • with auto-deploy on and neither functions nor a frontend selected, a push still applies its migrations (the platform runs migration-only pushes), so the summary says "a push to main applies its migrations and deploys no code" instead of "a push deploys nothing"
  • where migrations are unavailable the API answers 400 feature_unavailable; the command fails with that and saves nothing
  • docs: docs/git.md gets a section, a table row, and the local-mode list

Verification

  • regression tests for a binding that changes during the prompt and for the migration-only summary
  • 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-files on the changed packages: 0 issues
  • go vet ./tests/e2e/localmode/; the local-mode E2E list now includes git migrations on --yes (refused there: no GitHub App), run in CI
  • volcano git migrations --help checked by hand
  • deferred to CI: the full suite and localmode-e2e

🤖 Generated with Claude Code

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.
@swkeever
swkeever requested a review from a team as a code owner November 13, 2026 16:25
Copilot AI balanced review requested due to automatic review settings November 13, 2026 16:25
@github-actions

github-actions Bot commented Nov 13, 2026 •

Copy link
Copy Markdown

AI review: no P0-P2 findings

Reviewed 7456139. Run log

Adds volcano git migrations on|off [--yes], which sets only the apply_migrations Git deploy setting. Turning it on asks for confirmation, then re-reads the repository binding before saving and refuses to save if it changed. git status and the deploy summary now report the setting, and docs/git.md is updated. The previous round's findings are fixed at this head: the baseline guidance names volcano cloud databases migration baseline, the binding is re-checked after the prompt, and migration-only pushes are summarised correctly. The other setting commands still leave apply_migrations out of their writes. I found no new defects; the remaining risk is the product decision to let tokens and --yes turn on SQL-on-push.

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.

  • Whether the hosting API keeps the saved apply_migrations when the other setting commands leave it out of the PUT; the CLI relies on this so those commands never turn it off.
  • Whether production answers 400 feature_unavailable and saves nothing when migrations are turned on where they are unavailable.
  • Whether a push with auto-deploy on, migrations on, and neither functions nor a frontend selected really applies migrations, as the new summary and docs say.
  • Whether the platform keeps or resets apply_migrations when the project is reconnected to another repository or its production branch changes.
  • Whether the local-mode E2E case git migrations on --yes passes in CI with the expected 'integration is not configured' message.

Human review: needed

  • Should a project token with --yes turn on SQL-on-push from CI with no prompt (internal/cmd/git/migrations.go)? Anyone who can push to the branch can then run SQL as the DB owner.
  • With migrations on, git production-branch and reconnecting change which branch runs that SQL with no prompt. Is reporting it afterwards enough?
  • Should project access tokens and --yes enable production database migrations on Git pushes through internal/cmd/git/migrations.go?

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: shawnwu-kong (most recent commits to internal/cmd/git, gitconnect and the git command tests). Any collaborator with write access other than the author can approve.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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|off with confirmation when enabling and --yes for 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

Comment on lines +43 to +46
confirmed, err := setting.confirm(connection)
if err != nil || !confirmed {
return err
}
Comment thread internal/output/git.go
switch {
case !settings.ApplyMigrations:
Note(w, "Nothing was undone. Applied migrations stay; pushes no longer apply new ones.")
case settings.AutoDeployEnabled:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚪ 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)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚪ 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)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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

Comment thread internal/output/git.go
strings.Join(targets, ", "),
)
lead := fmt.Sprintf("A push to %s deploys: ", connection.ProductionBranch)
if settings.ApplyMigrations {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.
Comment thread internal/cmd/git/migrations.go Outdated

func newMigrations(deps cliruntime.Deps) *cobra.Command {
var yes bool
baseline := cliruntime.CommandPath(deps, "databases migration baseline")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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'.
@swkeever
swkeever merged commit 65b50e7 into main Nov 13, 2026
18 of 19 checks passed
@swkeever
swkeever deleted the skeever/git-migrations-setting branch November 13, 2026 17:10
@kong-volcano-app kong-volcano-app Bot mentioned this pull request Nov 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants