-
Notifications
You must be signed in to change notification settings - Fork 373
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
Clean up proto naming #176
Clean up proto naming #176
Conversation
/assign @saad-ali |
csi.proto
Outdated
@@ -413,7 +413,7 @@ message ListVolumesRequest { | |||
|
|||
message ListVolumesResponse { | |||
message Entry { | |||
VolumeInfo volume_info = 1; | |||
Volume volume_info = 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.
volume_info
-> volume
?
Minor change requested, otherwise, LGTM |
@saad-ali thanks for catching that, fixed. |
@jieyu please review when you have a chance |
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.
The rest LGTM. Only one naming nits.
@@ -717,7 +717,7 @@ message ControllerPublishVolumeResponse { | |||
// The SP specific information that will be passed to the Plugin in | |||
// the subsequent `NodePublishVolume` call for the given volume. | |||
// This information is opaque to the CO. This field is OPTIONAL. | |||
map<string, string> publish_volume_info = 1; | |||
map<string, string> publish_volume = 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.
I would actually rename this to publish_info
. publish_volume
sounds a bit weird.
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.
Yep, +1 for publish_info, done
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.
All of our fields that used to be uintXX
were typed that way because negative values aren't allowed. Now that they're converted to their sibling intXX
type, I'm wondering if we need additional language regarding the treatment of negative values? It certainly adds validation code for plugins and COs implementing the spec because now they need to deal with possibly invalid, negative numbers.
Closing as the branch stoped accepting any changes. This PR addresses the same issues: #182 |
Closes: #172 #173 #174
These are breaking changes from the 0.1 release.