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
chore(storage): enable several tests for grpc + related fixes #6827
Conversation
Enables tests: BucketPolicyOnly, ACL, PredefinedACLs Fixes: gets all fields in GetBucket and GetObject (were missing ACLs); makes sure ACL is sent when ACL is not sent but PredefinedACL is; initializes Bucket_IamConfig (was causing segfault); sends UBLA field when disabled in an update Additionally cleans up acl checking in tests to use the same method
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.
Nice fixes!
@@ -522,7 +521,7 @@ func (c *grpcStorageClient) UpdateObject(ctx context.Context, bucket, object str | |||
} | |||
// Note: This API currently does not support entites using project ID. | |||
// Use project numbers in ACL entities. Pending b/233617896. | |||
if uattrs.ACL != nil { | |||
if uattrs.ACL != nil || len(uattrs.PredefinedACL) > 0 { |
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'm surprised that this is necessary for PredefinedACL given how it is set in L485
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.
This line is to make sure the update mask contains something so we can send the request
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.
Gotcha; I guess this is safe because if you specify a PredefinedACL, you are overwriting the existing ACLs anyway.
Enables tests: BucketPolicyOnly, ACL, PredefinedACLs
Fixes: gets all fields in GetBucket and GetObject (were missing ACLs); makes sure ACL is sent when ACL is not sent but PredefinedACL is; initializes Bucket_IamConfig (was causing segfault); sends UBLA field when disabled in an update
Additionally cleans up acl checking in tests to use the same method