Repository navigation
feat(git): turn applying database migrations on push on or off #490
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,8 +18,15 @@ type deploySetting struct { | |
| // subcommand is the command's own name, whose next step the output leaves | ||
| // out. | ||
| subcommand string | ||
| change func(context.Context, gitconnect.Service) (*apiclient.ProjectGitDeploySettings, error) | ||
| render func(io.Writer, output.GitBinding) | ||
| // change saves the setting. It is given the binding the command read and | ||
| // showed, for a change that must not land on any other. | ||
| change func( | ||
| context.Context, gitconnect.Service, *apiclient.ProjectGitConnection, | ||
| ) (*apiclient.ProjectGitDeploySettings, error) | ||
| render func(io.Writer, output.GitBinding) | ||
| // confirm, when set, is asked once the repository is known and before | ||
| // anything is saved. Declining saves nothing and is not an error. | ||
| confirm func(*apiclient.ProjectGitConnection) (bool, error) | ||
| } | ||
|
|
||
| // runDeploySetting changes one deploy setting on a project with a repository | ||
|
|
@@ -36,7 +43,14 @@ func runDeploySetting(ctx context.Context, deps cliruntime.Deps, out io.Writer, | |
| return err | ||
| } | ||
|
|
||
| settings, err := setting.change(ctx, service) | ||
| if setting.confirm != nil { | ||
| confirmed, err := setting.confirm(connection) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. Found by gpt-6.1-sol |
||
| if err != nil || !confirmed { | ||
| return err | ||
| } | ||
|
Comment on lines
+47
to
+50
|
||
| } | ||
|
|
||
| settings, err := setting.change(ctx, service, connection) | ||
| if err != nil { | ||
| return guide(deps, err) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| package git | ||
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
|
|
||
| "github.com/spf13/cobra" | ||
|
|
||
| "github.com/Kong/volcano-cli/internal/apiclient" | ||
| "github.com/Kong/volcano-cli/internal/gitconnect" | ||
| "github.com/Kong/volcano-cli/internal/output" | ||
| cliruntime "github.com/Kong/volcano-cli/internal/runtime" | ||
| ) | ||
|
|
||
| func newMigrations(deps cliruntime.Deps) *cobra.Command { | ||
| var yes bool | ||
| // A push applies migrations to the cloud project's databases, so the | ||
| // baseline is the cloud one: the top-level databases commands run against | ||
| // the local server. | ||
| baseline := cliruntime.CommandPath(deps, "cloud databases migration baseline") | ||
| cmd := &cobra.Command{ | ||
| Use: "migrations on|off", | ||
| Short: "Choose whether a push to the connected repository applies its database migrations", | ||
| Long: fmt.Sprintf(`Choose whether a push to the connected repository's production branch applies | ||
| the commit's pending migrations, the SQL files under volcano/migrations, before | ||
| it deploys anything. Each database gets a tracked migration run; when one fails, | ||
| nothing from the push deploys and the live version keeps serving. | ||
|
|
||
| Anyone who can push to the production branch can then run any SQL as the | ||
| database owner, so protect that branch first. Turning it on asks for | ||
| confirmation; use --yes to skip the prompt. | ||
|
|
||
| Volcano applies only the files its migration history does not list yet. If | ||
| some files were already applied another way, record them first: | ||
| %s | ||
|
|
||
| Turning it off undoes nothing: applied migrations stay, and pushes stop | ||
| applying new ones. The other deploy settings stay as they were; see | ||
| '%s' for all of them. | ||
|
|
||
| This works with a project access token, so CI or an agent can set it. The | ||
| project needs a connected repository first: see '%s'.`, | ||
| baseline, | ||
| cliruntime.CommandPath(deps, "git status"), | ||
| cliruntime.CommandPath(deps, "git connect")), | ||
| Example: " volcano git migrations on\n volcano git migrations on --yes\n volcano git migrations off", | ||
| Args: cobra.ArbitraryArgs, | ||
| ValidArgs: []string{"on", "off"}, | ||
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| on, err := onOffArgument(args, "migrations") | ||
| if err != nil { | ||
| return err | ||
| } | ||
| setting := deploySetting{ | ||
| subcommand: "migrations", | ||
| change: func( | ||
| ctx context.Context, service gitconnect.Service, shown *apiclient.ProjectGitConnection, | ||
| ) (*apiclient.ProjectGitDeploySettings, error) { | ||
| return service.SetApplyMigrations(ctx, on, shown) | ||
| }, | ||
| render: output.GitMigrationsSet, | ||
| } | ||
| if on { | ||
| setting.confirm = func(connection *apiclient.ProjectGitConnection) (bool, error) { | ||
| return ask(cmd.InOrStdin(), cmd.OutOrStdout(), yes, | ||
| fmt.Sprintf("Each push to %[1]s of %[2]s will apply the SQL files under volcano/migrations "+ | ||
| "to the project's databases as their owner, before it deploys. Anyone who can push "+ | ||
| "to %[1]s can then run any SQL on them, so protect that branch first.\n"+ | ||
| "Files already applied another way need a baseline first: %[3]s", | ||
| connection.ProductionBranch, connection.RepoFullName, baseline), | ||
| "Apply migrations on push?") | ||
| } | ||
| } | ||
| return runDeploySetting(cmd.Context(), deps, cmd.OutOrStdout(), setting) | ||
| }, | ||
| } | ||
| cmd.Flags().BoolVarP(&yes, "yes", "y", false, "Skip the confirmation prompt when turning migrations on") | ||
| return cmd | ||
| } |
There was a problem hiding this comment.
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.changesaves 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 disconnectpasses the binding it showed toservice.Disconnect, which re-checks it withconfirmUnchangedfor this reason. Re-checking the binding after the prompt (for example withconfirmUnchanged, or comparing it withsameBinding) would close the window. The race is narrow, but this is the confirmation for a security-sensitive setting.Found by claude-opus-5-5