Skip to content
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

feat: implement GrpcStorageImpl#createDefaultAcl & GrpcStorageImpl#updateDefaultAcl #1806

Merged
merged 2 commits into from Dec 14, 2022

Conversation

BenWhitehead
Copy link
Collaborator

No description provided.

@BenWhitehead BenWhitehead requested a review from a team as a code owner December 8, 2022 20:23
@product-auto-label product-auto-label bot added size: m Pull request size is medium. api: storage Issues related to the googleapis/java-storage API. labels Dec 8, 2022
@BenWhitehead BenWhitehead force-pushed the grpc/defaultAcl/create branch 2 times, most recently from db4e8ff to 8fd605b Compare December 8, 2022 22:13
@BenWhitehead BenWhitehead changed the title feat: implement GrpcStorageImpl#createDefaultAcl feat: implement GrpcStorageImpl#createDefaultAcl & GrpcStorageImpl#updateDefaultAcl Dec 8, 2022
@BenWhitehead BenWhitehead added the owlbot:ignore instruct owl-bot to ignore a PR label Dec 9, 2022
.map(codecs.objectAcl()::decode);

return first.orElseThrow(
() -> new StorageException(404, "Acl update call success, but not in response"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When would this occur?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hopefully never, but since we have to search for the updated acl in the response, if the response somehow doesn't included the updated acl we need to signal that somehow. 404 here doesn't feel totally correct, but I'm not sure 500 is better. What do you think?

Copy link
Member

@frankyn frankyn Dec 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure it's necessary tbh, you can only have up to 100 acls associated to a bucket or object;
image

https://cloud.google.com/storage/docs/access-control/lists#:~:text=use%20IAM%20exclusively.-,What%20is%20an%20access%20control%20list%3F,of%20one%20or%20more%20entries.

Thinking we just rely on the service response code in this case unless I'm misunderstanding.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spoke with @frankyn offline, we came to the consensus that instead of returning an HTTP status code here, insead we will return undefined 0 to signal that it's purely a client side issue, not any system issue.

Base automatically changed from grpc/defaultAcl/list to main December 13, 2022 17:34
@BenWhitehead BenWhitehead merged commit 0f24a11 into main Dec 14, 2022
@BenWhitehead BenWhitehead deleted the grpc/defaultAcl/create branch December 14, 2022 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the googleapis/java-storage API. owlbot:ignore instruct owl-bot to ignore a PR size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants