ExpectedBucketOwner for S3 bucket policy operations #11827
Conversation
There was a problem hiding this comment.
Welcome to LocalStack! Thanks for raising your first Pull Request and landing in your contributions. Our team will reach out with any reviews or feedbacks that we have shortly. We recommend joining our Slack Community and share your PR on the #community channel to share your contributions with us. Please make sure you are following our contributing guidelines and our Code of Conduct.
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
2b62387 to
b541ef1
Compare
There was a problem hiding this comment.
This looks really great! I really like the approach, using existing methods, clearly splitting tests for what they are really testing, and allowing a progressive implementation of the ExpectedBucketOwner parameter through S3 API operations.
I really appreciate the thorough testing and use of parametrization.
I only have 2 comments:
- We cannot modify the
localstack-core/localstack/aws/api/s3/__init__.pyfile manually as it is auto-generated, and while running our automatic spec generation pipeline every week, those changes would be overwritten. I've added a comment to resolve this point, and is the only "blocking" point of the PR before a merge - other very minor comment is related to the clean-up of the automatically generated file, while renaming tests. This is also a shortcoming of our utility, and is not really blocking but would be appreciated.
All in all, the implementation is really clean, congratulations. 🚀
Once the comments are addressing, this is good to merge ✅
edit: thanks for also creating the GitHub issue linked to the PR 🙏
| self, | ||
| context: RequestContext, | ||
| bucket_name: BucketName, | ||
| *, |
There was a problem hiding this comment.
very nice use of * to force the use of the keyword-only parameters! 🚀
| @@ -964,31 +978,132 @@ def test_create_bucket_via_host_name(self, s3_vhost_client, aws_client, region_n | |||
| s3_vhost_client.delete_bucket(Bucket=bucket_name) | |||
|
|
|||
| @markers.aws.validated | |||
| def test_put_and_get_bucket_policy(self, s3_bucket, snapshot, aws_client, allow_bucket_acl): | |||
There was a problem hiding this comment.
nice refactoring of the tests, this makes it easier to validate all the use-cases now 💯
|
Thanks for the feedback @bentsku |
Motivation
Closes #11826
Changes
Implement the ExpectedBucketOwner parameter and its behavior in 3 S3 operations:
This parameter is currently ignored in LocalStack S3.