chore(storage): add UpdateBucketACL implementation #5974
Conversation
product-auto-label
bot
added
size: m
| @@ -986,11 +986,17 @@ func (ua *BucketAttrsToUpdate) toProtoBucket() *storagepb.Bucket { | |||
| lifecycle = *ua.Lifecycle | |||
| } | |||
| var bktACL []*storagepb.BucketAccessControl | |||
| if ua.acl != nil { | |||
| bktACL = toProtoBucketACL(ua.acl) | |||
| } | |||
| if ua.PredefinedACL != "" { | |||
There was a problem hiding this comment.
Very confused about what is going on with PredefinedACL and PredefinedDefaultObjectACL here... it doesn't seem like they are actually being set on the returned bucket?
There was a problem hiding this comment.
IIUC, one cannot provide both a predefinedAcl and an acl while patching/updating a bucket. I tried out buckets.patch and buckets.update with the JSON API, and it results in a 409 conflict error.
So in the library code here,, PredefinedACL and PredefinedDefaultObjectACL take precedence and therefore clears ACLs.
That being said, I believe I need to handle the updateMask with more care under the update semantics. Let me know if this clarifies your question.
There was a problem hiding this comment.
Ahh, gotcha. Yeah I think ForceSendFields plays the same role for JSON that the updateMask plays for gRPC
There was a problem hiding this comment.
Appreciate the review, Chris! Added some inline comments. Hopefully that's clearer for readers and our future selves.
storage/http_client.go
Outdated
| acl, err = req.Do() | ||
| aclRule = toBucketACLRule(acl) | ||
| return err | ||
| }, s.retry, false) |
There was a problem hiding this comment.
Same here, use s.idempotent so we can set it above the interface
| @@ -986,11 +986,17 @@ func (ua *BucketAttrsToUpdate) toProtoBucket() *storagepb.Bucket { | |||
| lifecycle = *ua.Lifecycle | |||
| } | |||
| var bktACL []*storagepb.BucketAccessControl | |||
| if ua.acl != nil { | |||
| bktACL = toProtoBucketACL(ua.acl) | |||
| } | |||
| if ua.PredefinedACL != "" { | |||
There was a problem hiding this comment.
Ahh, gotcha. Yeah I think ForceSendFields plays the same role for JSON that the updateMask plays for gRPC
storage/grpc_client.go
Outdated
| if err != nil { | ||
| return nil, err | ||
| } | ||
| aclRule := b.ACL[len(b.ACL)-1] |
There was a problem hiding this comment.
Let's just return the ACLRule we sent rather than picking the last one off the list (I'm not sure if the ordering is guaranteed)
cojenco
added
the
automerge
gcf-merge-on-green
bot
removed
the
automerge
This adds UpdateBucketACL implementation
aclanddefaultObjectACLtoBucketAttrsToUpdate