feat(core): `RemovalPolicies.of(scope)` #32283

watany-dev · 2024-11-26T04:05:22Z

Issue # (if applicable)

N/A - New feature proposal

Reason for this change

Currently, applying removal policies to multiple resources requires setting them individually or using Tags as a workaround. This change introduces a new RemovalPolicies module that provides a more intuitive and type-safe way to manage removal policies across multiple resources, similar to the existing Tags API.

Description of changes

Added a new RemovalPolicies module that provides:

A similar interface to Tags.of() for managing removal policies
Type-safe resource type specifications using CloudFormation resource type strings
Ability to include or exclude specific resource types
Convenient methods for common removal policies (destroy, retain, snapshot, retainOnUpdateOrDelete)

Example usage:

// Using CloudFormation resource type strings
RemovalPolicies.of(scope).retain({
  applyToResourceTypes: ['AWS::S3::Bucket', 'AWS::DynamoDB::Table']
});

const bucket = new s3.Bucket(scope, 'bucket')

// Using CDK resource classes (type-safe)
RemovalPolicies.of(scope).retain({
  applyToResourceTypes: [
       bucket.cfnResourceType,
       CfnTable.CFN_RESOURCE_TYPE_NAME,
  ]
});

// Mixed usage
RemovalPolicies.of(scope).retain({
  applyToResourceTypes: [bucket.cfnResourceType, 'AWS::DynamoDB::Table']
});

Description of how you validated changes

TBD

Checklist

[x] 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.

codecov · 2024-11-26T04:17:08Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.47%. Comparing base (0153da4) to head (5f6197c).
Report is 148 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #32283      +/-   ##
==========================================
+ Coverage   77.46%   79.47%   +2.00%     
==========================================
  Files         105      108       +3     
  Lines        7168     7158      -10     
  Branches     1314     1320       +6     
==========================================
+ Hits         5553     5689     +136     
+ Misses       1433     1285     -148     
- Partials      182      184       +2

Flag	Coverage Δ
suite.unit	`79.47% <ø> (+2.00%)`	⬆️

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

Components	Coverage Δ
packages/aws-cdk	`79.47% <ø> (+2.00%)`	⬆️

aws-cdk-automation · 2024-12-18T00:12:14Z

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

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

packages/aws-cdk-lib/core/lib/removal-policys.ts

go-to-k · 2024-12-21T17:17:15Z

packages/aws-cdk-lib/core/lib/removal-policies.ts

+    try {
+      cfnResource.applyRemovalPolicy(this.policy);
+    } catch (error) {
+      // Check if the error is an instance of the built-in Error class
+      if (error instanceof Error) {
+        throw new Error(`Failed to apply removal policy to resource type ${resourceType}: ${error.message}`);
+      } else {
+        // If it's not an Error instance, convert it to a string for the message
+        throw new Error(`Failed to apply removal policy to resource type ${resourceType}: ${String(error)}`);
+      }


Is this try/catch necessary? Are there any (bad) effects of not having it?
(IMO, I don't think we need this one.)

I thought about it, and it might not be necessary, so I removed it.

go-to-k · 2024-12-21T17:40:45Z

packages/aws-cdk-lib/core/lib/removal-policies.ts

+/**
+ * Properties for applying a removal policy
+ */
+export interface RemovalPolicyProps {


It would be good to add a property priority to RemovalPolicyProps? (ref: https://github.com/aws/aws-cdk/blob/v2.172.0/packages/aws-cdk-lib/core/lib/aspect.ts#L45)

(* The priority was introduced in v2.172.0: https://dev.to/aws-heroes/aws-cdk-aspects-specifications-have-changed-3i75.)

And the apply method will be changed like:

public apply(policy: RemovalPolicy, props: RemovalPolicyProps = {}) { Aspects.of(this.scope).add(new RemovalPolicyAspect(policy, props), { priority: props.priority ?? AspectPriority.MUTATING, }); }

Since the RemovalPolicy change is a kind of resource update, I think it is good that the default value should be MUTATING. (It is the same with the add and remove method of the Tags class.)

thanks.
dec21e2

Thanks for the change.

If multiple aspects apply conflicting settings, the one with the higher priority wins.

I found the words "the higher priority" and "wins" a little confusing.

The higher priority (lower numeric value) is applied first, and the lower priority (higher numeric value) is applied later. (Complicated explanation...)

In other words, the removal policy will be the value by the former if overwrite is false, and the value by the latter if true, right?

Can we explain that well?

packages/aws-cdk-lib/core/README.md

packages/aws-cdk-lib/core/lib/removal-policys.ts

go-to-k

I've only seen halfway through, but I'll leave you with my comments so far.

packages/aws-cdk-lib/core/lib/removal-policies.ts

packages/aws-cdk-lib/core/README.md

go-to-k · 2024-12-21T19:03:56Z

packages/aws-cdk-lib/core/README.md

+#### `RemovalPolicyProps` Interface
+
+Additional configuration options for applying removal policies.
+
+- **`applyToResourceTypes?`**: _(optional)_  
+  Array of CloudFormation resource types (e.g., `'AWS::S3::Bucket'`) to which the removal policy should be applied.  
+  Defaults to applying to all resources.
+
+- **`excludeResourceTypes?`**: _(optional)_  
+  Array of CloudFormation resource types to exclude from applying the removal policy.  
+  Defaults to no exclusions.
+
+- **`overwrite?`**: _(optional)_  
+  If `true`, the removal policy will overwrite any existing policy already set on the resource. Defaults to `false`.


Wouldn't it be difficult to maintain if all properties were listed on the README? (Because all of them are also in JSDoc...)

clean up this doc.
da6b4c5

go-to-k · 2024-12-22T02:02:47Z

packages/@aws-cdk-testing/framework-integ/test/core/test/integ.removal-policies.ts

+new s3.Bucket(stack, 'TestBucket');
+new dynamodb.Table(stack, 'TestTable', {
+  partitionKey: { name: 'id', type: dynamodb.AttributeType.STRING },
+});
+new iam.User(stack, 'TestUser');
+
+// Apply different removal policies to demonstrate functionality
+RemovalPolicies.of(stack).destroy();


How about creating a custom construct that attaches a retain method? The following situation can be realized:

Stack (with a destroy method)

Resource A: DESTROY

Resource B: DESTROY

RetainConstruct (with a retain method)

X: RETAIN

aws-cdk-automation · 2024-12-26T03:08:23Z

AWS CodeBuild CI Report

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

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

go-to-k · 2024-12-26T04:06:34Z

packages/aws-cdk-lib/core/test/removal-policies.test.ts

@@ -220,4 +220,18 @@ describe('removal-policys', () => {
    expect(bucket.cfnOptions.deletionPolicy).toBe('Retain');
    expect(table.cfnOptions.deletionPolicy).toBe('RetainExceptOnCreate');
  });
+
+  test('higher priority removal policy overrides lower priority removal policy', () => {


It would be good to add a new test using priority and overwrite with true.

1.0

70f9e43

watany-dev requested a review from a team as a code owner November 26, 2024 04:05

aws-cdk-automation requested a review from a team November 26, 2024 04:05

github-actions bot added p2 distinguished-contributor [Pilot] contributed 50+ PRs to the CDK labels Nov 26, 2024

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

View reviewed changes

watany-dev changed the title ~~feat(core): RemovalPolicys.of(scope)~~ feat(core): RemovalPolicys.of(scope) Nov 26, 2024

github-actions bot mentioned this pull request Dec 1, 2024

Monthly PR metrics report #32340

Open

watany-dev added 3 commits December 19, 2024 09:29

integ first

47a41f1

integ

5d76562

readme

3770eb8

watany-dev added 4 commits December 19, 2024 16:13

types

d4a2db3

exit resource

2437d3c

type check

3ca49c8

integ

e0db4c8

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Dec 20, 2024

go-to-k reviewed Dec 20, 2024

View reviewed changes

packages/aws-cdk-lib/core/lib/removal-policys.ts Outdated Show resolved Hide resolved

go-to-k reviewed Dec 20, 2024

View reviewed changes

packages/aws-cdk-lib/core/lib/removal-policys.ts Outdated Show resolved Hide resolved

watany-dev added 4 commits December 21, 2024 10:47

rename

42b82fd

RemovalPolicys to RemovalPolicies

710c35e

integ

2d90ae7

enum like class

f8d1734

watany-dev requested a review from go-to-k December 21, 2024 13:56

go-to-k reviewed Dec 21, 2024

View reviewed changes

go-to-k suggested changes Dec 21, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Dec 21, 2024

go-to-k reviewed Dec 21, 2024

View reviewed changes

packages/aws-cdk-lib/core/lib/removal-policies.ts Show resolved Hide resolved

go-to-k reviewed Dec 21, 2024

View reviewed changes

packages/aws-cdk-lib/core/README.md Outdated Show resolved Hide resolved

go-to-k reviewed Dec 21, 2024

View reviewed changes

watany-dev changed the title ~~feat(core): RemovalPolicys.of(scope)~~ feat(core): RemovalPolicies.of(scope) Dec 22, 2024

go-to-k reviewed Dec 22, 2024

View reviewed changes

watany-dev added 6 commits December 22, 2024 02:19

miss spell

af4e0ff

adding anyL1Type).CFN_RESOURCE_TYPE_NAME

5f6197c

remove try-catch

c0d4fd7

Merge remote-tracking branch 'origin/main' into removalpolicy-all

f2df86d

AspectPriority

dec21e2

readme updated

da6b4c5

watany-dev force-pushed the removalpolicy-all branch from 367ca39 to da6b4c5 Compare December 26, 2024 02:46

go-to-k reviewed Dec 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core): `RemovalPolicies.of(scope)` #32283

feat(core): `RemovalPolicies.of(scope)` #32283

watany-dev commented Nov 26, 2024 •

edited

Loading

aws-cdk-automation left a comment

codecov bot commented Nov 26, 2024 •

edited

Loading

aws-cdk-automation commented Dec 18, 2024

go-to-k Dec 21, 2024

watany-dev Dec 22, 2024

go-to-k Dec 21, 2024 •

edited

Loading

watany-dev Dec 26, 2024

go-to-k Dec 26, 2024 •

edited

Loading

go-to-k left a comment

go-to-k Dec 21, 2024 •

edited

Loading

watany-dev Dec 26, 2024

go-to-k Dec 22, 2024 •

edited

Loading

aws-cdk-automation commented Dec 26, 2024

go-to-k Dec 26, 2024

feat(core): RemovalPolicies.of(scope) #32283

Are you sure you want to change the base?

feat(core): RemovalPolicies.of(scope) #32283

Conversation

watany-dev commented Nov 26, 2024 • edited Loading

Issue # (if applicable)

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

codecov bot commented Nov 26, 2024 • edited Loading

Codecov Report

aws-cdk-automation commented Dec 18, 2024

go-to-k Dec 21, 2024

Choose a reason for hiding this comment

watany-dev Dec 22, 2024

Choose a reason for hiding this comment

go-to-k Dec 21, 2024 • edited Loading

Choose a reason for hiding this comment

watany-dev Dec 26, 2024

Choose a reason for hiding this comment

go-to-k Dec 26, 2024 • edited Loading

Choose a reason for hiding this comment

go-to-k left a comment

Choose a reason for hiding this comment

go-to-k Dec 21, 2024 • edited Loading

Choose a reason for hiding this comment

watany-dev Dec 26, 2024

Choose a reason for hiding this comment

go-to-k Dec 22, 2024 • edited Loading

Choose a reason for hiding this comment

aws-cdk-automation commented Dec 26, 2024

AWS CodeBuild CI Report

go-to-k Dec 26, 2024

Choose a reason for hiding this comment

feat(core): `RemovalPolicies.of(scope)` #32283

feat(core): `RemovalPolicies.of(scope)` #32283

watany-dev commented Nov 26, 2024 •

edited

Loading

codecov bot commented Nov 26, 2024 •

edited

Loading

go-to-k Dec 21, 2024 •

edited

Loading

go-to-k Dec 26, 2024 •

edited

Loading

go-to-k Dec 21, 2024 •

edited

Loading

go-to-k Dec 22, 2024 •

edited

Loading