fix(s3-notifications): cdk destroy deletes external/existing s3 notification events #29939
Conversation
github-actions
bot
added
bug
| @@ -95,8 +95,8 @@ def test_create(self, _, put: MagicMock): | |||
| index.handler(event, {}) | |||
|
|
|||
| put.assert_called_once_with( | |||
| event["ResourceProperties"]["BucketName"], | |||
| event["ResourceProperties"]["NotificationConfiguration"], | |||
| Bucket=event["ResourceProperties"]["BucketName"], | |||
There was a problem hiding this comment.
Need to update this test file because this PR only updated the handler file but didn't change or run the test files. This results in all these tests failing.
There was a problem hiding this comment.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.
GavinZZ
added
the
pr-linter/exempt-integ-test
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
| elif request_type == 'Delete': | ||
| # For 'Delete' request, old parameter is an empty dict so we cannot use this to determine which are external | ||
| # notifications. Fall back to rely on the stack naming logic. | ||
| external_notifications[t] = [n for n in existing_notifications.get(t, []) if not n['Id'].startswith(f"{stack_id}-")] |
There was a problem hiding this comment.
Falling back to the previous correct solution 37be7b9#diff-00d1f29e73955f487795d4a9873bcff824e70d6268e7926e44b7defb31b4ecb1L74
|
Does this suggested fix not work? 37be7b9#r137776660 |
aws-cdk-automation
added
the
pr/needs-maintainer-review
No, I added comment in code to explain why I had to do it this way. Basically the |
ah, didn't notice. LGTM |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
aws-cdk-automation
removed
the
pr/needs-maintainer-review
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
Comments on closed issues and PRs are hard for our team to see. If you need help, please open a new issue that references this one. |
Issue # (if applicable)
Closes #29004
Reason for this change
cdk destroyremoves all event notifications configured on an existing S3 bucket instead of only CDK managed event notifications. This occurs whenever a stack that creates an event notification for an existing bucket is deleted or rolled back.Description of changes
Add a
Deletestatement which will only remove the ones created from within the stackDescription of how you validated changes
Manually tested this.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license