-
Notifications
You must be signed in to change notification settings - Fork 304
feat: add deploy command to push all changes to linked project #4368
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
base: develop
Are you sure you want to change the base?
Conversation
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.
Just a few high level comments before I dive into the implementation detail.
cmd/deploy.go
Outdated
| // What to deploy - use direct Bool() since we check via cmd.Flags().Changed() | ||
| cmdFlags.Bool("include-db", true, "Include database migrations (default: true)") | ||
| cmdFlags.Bool("include-functions", true, "Include edge functions (default: true)") | ||
| cmdFlags.Bool("include-config", true, "Include config.toml settings (default: true)") | ||
|
|
||
| // DB push options (from db push command) | ||
| cmdFlags.BoolVar(&deployDryRun, "dry-run", false, "Print operations that would be performed without executing them") | ||
| cmdFlags.BoolVar(&deployIncludeAll, "include-all", false, "Include all migrations not found on remote history table") | ||
| cmdFlags.BoolVar(&deployIncludeRoles, "include-roles", false, "Include custom roles from "+utils.CustomRolesPath) | ||
| cmdFlags.BoolVar(&deployIncludeSeed, "include-seed", false, "Include seed data from your config") |
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.
I'm worried that having all these flags would be too overwhelming for the user. Can we show a suggestion to use db specific commands instead? Generally we want to build commands that do one thing and one thing well.
Also from maintenance perspective, we want to very selective about which flag to add. We want to make supabase deploy easy for new users to get started but defer them to specific commands as soon as they need to customise the default behaviour.
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.
I've simplified things so you can filter down what you want to deploy with --only=db,config and removed some of those db-specifc flags.
| } | ||
| } | ||
| if !shouldDeploy { | ||
| fmt.Fprintln(os.Stderr, "Would deploy:", slug) |
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.
| fmt.Fprintln(os.Stderr, "Would deploy:", slug) |
Do you mean to log "would not deploy"? We can probably move this log to the keep filter https://github.com/supabase/cli/pull/4368/files#diff-331af3005b73dace93ae60f98283eb774537c9de0399550d705e3dc5c9330ec5R81
| return s.upsertFunctions(ctx, functionConfig, filter...) | ||
| } | ||
|
|
||
| func (s *EdgeRuntimeAPI) upsertFunctions(ctx context.Context, functionConfig config.FunctionConfig, filter ...func(string) bool) error { |
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.
ok to undo changes in this file?
| if err := api.Deploy(ctx, functionConfig, afero.NewIOFS(fsys), filter...); errors.Is(err, function.ErrNoDeploy) { | ||
| fmt.Fprintln(os.Stderr, err) | ||
| return nil | ||
| return err |
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.
We were returning nil so that exit code is 0 when there are no changed functions to deploy. What's the reason for changing this?
| func init() { | ||
| cmdFlags := deployCmd.Flags() | ||
|
|
||
| deployCmd.Flags().StringSliceVar(&only, "only", []string{}, "Comma-separated list of components to deploy (e.g., db,storage,functions).") |
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.
Since we intend to use this on CI/CD, I think it's neater to set this via config.toml. For eg. to disable deploying storage
[storage]
enabled = falseFor adhoc deployments from local, I'd still prefer directing users to db specific commands.
What kind of change does this PR introduce?
Feature
What is the current behavior?
Have to seperately deploy changes to migrations, config, and functions.
What is the new behavior?
A single command that will deploy all changes to a remote proejct.
Additional context
--remoteflag tostatusto get the health of the remote project.--dry-runsupport to all deploy commands (added to functions and config)