-
Notifications
You must be signed in to change notification settings - Fork 20
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.
xing-yang
left a comment
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