fix(cli): externally managed stack notification arns are deleted on `deploy` #32163

iliapolo · 2024-11-17T10:45:29Z

Reason for this change

When a stack contains externally managed notification ARNs (i.e ones that were added outside of CDK), a cdk deploy command will remove those ARNs.

Description of changes

We incorrectly default notification ARNs to [] instead of undefined. 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.g undefined), 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

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

aws-cdk-automation

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.

iliapolo · 2024-11-17T14:57:16Z

packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/app/app.js

@@ -830,9 +841,7 @@ switch (stackSet) {
    new DockerInUseStack(app, `${stackPrefix}-docker-in-use`);
    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`],


Moved this to inside the stack, which now reads it from an env variable to avoid hard coding stuff.

iliapolo · 2024-11-17T14:59:17Z

packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli.integtest.ts

+  }
+}));
+
+integTest('deploy deletes ALL notification arns when empty array is passed', withDefaultFixture(async (fixture) => {


This is essentially testing SDK behavior but what the heck

iliapolo · 2024-11-17T15:06:38Z

packages/aws-cdk/lib/cdk-toolkit.ts

+      //  - 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)


This is the fix.

codecov · 2024-11-17T15:15:40Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.18%. Comparing base (b70ad52) to head (b63d5c0).
Report is 1 commits behind head on main.

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

Flag	Coverage Δ
suite.unit	`77.18% <100.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
packages/aws-cdk	`77.18% <100.00%> (-0.01%)`	⬇️

mrgrain · 2024-11-17T19:36:52Z

Build failed in CodeBuild. Not clear why status not reported back.

iliapolo · 2024-11-18T08:19:20Z

Waiting for the pipeline to succeed

aws-cdk-automation · 2024-11-18T11:35:30Z

➡️ PR build request submitted to test-main-pipeline ⬅️

A maintainer must now check the pipeline and add the pr-linter/cli-integ-tested label once the pipeline succeeds.

aws-cdk-automation · 2024-11-18T12:03:24Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: b63d5c0
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

mergify · 2024-11-18T13:57:30Z

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).

github-actions · 2024-11-18T13:57:57Z

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.

mid work

2e02dbf

github-actions bot added the p2 label Nov 17, 2024

aws-cdk-automation requested a review from a team November 17, 2024 10:45

mergify bot added the contribution/core This is a PR that came from AWS. label Nov 17, 2024

github-actions bot added bug This issue is a bug. effort/medium Medium work item – several days of effort p0 and removed p2 labels Nov 17, 2024

iliapolo had a problem deploying to test-pipeline November 17, 2024 10:46 — with GitHub Actions Failure

aws-cdk-automation previously requested changes Nov 17, 2024

View reviewed changes

aws-cdk-automation added the pr/needs-cli-test-run This PR needs CLI tests run against it. label Nov 17, 2024

mid work

293e4d6

iliapolo had a problem deploying to test-pipeline November 17, 2024 14:56 — with GitHub Actions Failure

mid work

c940d55

iliapolo commented Nov 17, 2024

View reviewed changes

iliapolo had a problem deploying to test-pipeline November 17, 2024 14:57 — with GitHub Actions Failure

iliapolo commented Nov 17, 2024

View reviewed changes

mid work

8a082de

iliapolo temporarily deployed to test-pipeline November 17, 2024 15:05 — with GitHub Actions Inactive

iliapolo commented Nov 17, 2024

View reviewed changes

iliapolo marked this pull request as ready for review November 17, 2024 15:07

mrgrain approved these changes Nov 17, 2024

View reviewed changes

mrgrain added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Nov 17, 2024

iliapolo removed the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Nov 18, 2024

iliapolo added 2 commits November 18, 2024 09:45

allow breaking change

cc2c4b1

Merge branch 'main' into epolon/preserver-notification-arns

c65ccc3

iliapolo temporarily deployed to test-pipeline November 18, 2024 07:46 — with GitHub Actions Inactive

iliapolo added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Nov 18, 2024

wait for stack update complete

b63d5c0

iliapolo temporarily deployed to test-pipeline November 18, 2024 11:34 — with GitHub Actions Inactive

mrgrain added pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested and removed pr/needs-cli-test-run This PR needs CLI tests run against it. labels Nov 18, 2024

mergify bot merged commit 9966f57 into main Nov 18, 2024
23 of 24 checks passed

mergify bot deleted the epolon/preserver-notification-arns branch November 18, 2024 13:57

github-actions bot locked as resolved and limited conversation to collaborators Nov 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cli): externally managed stack notification arns are deleted on `deploy` #32163

fix(cli): externally managed stack notification arns are deleted on `deploy` #32163

iliapolo commented Nov 17, 2024 •

edited

Loading

aws-cdk-automation left a comment

iliapolo Nov 17, 2024 •

edited

Loading

iliapolo Nov 17, 2024

iliapolo Nov 17, 2024

codecov bot commented Nov 17, 2024 •

edited

Loading

mrgrain commented Nov 17, 2024

iliapolo commented Nov 18, 2024

aws-cdk-automation commented Nov 18, 2024

aws-cdk-automation commented Nov 18, 2024

mergify bot commented Nov 18, 2024

github-actions bot commented Nov 18, 2024

fix(cli): externally managed stack notification arns are deleted on deploy #32163

fix(cli): externally managed stack notification arns are deleted on deploy #32163

Conversation

iliapolo commented Nov 17, 2024 • edited Loading

Reason for this change

Description of changes

Description of how you validated changes

Checklist

aws-cdk-automation left a comment

Choose a reason for hiding this comment

iliapolo Nov 17, 2024 • edited Loading

Choose a reason for hiding this comment

iliapolo Nov 17, 2024

Choose a reason for hiding this comment

iliapolo Nov 17, 2024

Choose a reason for hiding this comment

codecov bot commented Nov 17, 2024 • edited Loading

Codecov Report

mrgrain commented Nov 17, 2024

iliapolo commented Nov 18, 2024

aws-cdk-automation commented Nov 18, 2024

aws-cdk-automation commented Nov 18, 2024

AWS CodeBuild CI Report

mergify bot commented Nov 18, 2024

github-actions bot commented Nov 18, 2024

fix(cli): externally managed stack notification arns are deleted on `deploy` #32163

fix(cli): externally managed stack notification arns are deleted on `deploy` #32163

iliapolo commented Nov 17, 2024 •

edited

Loading

iliapolo Nov 17, 2024 •

edited

Loading

codecov bot commented Nov 17, 2024 •

edited

Loading