fix(cloudformation-include): drops unknown policy attributes #32321

rix0rrr · 2024-11-28T16:47:39Z

Unknown fields in CreationPolicy/UpdatePolicy were dropped while loading resources using CfnInclude.

In this PR, handle unknown fields in those places like unknown fields in other places: retain them during parsing and add them as overrides.

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

Unknown fields in `CreationPolicy`/`UpdatePolicy` were dropped while loading resources using `CfnInclude`. In this PR, handle unknown fields in those places like unknown fields in other places: retain them during parsing and add them as overrides.

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.

otaviomacedo · 2024-11-28T16:52:10Z

packages/aws-cdk-lib/core/lib/helpers-internal/cfn-parse.ts

-      });
+      const sig = new ObjectParser<CfnResourceSignal>(p);
+      sig.parse('count', 'Count', FromCloudFormation.getNumber);
+      sig.parse('timeout', 'Tiemout', FromCloudFormation.getString);


Suggested change

sig.parse('timeout', 'Tiemout', FromCloudFormation.getString);

sig.parse('timeout', 'Timeout', FromCloudFormation.getString);

Perhaps we could have special method, built on top of parse, that converts the cases automatically, to reduce the risk of typos like this one.

parseConvertingCase('timeout', FromCloudFormation.getString);

codecov · 2024-11-28T17:00:42Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.50%. Comparing base (24adca3) to head (5700dec).

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #32321      +/-   ##
==========================================
+ Coverage   78.46%   78.50%   +0.03%     
==========================================
  Files         106      106              
  Lines        7208     7201       -7     
  Branches     1323     1321       -2     
==========================================
- Hits         5656     5653       -3     
+ Misses       1364     1361       -3     
+ Partials      188      187       -1

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

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

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

mrgrain · 2024-11-28T18:03:50Z

packages/aws-cdk-lib/core/lib/helpers-internal/cfn-parse.ts

+  }
+
+  // eslint-disable-next-line max-len
+  public parse<K extends keyof T>(targetKey: K, key: string, parser: (x: any) => T[K] | FromCloudFormationResult<T[K] | IResolvable>) {


Suggested change

public parse<K extends keyof T>(targetKey: K, key: string, parser: (x: any) => T[K] | FromCloudFormationResult<T[K] | IResolvable>) {

public parse<K extends Extract<keyof T, string>>(targetKey: K, key: Capitalize<K>, parser: (x: any) => T[K] | FromCloudFormationResult<T[K] | IResolvable>) {

But I agree with @otaviomacedo A simple firstCharUpper would be even better.

I didn't because we can't assume this is true in general. But I guess this is on a small enough domain that it's always true here.

Didn't know about Capitalize btw. That's neat.

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

mergify · 2024-11-29T10:08:47Z

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

mergify · 2024-11-29T10:41:47Z

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

…b.com:aws/aws-cdk into huijbers/parse-unrecognized-policy-attributes

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.

…b.com:aws/aws-cdk into huijbers/parse-unrecognized-policy-attributes

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

mergify · 2024-12-02T16:53:32Z

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

aws-cdk-automation · 2024-12-02T17:24:01Z

AWS CodeBuild CI Report

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

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

mergify · 2024-12-02T17:24:13Z

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-12-02T17:24:27Z

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.

rix0rrr requested a review from a team as a code owner November 28, 2024 16:47

aws-cdk-automation requested a review from a team November 28, 2024 16:47

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

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

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

View reviewed changes

otaviomacedo reviewed Nov 28, 2024

View reviewed changes

mrgrain reviewed Nov 28, 2024

View reviewed changes

Auto-convert casing, it works

307691b

rix0rrr requested review from otaviomacedo and mrgrain November 28, 2024 19:25

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

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Nov 28, 2024

mrgrain approved these changes Nov 29, 2024

View reviewed changes

Merge branch 'main' into huijbers/parse-unrecognized-policy-attributes

424e306

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Nov 29, 2024

rix0rrr added 2 commits November 29, 2024 14:21

CodeCov

abaf560

Merge branch 'huijbers/parse-unrecognized-policy-attributes' of githu…

79ecae6

…b.com:aws/aws-cdk into huijbers/parse-unrecognized-policy-attributes

rix0rrr requested a deployment to test-pipeline November 29, 2024 13:22 — with GitHub Actions Waiting

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

View reviewed changes

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

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

Monthly PR metrics report #32340

Open

Raise default timeout

91a1432

rix0rrr requested a deployment to test-pipeline December 2, 2024 10:12 — with GitHub Actions Waiting

Trigger yet another rebuild because that's just what we do

78643ca

rix0rrr requested a deployment to test-pipeline December 2, 2024 13:03 — with GitHub Actions Waiting

rix0rrr added 2 commits December 2, 2024 14:23

Newliiiiiiiiiiiiiine

e51e2a0

Merge branch 'huijbers/parse-unrecognized-policy-attributes' of githu…

90de287

…b.com:aws/aws-cdk into huijbers/parse-unrecognized-policy-attributes

rix0rrr requested a deployment to test-pipeline December 2, 2024 13:32 — with GitHub Actions Waiting

rix0rrr added the pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested label Dec 2, 2024

aws-cdk-automation removed the pr/needs-cli-test-run This PR needs CLI tests run against it. label Dec 2, 2024

Merge branch 'main' into huijbers/parse-unrecognized-policy-attributes

5700dec

mergify bot requested a deployment to test-pipeline December 2, 2024 16:54 Waiting

mergify bot merged commit 20edc7f into main Dec 2, 2024
16 of 17 checks passed

mergify bot deleted the huijbers/parse-unrecognized-policy-attributes branch December 2, 2024 17:24

github-actions bot locked as resolved and limited conversation to collaborators Dec 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cloudformation-include): drops unknown policy attributes #32321

fix(cloudformation-include): drops unknown policy attributes #32321

rix0rrr commented Nov 28, 2024

aws-cdk-automation left a comment

otaviomacedo Nov 28, 2024

codecov bot commented Nov 28, 2024 •

edited

Loading

mrgrain Nov 28, 2024 •

edited

Loading

mrgrain Nov 28, 2024

rix0rrr Nov 28, 2024

rix0rrr Nov 28, 2024

mergify bot commented Nov 29, 2024

mergify bot commented Nov 29, 2024

aws-cdk-automation left a comment

mergify bot commented Dec 2, 2024

aws-cdk-automation commented Dec 2, 2024

mergify bot commented Dec 2, 2024

github-actions bot commented Dec 2, 2024

	sig.parse('timeout', 'Tiemout', FromCloudFormation.getString);
	sig.parse('timeout', 'Timeout', FromCloudFormation.getString);

	public parse<K extends keyof T>(targetKey: K, key: string, parser: (x: any) => T[K] \| FromCloudFormationResult<T[K] \| IResolvable>) {
	public parse<K extends Extract<keyof T, string>>(targetKey: K, key: Capitalize<K>, parser: (x: any) => T[K] \| FromCloudFormationResult<T[K] \| IResolvable>) {

fix(cloudformation-include): drops unknown policy attributes #32321

fix(cloudformation-include): drops unknown policy attributes #32321

Conversation

rix0rrr commented Nov 28, 2024

aws-cdk-automation left a comment

Choose a reason for hiding this comment

otaviomacedo Nov 28, 2024

Choose a reason for hiding this comment

codecov bot commented Nov 28, 2024 • edited Loading

Codecov Report

mrgrain Nov 28, 2024 • edited Loading

Choose a reason for hiding this comment

mrgrain Nov 28, 2024

Choose a reason for hiding this comment

rix0rrr Nov 28, 2024

Choose a reason for hiding this comment

rix0rrr Nov 28, 2024

Choose a reason for hiding this comment

mergify bot commented Nov 29, 2024

mergify bot commented Nov 29, 2024

aws-cdk-automation left a comment

Choose a reason for hiding this comment

mergify bot commented Dec 2, 2024

aws-cdk-automation commented Dec 2, 2024

AWS CodeBuild CI Report

mergify bot commented Dec 2, 2024

github-actions bot commented Dec 2, 2024

codecov bot commented Nov 28, 2024 •

edited

Loading

mrgrain Nov 28, 2024 •

edited

Loading