-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
Add ability to import Google Compute persistent disks #14573
Add ability to import Google Compute persistent disks #14573
Conversation
bashtoni
commented
May 17, 2017
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.
Thank you for the PR and sorry for a small delay in reviewing it.
The tests are passing, I just left you 2 comments, but overall this looks good.
If you can also resolve conflicts that would be helpful. 😃
resource, e = getZonalResourceFromRegion(getDisk, region, config.clientCompute, project) | ||
|
||
if e != nil { | ||
return e |
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.
Is there any specific reason we cannot use err
here for clarity?
if disk.SourceSnapshot != "" { | ||
snapshotUrl := strings.Split(disk.SourceSnapshot, "/") | ||
d.Set("snapshot", snapshotUrl[len(snapshotUrl)-1]) | ||
} |
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.
It may sound like nitpick and it may be just me, but I spent good couple of minutes trying to understand what all these changes above (with strings.Split
) mean.
So just for clarity/readability I think it would help if we renamed zoneUrl
to zoneUrlParts
, typeUrl
to typeUrlParts
etc. so that it's more obvious that these variables hold slice, not a string?
3609c35
to
4f5a51e
Compare
@radeksimko Reworded as suggested, fixed conflicts. Thanks for reviewing! |
@bashtoni Thanks for making those changes, would you mind changing the last two variables too? i.e. |
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |