[STORY-3149] Add DBNG plan upgrade command #1178
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a new command to upgrade database next generation (DBNG) plans, including support for waiting until the upgrade completes.
Changes:
- Added
database-upgradecommand with plan change logic and optional wait functionality - Integrated the new command into the CLI command registry
- Updated cross-references to include the new command in related database commands
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| dbng/upgrade.go | Implements the core upgrade logic with plan validation, addon upgrade API call, and wait functionality for plan changes |
| cmd/databases_ng.go | Adds the database-upgrade command definition with argument parsing and integrates it into SeeAlso references |
| cmd/commands.go | Registers the new database-upgrade command in the global command list |
| CHANGELOG.md | Documents the new database plan upgrade command feature |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
matthieu526-scalingo
left a comment
matthieu526-scalingo
left a comment
There was a problem hiding this comment.
Good job! Two small comments.
curzolapierre
left a comment
curzolapierre
left a comment
There was a problem hiding this comment.
Some questions otherwise LGTM
| io.Error("Invalid number of arguments") | ||
| return cli.ShowCommandHelp(ctx, c, "database-upgrade") |
There was a problem hiding this comment.
Nitpick: generally with no argument given we just print the help WDYT?
Otherwise, could you share an example of the output either here in comment or in PR's body?
There was a problem hiding this comment.
Agree, it should behave like database-create and just print the help when no args are provided. I’ll align with that.
For the output, here’s a sample from staging:
$ scalingo database-upgrade 69652796697f13aac6687e51 postgresql-ng-enterprise-8192
Your postgresql-ng database 69652796697f13aac6687e51 ('ng-plan-change-xxx') is being upgraded…
With --wait, it shows the spinner then:
Your database plan change is complete.”
cmd/databases_ng.go
Outdated
| if c.IsSet("database") || os.Getenv("SCALINGO_DATABASE") != "" { | ||
| databaseID, _ = detect.GetCurrentDatabase(ctx, c) | ||
| plan = args[0] | ||
| } else { | ||
| io.Error("Missing required argument: plan") | ||
| return cli.ShowCommandHelp(ctx, c, "database-upgrade") | ||
| } |
There was a problem hiding this comment.
Question: is this still working when we pass --database as first argument?
IIRC is it what is usually expected for application flag:
scalingo --database MY-DB-ID database-upgrade postgresql-ng-enterprise-8192There was a problem hiding this comment.
Yes that’s the expected usage:
scalingo --database MY-DB-ID database-upgrade postgresql-ng-enterprise-8192
It work since the flag is global.
| return err | ||
| } | ||
|
|
||
| if db.Database.Status == scalingo.DatabaseStatusRunning { |
There was a problem hiding this comment.
Suggestion I'd also check the plan. IIRC API still accept the request without error even if the requested plan is the same as the current one.
So in that specific case it'd hang forever.
|
|
||
| var dbAddon *scalingo.Addon | ||
| if db.Database.ResourceID != "" { | ||
| if len(addons) == 1 { |
There was a problem hiding this comment.
nitpick: I am not sure this optimization is needed. The loop is probably not expensive.
Summary
Testing