-
Notifications
You must be signed in to change notification settings - Fork 21
Create proto definitions for cosi #8
Create proto definitions for cosi #8
Conversation
thanks @brahmaroutu. LGTM |
/assign wlan0 |
/approve |
/lgtm |
@xing-yang Can you please review. |
@brahmaroutu I have the same questions as @xing-yang |
27f9415
to
3a7ea28
Compare
3a7ea28
to
846906d
Compare
go.mod
Outdated
@@ -0,0 +1,9 @@ | |||
module github.com/container-object-storage-interface/spec | |||
|
|||
go 1.14 |
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.
Can this be updated to 1.15. 1.15 is required for k8s 1.19.
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.
Fixed
846906d
to
59aecec
Compare
@xing-yang @wlan0 I have updated the proto file as per the comments adding detailed documentation of the calls and parameters. Please review. |
59aecec
to
52441ec
Compare
cosi.proto
Outdated
rpc ProvisionerCreateBucket (ProvisionerCreateBucketRequest) returns (ProvisionerCreateBucketResponse) {} | ||
// This call is made to delete the bucket in the backend. | ||
// If the bucket has already been deleted, then no error should be returned. | ||
// If the bucket has already been deleted, then no error should be returned. |
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.
Line 65 and 66 are the same
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.
fixed, sorry
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.
Looks like you have not pushed your changes? I still see two duplicate lines:).
cosi.proto
Outdated
// This call grants access to a particular principal. The principal is the account for which this access should be granted. | ||
// If the principal is set, then it should be used as the username of the created credentials. | ||
// If the principal is empty, then a new service account should be created in the backend that satisfies the requested access_policy. | ||
// The principal returned in the repsonce will be used as the unique identifier for deleting this access by calling ProvisionerRevokeBucketAccess. |
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.
typo: repsonce -> response
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.
fixed
rpc ProvisionerGrantBucketAccess (ProvisionerGrantBucketAccessRequest) returns (ProvisionerGrantBucketAccessResponse); | ||
rpc ProvisionerRevokeBucketAccess (ProvisionerRevokeBucketAccessRequest) returns (ProvisionerRevokeBucketAccessResponse); | ||
} | ||
|
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.
Sounds good. In CSI spec repo, we only need to update spec.md and csi.proto will be generated as well.
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.
Looks like you forgot to push your changes?
cosi.proto
Outdated
rpc ProvisionerCreateBucket (ProvisionerCreateBucketRequest) returns (ProvisionerCreateBucketResponse) {} | ||
// This call is made to delete the bucket in the backend. | ||
// If the bucket has already been deleted, then no error should be returned. | ||
// If the bucket has already been deleted, then no error should be returned. |
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.
Looks like you have not pushed your changes? I still see two duplicate lines:).
52441ec
to
534dd4c
Compare
@xing-yang I am sorry I did not push the code. I now have updated the code. Please review. |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brahmaroutu, wlan0, xing-yang The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Create the initial photo definitions for COSI Provisioner.
/assign @wlan0