Added support for Upload Disk Feature #318
Conversation
| } | ||
|
|
||
| // SetUploadStatus | ||
| func SetUploadStatus(s *common.Status, dProgressPercentage, dUploadSizeInBytes, dFileSizeInBytes int64, err ...error) { |
There was a problem hiding this comment.
Out of scope for this PR - I'm curious why this file has func Set...() and passes in a common.Status - it'd make sense for the Status object to have methods to set its upload status, upload error, etc.
There was a problem hiding this comment.
Yeah I agree. This could be refactored to something like func (s *Status) SetUploadStatus(). I am not sure why this is done this way though. I just followed the pattern I saw in the rest of the file.
pkg/status/status.go
Outdated
| @@ -146,6 +167,8 @@ func GetStatuses(status *common.Status) map[string]*string { | |||
| if placementStatus != "" { | |||
| statuses["PlacementStatus"] = &placementStatus | |||
| } | |||
| ustate := status.GetUploadStatus().String() | |||
There was a problem hiding this comment.
Just to keep it consistent with the changes being made in the hydration PR, could we null check the result of status.GetUploadStatus like what's done here: https://github.com/microsoft/moc/pull/237/files#diff-1deea3647518183ee4915dd3c10ccfc7bb60e1ed860673c99eefaf5cfeefaa6b
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
pkg/status/status.go
Outdated
| @@ -129,6 +130,26 @@ func GetPlacementStatus(s *common.Status) common.PlacementStatusType { | |||
| return s.GetPlacementStatus().GetStatus() | |||
| } | |||
|
|
|||
| // Set UploadError | |||
| func SetUploadError(s *common.Status, err ...error) { | |||
| s.UploadStatus.LastUploadError = errors.ErrorToProto(err[0]) | |||
There was a problem hiding this comment.
I realize this might be following a pattern but there is a flaw that can result in a runtime panic. If someone calls this function with 0 errors (err ...error) is a variadic paramater that can be 0..many errors.
if it is 0 errors
this function will result in a runtime panic -> err[0] will panic due to out of bounds access.
suggested fix:
func SetUploadError(s *common.Status, err ...error) {
if len(err) == 0 {
s.UploadStatus.LastUploadError = nil
return
}
s.UploadStatus.LastUploadError = errors.ErrorToProto(err[0])
}
There was a problem hiding this comment.
If the only place we are calling is below then why do we need a variadic parameter?
There was a problem hiding this comment.
Thanks for pointing it out. I actually fixed it by removing the variadic err param.
| int64 fileSizeInBytes = 3; | ||
| Error lastUploadError = 4; | ||
| } | ||
|
|
||
| message Status { |
There was a problem hiding this comment.
I feel like it wouldn't be a bad idea to throw a todo here to re-evaluate how to approach resource Status so we don't just keep creating a giant status that gets applied to every resource. Like a VM with a nil upload status.
There was a problem hiding this comment.
Agreed. Added a comment.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
pkg/status/status.go
Outdated
| @@ -125,6 +126,26 @@ func GetPlacementStatus(s *common.Status) common.PlacementStatusType { | |||
| return s.GetPlacementStatus().GetStatus() | |||
| } | |||
|
|
|||
| // Set UploadError | |||
| func SetUploadError(s *common.Status, err ...error) { | |||
| s.UploadStatus.LastUploadError = errors.ErrorToProto(err[0]) | |||
There was a problem hiding this comment.
if len(err) > 0 { // check if it is not empty
There was a problem hiding this comment.
Sorry. I am not sure how I missed it. I have remove the variadic error parameter.
rpc/common/moc_common_common.proto
Outdated
| @@ -346,6 +356,7 @@ enum ProviderAccessOperation { | |||
|
|
|||
| VirtualHardDisk_Create = 3100; | |||
| VirtualHardDisk_Update = 3101; | |||
| VirtualHardDisk_Upload = 3102; | |||
There was a problem hiding this comment.
nit please remove tabs for formatting
There was a problem hiding this comment.
I am not sure why it shows up like this here. It looks good on my local setup.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
No description provided.