CloudBackup enumerate status conversion.#957
Conversation
Also fix bug in the default creds path for enumerate. Signed-off-by: veda <veda@portworx.com>
| t SdkCloudBackupStatusType, | ||
| ) string { | ||
| switch t { | ||
| case SdkCloudBackupStatusType_SdkCloudBackupStatusTypeNotStarted: |
There was a problem hiding this comment.
I think this is a problem with the SDK types. This fixes the issue with the status for the REST API but the SDK status strings still are "SdkCloudBackupStatusTypeDone". Why do users care about what interface was being used in the status string? And the status is already stored in the Backup Status field, so it doesn't need to have redundant information. The status string should just be "Done"
There was a problem hiding this comment.
Strings cannot be stored or described in gRPC proto file, only enums that is why it is called SdkCloudBackupStatusTypeDone.
There was a problem hiding this comment.
That just means the variable names need to be fixed. Please don't merge PRs when changes are requested on them
There was a problem hiding this comment.
You can look at the CloudMigrate status and stage strings on how to do this correctly: https://github.com/libopenstorage/openstorage/blob/master/api/api.pb.go#L992
There was a problem hiding this comment.
And this needs to be fixed for all the other SDK strings too
There was a problem hiding this comment.
@disrani-px the method you have above works well. Please write an issue so that we can fix all the values in the SDK to use that model. In the mean time, release-6.0 needs to have the current model.
3 participants
Also fix bug in the default creds path for enumerate.
Signed-off-by: veda veda@portworx.com
What this PR does / why we need it:
Status conversion to string while enumerating cloud backups.
Which issue(s) this PR fixes (optional)
Closes #
Special notes for your reviewer: