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 80.64%. Comparing base (0731095) to head (467ebbd).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #32283   +/-   ##
=======================================
  Coverage   80.64%   80.64%           
=======================================
  Files         107      107           
  Lines        6996     6996           
  Branches     1290     1290           
=======================================
  Hits         5642     5642           
  Misses       1175     1175           
  Partials      179      179

Flag	Coverage Δ
suite.unit	`80.64% <ø> (ø)`

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

Components	Coverage Δ
packages/aws-cdk	`80.64% <ø> (ø)`

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

I looked at the integ code now; 467ebbd

Bucket and Table default to RETAIN, so with the current stack, all resources are RETAIN. So it would be nice to have a resource that would be DESTROY.
Sorry for repeating.

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.

aws-cdk-automation · 2024-12-26T14:59:49Z

AWS CodeBuild CI Report

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

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

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

integ

467ebbd

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

go-to-k Dec 26, 2024 •

edited

Loading

go-to-k Dec 26, 2024

aws-cdk-automation commented 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

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

Choose a reason for hiding this comment

go-to-k Dec 26, 2024

Choose a reason for hiding this comment

aws-cdk-automation commented Dec 26, 2024

AWS CodeBuild CI Report

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

go-to-k Dec 26, 2024 •

edited

Loading