fix(aws-cdk-lib): allow create explicitly empty trees on aws-cdk-lib core #29217

Rondineli · 2024-02-22T10:58:30Z

Issue # (if applicable)

Closes #29165.

Reason for this change

Using addOverride calls deepMerge, and few edge cases like wafv2 rules needs Actions and other keys with objects and empty objects.

Description of changes

Changing the code to be more realistic with the comment, where only null data is deleted

Description of how you validated changes

No tests, just making sure the current change would be compatible with current tests, should not break anything

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.

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

aws-cdk-automation · 2024-02-22T20:37:18Z

AWS CodeBuild CI Report

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

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

paddy-bedraggled · 2024-02-23T11:50:49Z

+1 request for this change

jeffersonfrazer · 2024-02-27T13:42:42Z

+1

fshermann · 2024-02-27T17:51:34Z

+1

paulhcsun

Left some comments, also you may need to rerun the integ tests locally with --update-on-failed and push any changes as the snapshots in the PR may not reflect your latest changes which might explain why it's failing.

paulhcsun · 2024-03-08T17:17:40Z

packages/aws-cdk-lib/core/lib/cfn-resource.ts

@@ -654,7 +664,6 @@ function deepMerge(target: any, ...sources: any[]) {
      if (key === '__proto__' || key === 'constructor') {
        continue;
      }
-


can you add back the linebreak here

paulhcsun · 2024-03-08T17:21:39Z

packages/aws-cdk-lib/core/lib/cfn-resource.ts

+// Availabe wafv2 empty objects
+// https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-wafv2-webacl-ruleaction.html


Suggested change

// Availabe wafv2 empty objects

// https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-wafv2-webacl-ruleaction.html

/**

* Availabe WAFv2 empty objects

*

* @see https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-wafv2-webacl-ruleaction.html

*/

paulhcsun · 2024-03-08T17:25:07Z

packages/aws-cdk-lib/core/lib/cfn-resource.ts

@@ -640,6 +640,16 @@ const MERGE_EXCLUDE_KEYS: string[] = [
  'Fn::Or',
 ];

+// Availabe wafv2 empty objects


This description is vague and doesn't explain what these are used for, nor how "empty objects" relates to DELETE_MERGE_EXCLUDE_KEYS. Maybe provide a short snippet of context, you can reference the description for MERGE_EXCLUDE_KEYS on line 617

paulhcsun · 2024-03-08T17:28:05Z

packages/aws-cdk-lib/core/test/resource.test.ts

@@ -541,6 +541,44 @@ describe('resource', () => {
      });
    });

+    test('addOverride(p, {object: {object: {}}}) will respect empty trees', () => {


prefer different wording like support over respect here

paulhcsun · 2024-03-08T17:29:47Z

packages/aws-cdk-lib/core/lib/cfn-resource.ts

@@ -725,7 +734,20 @@ function deepMerge(target: any, ...sources: any[]) {
        // sibling concrete values alongside, so we can delete this tree.
        const output = target[key];
        if (typeof(output) === 'object' && Object.keys(output).length === 0) {
-          delete target[key];
+          /*
+          * some empty trees are needed in some edge cases


Suggested change

* some empty trees are needed in some edge cases

* Some empty trees are needed in some edge cases

paulhcsun · 2024-03-08T17:33:42Z

packages/aws-cdk-lib/core/lib/cfn-resource.ts

+          * some empty trees are needed in some edge cases
+          * ie: wafv2 Rules (for Action, OverrideAction, etc...)


Please clarify which some empty trees and maybe combine the ie into the some edge cases like ... are needed when WAFv2 rule action is one of 'Action', 'Block', 'Captcha', 'Challenge' or 'Count'.

paulhcsun · 2024-03-12T16:47:48Z

@Rondineli I have some concerns about this change breaking existing customers even behind a feature flag. Please have a look at the comments on the original issue.

aws-cdk-automation · 2024-03-15T00:08:16Z

This PR has been in the BUILD FAILING 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.

Rondineli · 2024-03-15T16:17:51Z

@Rondineli I have some concerns about this change breaking existing customers even behind a feature flag. Please have a look at the comments on the original issue.

Sorry, but I really dont get the backwards compatibility bit? As I am explicitly setting the keys to it ignore. And trust me, I tried the way it is right now and cloudformation broke because those missing keys on waf rules, so, my guess is if someone tried to use it, it did not work.

I would be happy to change the PR for another approach, but this is definitely something broken and wont cause regression, quite the opposite.

aws-cdk-automation · 2024-03-22T00:09:13Z

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

fix(core) - Allow create explicity empty trees on aws-cdk-lib core

24e6d9d

github-actions bot added beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2 labels Feb 22, 2024

aws-cdk-automation requested a review from a team February 22, 2024 10:58

aws-cdk-automation previously requested changes Feb 22, 2024

View reviewed changes

Rondineli changed the title ~~fix(aws-cdk-lib) - Allow create explicitly empty trees on aws-cdk-lib core~~ fix(aws-cdk-lib): Allow create explicitly empty trees on aws-cdk-lib core Feb 22, 2024

rgomesdearaujo and others added 2 commits February 22, 2024 20:15

fix(aws-cdk-lib): Adding integ tests

b2f915a

Merge branch 'main' into fix-deep_merge_empty_objects

16fad97

Rondineli changed the title ~~fix(aws-cdk-lib): Allow create explicitly empty trees on aws-cdk-lib core~~ fix(aws-cdk-lib): allow create explicitly empty trees on aws-cdk-lib core Feb 22, 2024

Rondineli marked this pull request as ready for review February 22, 2024 20:19

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

Monthly PR metrics report - Feb 2023 #29328

Closed

paulhcsun requested changes Mar 8, 2024

View reviewed changes

udaypant linked an issue Mar 11, 2024 that may be closed by this pull request

wafv2: add_override does not obey Action key #29165

Open

aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Mar 22, 2024

aws-cdk-automation closed this Mar 22, 2024

Rondineli mentioned this pull request Apr 10, 2024

fix(aws-cdk-lib): allow create explicitly empty trees on aws-cdk-lib core #29784

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(aws-cdk-lib): allow create explicitly empty trees on aws-cdk-lib core #29217

fix(aws-cdk-lib): allow create explicitly empty trees on aws-cdk-lib core #29217

Rondineli commented Feb 22, 2024

aws-cdk-automation left a comment

aws-cdk-automation commented Feb 22, 2024

paddy-bedraggled commented Feb 23, 2024

jeffersonfrazer commented Feb 27, 2024

fshermann commented Feb 27, 2024

paulhcsun left a comment •

edited

Loading

paulhcsun Mar 8, 2024

paulhcsun Mar 8, 2024

paulhcsun Mar 8, 2024

paulhcsun Mar 8, 2024

paulhcsun Mar 8, 2024

paulhcsun Mar 8, 2024

paulhcsun commented Mar 12, 2024 •

edited

Loading

aws-cdk-automation commented Mar 15, 2024

Rondineli commented Mar 15, 2024

aws-cdk-automation commented Mar 22, 2024

		// Availabe wafv2 empty objects
		// https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-wafv2-webacl-ruleaction.html

	* some empty trees are needed in some edge cases
	* Some empty trees are needed in some edge cases

		* some empty trees are needed in some edge cases
		* ie: wafv2 Rules (for Action, OverrideAction, etc...)

fix(aws-cdk-lib): allow create explicitly empty trees on aws-cdk-lib core #29217

fix(aws-cdk-lib): allow create explicitly empty trees on aws-cdk-lib core #29217

Conversation

Rondineli commented Feb 22, 2024

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

aws-cdk-automation commented Feb 22, 2024

AWS CodeBuild CI Report

paddy-bedraggled commented Feb 23, 2024

jeffersonfrazer commented Feb 27, 2024

fshermann commented Feb 27, 2024

paulhcsun left a comment • edited Loading

Choose a reason for hiding this comment

paulhcsun Mar 8, 2024

Choose a reason for hiding this comment

paulhcsun Mar 8, 2024

Choose a reason for hiding this comment

paulhcsun Mar 8, 2024

Choose a reason for hiding this comment

paulhcsun Mar 8, 2024

Choose a reason for hiding this comment

paulhcsun Mar 8, 2024

Choose a reason for hiding this comment

paulhcsun Mar 8, 2024

Choose a reason for hiding this comment

paulhcsun commented Mar 12, 2024 • edited Loading

aws-cdk-automation commented Mar 15, 2024

Rondineli commented Mar 15, 2024

aws-cdk-automation commented Mar 22, 2024

paulhcsun left a comment •

edited

Loading

paulhcsun commented Mar 12, 2024 •

edited

Loading