Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 2 commits into from

Conversation

Rondineli
Copy link

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. Fixing reviews on (29217)[https://github.com//pull/29217]

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


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

@aws-cdk-automation aws-cdk-automation requested a review from a team April 10, 2024 16:42
@github-actions github-actions bot added p2 beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels Apr 10, 2024
@Rondineli Rondineli marked this pull request as ready for review April 10, 2024 16:54
@Rondineli
Copy link
Author

Rondineli commented Apr 10, 2024

@paulhcsun I fixed the PR based on your requests and also, changed it to have a safeguard where we are not deleting only the waf objects list and everything else should be exactly the same. If you check the original issue 29165 The suggested scenarios is currently not working and this should fix part of the problem.

LEt me know if there is anything else I can do to make it more to what aws expects.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@vinayak-kukreja
Copy link
Contributor

Hey @Rondineli, thank you for opening a PR with us and apologies for the delay.

I do not agree with the approach in this PR that we are taking to resolve the issue. The keys being mentioned to be excluded can be used in some other scenarios and could lead to potential issues. Even if it works for wafv2, we will want this to be more generic for other resources too.

IMO instead of adding exemption/exception keys for this, we need to fix the underlying issue and that is related to how deepMerge is working.

I have added a comment to the issue you opened. Please go through it and let us know if you feel comfortable working on the suggested approach, or if you would like us to work on this? :)

@Rondineli
Copy link
Author

@vinayak-kukreja Will adjust this PR to the requested approach :)

Thanks for the explanation and review!

@aws-cdk-automation
Copy link
Collaborator

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.

@vinayak-kukreja vinayak-kukreja added the pr-linter/do-not-close The PR linter will not close this PR while this label is present label May 6, 2024
@TheRealAmazonKendra TheRealAmazonKendra removed the pr-linter/do-not-close The PR linter will not close this PR while this label is present label Jun 6, 2024
@aws-cdk-automation
Copy link
Collaborator

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.

@aws-cdk-automation aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Jun 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. p2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants