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

feat: metadata collection for construct methods #33292

Merged
merged 7 commits into from
Feb 5, 2025

Conversation

GavinZZ
Copy link
Contributor

@GavinZZ GavinZZ commented Feb 4, 2025

Issue # (if applicable)

N/A

Reason for this change

Support construct public method logging. Method name, keys, and property values of Boolean and ENUM types - When you use an L2 construct method, we will collect the method name, property keys, and property values of Boolean and enum types.

Description of changes

Based on the discussion #33198. This change adds @MetadataMethod decorator to public construct methods which will allow metadata logging during synthesis into AWS::CDK::Metadata resource.

Describe any new or updated permissions being added

N/A

Description of how you validated changes

No integ test is possible as integ test requires analytics reporting to be false. Unit tests added.

Checklist


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

@GavinZZ GavinZZ requested a review from a team as a code owner February 4, 2025 20:32
@github-actions github-actions bot added the p2 label Feb 4, 2025
@aws-cdk-automation aws-cdk-automation requested a review from a team February 4, 2025 20:32
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Feb 4, 2025
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pull request linter fails with the following errors:

❌ Manual changes to the classes.ts file are not allowed.
❌ Manual changes to the enums.ts file are not allowed.

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.

✅ A exemption request has been requested. Please wait for a maintainer's review.

@GavinZZ GavinZZ added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Feb 4, 2025
@GavinZZ
Copy link
Contributor Author

GavinZZ commented Feb 4, 2025

The pull request linter fails with the following errors:

❌ Manual changes to the classes.ts file are not allowed.
❌ Manual changes to the enums.ts file are not allowed.

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.

This PR added a label to allow exempt but will not take effect until this PR is merged. Will ignore this check.

> ❌ Manual changes to the classes.ts file are not allowed.
> ❌ Manual changes to the enums.ts file are not allowed.

@aws-cdk-automation aws-cdk-automation added pr/reviewer-clarification-requested The contributor has requested clarification on feedback, a failing build, or a failing PR Linter run pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. labels Feb 4, 2025
@aws aws deleted a comment from aws-cdk-automation Feb 4, 2025
Copy link

codecov bot commented Feb 4, 2025

Codecov Report

Attention: Patch coverage is 90.47619% with 2 lines in your changes missing coverage. Please review.

Project coverage is 80.83%. Comparing base (3403da4) to head (255a3a9).
Report is 1 commits behind head on main.

❌ Your patch status has failed because the patch coverage (90.47%) is below the target coverage (95.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #33292      +/-   ##
==========================================
+ Coverage   80.80%   80.83%   +0.03%     
==========================================
  Files         236      236              
  Lines       14235    14253      +18     
  Branches     2487     2490       +3     
==========================================
+ Hits        11503    11522      +19     
+ Misses       2448     2446       -2     
- Partials      284      285       +1     
Flag Coverage Δ
suite.unit 80.83% <90.47%> (+0.03%) ⬆️

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

Components Coverage Δ
packages/aws-cdk 79.57% <ø> (ø)
packages/aws-cdk-lib/core 82.20% <90.47%> (+0.06%) ⬆️

@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Feb 4, 2025
@paulhcsun paulhcsun added the pr/do-not-merge This PR should not be merged at this time. label Feb 4, 2025
Copy link
Contributor

@paulhcsun paulhcsun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! Changes look good to me, approving and adding the do-not-merge label for now which can be removed after the test is added.

@aws-cdk-automation aws-cdk-automation removed pr/needs-maintainer-review This PR needs a review from a Core Team Member labels Feb 4, 2025
Copy link
Member

@godwingrs22 godwingrs22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @GavinZZ for this change. LGTM overall. Just a minor comment for clarification.

@godwingrs22 godwingrs22 self-assigned this Feb 4, 2025
@GavinZZ GavinZZ added pr-linter/exempt-codecov The PR linter will not require codecov checks to pass and removed pr/do-not-merge This PR should not be merged at this time. labels Feb 4, 2025
Copy link
Contributor

mergify bot commented Feb 4, 2025

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

Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pull request linter fails with the following errors:

❌ Manual changes to the classes.ts file are not allowed.
❌ Manual changes to the enums.ts file are not allowed.

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.

✅ A exemption request has been requested. Please wait for a maintainer's review.

@GavinZZ
Copy link
Contributor Author

GavinZZ commented Feb 4, 2025

@Mergifyio requeue

@GavinZZ
Copy link
Contributor Author

GavinZZ commented Feb 4, 2025

Going to manually override and merge this PR as this PR will always fail due to manual changes to classes.ts and enums.ts file. I added PR linter which will take effective after this is merged.

Copy link
Contributor

mergify bot commented Feb 4, 2025

requeue

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 255a3a9
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@GavinZZ
Copy link
Contributor Author

GavinZZ commented Feb 5, 2025

@mergify requeue

Copy link
Contributor

mergify bot commented Feb 5, 2025

requeue

☑️ This pull request is already queued

@aws aws deleted a comment from mergify bot Feb 5, 2025
@GavinZZ GavinZZ merged commit bc96ee1 into main Feb 5, 2025
18 of 21 checks passed
@GavinZZ GavinZZ deleted the yuanhaoz/metadata_methods branch February 5, 2025 00:53
Copy link

github-actions bot commented Feb 5, 2025

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 5, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
contribution/core This is a PR that came from AWS. p2 pr/reviewer-clarification-requested The contributor has requested clarification on feedback, a failing build, or a failing PR Linter run pr-linter/analytics-metadata-change pr-linter/exempt-codecov The PR linter will not require codecov checks to pass pr-linter/exempt-integ-test The PR linter will not require integ test changes pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants