fix(cli): externally managed stack notification arns are deleted on deploy#32163
fix(cli): externally managed stack notification arns are deleted on deploy#32163mergify[bot] merged 7 commits intomainfrom
deploy#32163Conversation
aws-cdk-automation
left a comment
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.
| new DockerStackWithCustomFile(app, `${stackPrefix}-docker-with-custom-file`); | ||
|
|
||
| new NotificationArnPropStack(app, `${stackPrefix}-notification-arn-prop`, { | ||
| notificationArns: [`arn:aws:sns:${defaultEnv.region}:${defaultEnv.account}:${stackPrefix}-test-topic-prop`], |
There was a problem hiding this comment.
Moved this to inside the stack, which now reads it from an env variable to avoid hard coding stuff.
| } | ||
| })); | ||
|
|
||
| integTest('deploy deletes ALL notification arns when empty array is passed', withDefaultFixture(async (fixture) => { |
There was a problem hiding this comment.
This is essentially testing SDK behavior but what the heck
| // - undefined => cdk ignores it, as if it wasn't supported (allows external management). | ||
| // - []: => cdk manages it, and the user wants to wipe it out. | ||
| // - ['arn-1'] => cdk manages it, and the user wants to set it to ['arn-1']. | ||
| const notificationArns = (!!options.notificationArns || !!stack.notificationArns) |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #32163 +/- ##
==========================================
- Coverage 77.18% 77.18% -0.01%
==========================================
Files 105 105
Lines 7163 7161 -2
Branches 1311 1312 +1
==========================================
- Hits 5529 5527 -2
Misses 1454 1454
Partials 180 180
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
Build failed in CodeBuild. Not clear why status not reported back. |
|
Waiting for the pipeline to succeed |
|
➡️ PR build request submitted to A maintainer must now check the pipeline and add the |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
|
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. |
Closes #32153.
Reason for this change
When a stack contains externally managed notification ARNs (i.e ones that were added outside of CDK), a
cdk deploycommand will remove those ARNs.Description of changes
We incorrectly default notification ARNs to
[]instead ofundefined. When an empty array is passed to the SDK , it reasonably assumes you want to delete existing ARNs (because how otherwise would you delete them). If on the other hand you don't pass notification ARNs at all to the SDK (e.gundefined), it will preserve them.This is the correct behavior and CDK should follow suite. This does however create a (maybe) quirky API ergonomic where in order to remove notification ARNs, one must pass
[]instead of simply omitting the property.This stems from the fact notification ARNs are not managed through the template, but rather through imperative actions. So it seems reasonable al things considered.
Description of how you validated changes
Added both unit and integration tests.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license