feat(cli): onboard iaas volume backup api #773
Conversation
| args: args{}, | ||
| wantErr: true, | ||
| }, | ||
| { |
There was a problem hiding this comment.
consider adding a test case which tests an empty iaas backup in the slice
args: args{
backups: []iaas.Backup{{}},
},There was a problem hiding this comment.
Well, now you don't have a testcase for an empty slice anymore...
|
@Benjosh95 please check the failing CI pipeline |
CI pipeline fails due to missing/undefined waiters that will be merged with this PR: stackitcloud/stackit-sdk-go#2669 |
| func buildRequest(ctx context.Context, model *inputModel, apiClient *iaas.APIClient) iaas.ApiCreateBackupRequest { | ||
| req := apiClient.CreateBackup(ctx, model.ProjectId) | ||
|
|
||
| // Convert map[string]string to map[string]interface{} |
There was a problem hiding this comment.
This code is duplicated already 20 times across the codebase: https://github.com/search?q=repo%3Astackitcloud%2Fstackit-cli%20%2F%2F%20Convert%20map%5Bstring%5Dstring%20to%20map%5Bstring%5Dinterface%7B%7D&type=code
Please create a util function for it.
You can also create a Jira ticket in the backlog instead if you prefer. But we shouldn't continue to work like that.
| wantErr bool | ||
| }{ | ||
| { | ||
| name: "empty backup", |
There was a problem hiding this comment.
| name: "empty backup", | |
| name: "backup is nil", |
There was a problem hiding this comment.
Empty backup is another testcase which you should add IMO
|
|
||
| default: | ||
| if async { | ||
| p.Outputf("Triggered backup of %s in %s. Backup ID: %s\n", sourceLabel, projectLabel, *resp.Id) |
There was a problem hiding this comment.
will panic if resp.Id is nil, see my comment below about the missing testcase
| table := tables.NewTable() | ||
| table.SetHeader("ID", "NAME", "SIZE", "STATUS", "SNAPSHOT ID", "VOLUME ID", "AVAILABILITY ZONE", "LABELS", "CREATED AT", "UPDATED AT") | ||
|
|
||
| for i := range backups { |
There was a problem hiding this comment.
| for i := range backups { | |
| for _, backup := range backups { |
or did I miss something? 😄
| if backup != nil && backup.VolumeId != nil { | ||
| volume, err := apiClient.GetVolume(ctx, model.ProjectId, *backup.VolumeId).Execute() | ||
| if err != nil { | ||
| params.Printer.Debug(print.ErrorLevel, "get volume details: %v", err) |
There was a problem hiding this comment.
the line sourceLabel = *backup.VolumeId is the same as in the else branch.
Just put this line before the whole if/else if/ else block.
Then you can save yourself the duplication and the else block :)
| s.Stop() | ||
| } | ||
|
|
||
| projectLabel := model.ProjectId |
There was a problem hiding this comment.
I don't see the point in creating a new variable here
| examples.NewExample( | ||
| `Create a backup from a volume`, | ||
| "$ stackit volume backup create --source-id xxx --source-type volume --project-id xxx"), | ||
| examples.NewExample( | ||
| `Create a backup from a snapshot with a name`, | ||
| "$ stackit volume backup create --source-id xxx --source-type snapshot --name my-backup --project-id xxx"), | ||
| examples.NewExample( | ||
| `Create a backup with labels`, | ||
| "$ stackit volume backup create --source-id xxx --source-type volume --labels key1=value1,key2=value2 --project-id xxx"), | ||
| ), |
There was a problem hiding this comment.
The CLI has also an option to configure the project-id global for all commands. The project-id can be overwritten in the commands with the flag, but for the examples I wouldn't add the flag to stay consistent with the other commands and their examples
| examples.NewExample( | |
| `Create a backup from a volume`, | |
| "$ stackit volume backup create --source-id xxx --source-type volume --project-id xxx"), | |
| examples.NewExample( | |
| `Create a backup from a snapshot with a name`, | |
| "$ stackit volume backup create --source-id xxx --source-type snapshot --name my-backup --project-id xxx"), | |
| examples.NewExample( | |
| `Create a backup with labels`, | |
| "$ stackit volume backup create --source-id xxx --source-type volume --labels key1=value1,key2=value2 --project-id xxx"), | |
| ), | |
| examples.NewExample( | |
| `Create a backup from a volume`, | |
| "$ stackit volume backup create --source-id xxx --source-type volume"), | |
| examples.NewExample( | |
| `Create a backup from a snapshot with a name`, | |
| "$ stackit volume backup create --source-id xxx --source-type snapshot --name my-backup"), | |
| examples.NewExample( | |
| `Create a backup with labels`, | |
| "$ stackit volume backup create --source-id xxx --source-type volume --labels key1=value1,key2=value2"), | |
| ), |
|
|
||
| func configureFlags(cmd *cobra.Command) { | ||
| cmd.Flags().String(sourceIdFlag, "", "ID of the source from which a backup should be created") | ||
| cmd.Flags().String(sourceTypeFlag, "", "Source type of the backup (volume or snapshot)") |
There was a problem hiding this comment.
This can be configured with flags.EnumFlag(). See also:
This as the advantage, that you can define the allowed values in a slice and reuse it also for the validation and description. And in the future it can easily be extended by just adding a value to the slice
| `Delete a backup`, | ||
| "$ stackit volume backup delete xxx-xxx-xxx"), |
There was a problem hiding this comment.
Just a nitpick to stay consistent with the existing commands
| `Delete a backup`, | |
| "$ stackit volume backup delete xxx-xxx-xxx"), | |
| `Delete a backup with ID "xxx"`, | |
| "$ stackit volume backup delete xxx"), |
| `Delete a backup and wait for deletion to be completed`, | ||
| "$ stackit volume backup delete xxx-xxx-xxx --async=false"), |
There was a problem hiding this comment.
The async flag can be globally configured like the projectId and is on default already false. So compared to the previous example there is no difference.
I think it's fine to have just one example for the delete command. For other delete commands we also have only one example
| `Get details of a backup`, | ||
| "$ stackit volume backup describe xxx-xxx-xxx"), |
There was a problem hiding this comment.
| `Get details of a backup`, | |
| "$ stackit volume backup describe xxx-xxx-xxx"), | |
| `Get details of a backup with ID "xxx"`, | |
| "$ stackit volume backup describe xxx"), |
| `Restore a backup`, | ||
| "$ stackit volume backup restore xxx-xxx-xxx"), |
There was a problem hiding this comment.
| `Restore a backup`, | |
| "$ stackit volume backup restore xxx-xxx-xxx"), | |
| `Restore a backup with ID "xxx"`, | |
| "$ stackit volume backup restore xxx"), |
| backup, err := apiClient.GetBackup(ctx, model.ProjectId, model.BackupId).Execute() | ||
| if err != nil { | ||
| params.Printer.Debug(print.ErrorLevel, "get backup details: %v", err) | ||
| } |
There was a problem hiding this comment.
You can move this into a utils function. You can reuse it then also for the update functions, to prompt if the wants to update the backup with the name "yyy"
| s.Stop() | ||
| } | ||
|
|
||
| projectLabel := model.ProjectId |
There was a problem hiding this comment.
This line isn't required. You can just use model.ProjectId in the params.Printer.Info() calls
| `Update a backup name`, | ||
| "$ stackit volume backup update xxx-xxx-xxx --name new-name"), |
There was a problem hiding this comment.
| `Update a backup name`, | |
| "$ stackit volume backup update xxx-xxx-xxx --name new-name"), | |
| `Update the name of a backup with ID "xxx"`, | |
| "$ stackit volume backup update xxx --name new-name"), |
| `Update backup labels`, | ||
| "$ stackit volume backup update xxx-xxx-xxx --labels key1=value1,key2=value2"), |
There was a problem hiding this comment.
| `Update backup labels`, | |
| "$ stackit volume backup update xxx-xxx-xxx --labels key1=value1,key2=value2"), | |
| `Update the labels of a backup with ID "xxx"`, | |
| "$ stackit volume backup update xxx --labels key1=value1,key2=value2"), |
Description
Onboarding the volume backup API https://docs.api.stackit.cloud/documentation/iaas/version/v1#tag/Backups to manage the backup resources via the cli.
Checklist
make fmtmake generate-docs(will be checked by CI)make test(will be checked by CI)make lint(will be checked by CI)Testing-Instructions
Create a test volume to have something to backup
stackit volume create --name backup-test-vol --size 10 --performance-class storage_premium_perf0 --availability-zone eu01-1 --labels initial=true,purpose=backup-testing
Create backup of the volume with name and labels
stackit volume backup create --source-id <VOLUME_ID> --source-type volume --name first-backup --labels state=original,test=true
List backups with limit and label selector
stackit volume backup list --limit 5 --label-selector state=original
Update backup with new name and labels
stackit volume backup update <BACKUP_ID> --name updated-backup --labels state=modified,test=true
Describe backup to verify changes
stackit volume backup describe <BACKUP_ID>
Restore backup
stackit volume backup restore <BACKUP_ID>
Clean up by deleting backup
stackit volume backup delete <BACKUP_ID>
Clean up by deleting volume
stackit volume delete <VOLUME_ID>