Implementation of AWS::S3::* Resource Provider support for CloudFormation #9404
Conversation
Morijarti
added
aws:cloudformation
|
|
||
| try: | ||
| s3_client.delete_bucket_policy(Bucket=model["BucketName"]) | ||
| except s3_client.exceptions.ClientError: |
There was a problem hiding this comment.
In original code of BaseLegacyProvider we were catching Exception. Instead of it, I went with more specific exception here.
Should I revert it to Exception?
There was a problem hiding this comment.
@bentsku Probably would know if the NotFound exception is considered a ClientError.
There was a problem hiding this comment.
why do we need to manually delete the bucket policy in the first place? Shouldn't it be deleted by deleting the bucket?
It actually does not raise an error if the policy doesn't exist, just validated the behaviour in AWS 👍 it does not raise an error either in LocalStack with either v2 or v3 provider.
But it will raise a BucketNotFound if the bucket does not. Those are ClientError.
The only weird error in S3 is with HeadBucket, because it does not have a body, it will raise a ClientError but not a specific s3_client.exceptions.ClientError, I'm not sure why.
There was a problem hiding this comment.
I've removed explicit deletion of bucket policies and replaced exception with more general ClientError
Let me know If there is anything else I can do to improve the code.
Morijarti
requested review from
bentsku,
macnev2013,
dominikschubert,
pinzon and
simonrw
as code owners
# Conflicts: # localstack/services/cloudformation/resource_provider.py
- Replaced exception caught during call to head bucket
Changes
Implementation of support for: