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.

Sign up for GitHub

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

Jump to bottom

chore: add enum-like updater #33500

Merged
merged 13 commits into from
Feb 19, 2025
Merged

chore: add enum-like updater #33500

merged 13 commits into from
Feb 19, 2025
+9,177 −100

Conversation

Leo10Gama
Copy link
Member

Issue # (if applicable)

N/A

Reason for this change

Adds metadata collector for CDK's enum-like classes.

Description of changes

  • Added EnumLikeUpdater to parse out the enum-like classes and write them to a separate file to be used and updated.
  • Altered existing EnumUpdater to also write to another file, including the module name to prevent ambiguity when updating.

Describe any new or updated permissions being added

N/A

Description of how you validated changes

N/A

Checklist

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

@github-actions github-actions bot added the p2 label Feb 18, 2025
@aws-cdk-automation aws-cdk-automation requested a review from a team February 18, 2025 22:53
@Leo10Gama Leo10Gama changed the title Add enumlike updater feat: add enum-like updater Feb 18, 2025
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Feb 18, 2025
aws-cdk-automation
aws-cdk-automation previously requested changes Feb 18, 2025
View reviewed changes
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment •
edited
Loading

Choose a reason for hiding this comment

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

(This review is outdated)

@Leo10Gama Leo10Gama changed the title feat: add enum-like updater chore: add enum-like updater Feb 18, 2025
@aws-cdk-automation aws-cdk-automation dismissed their stale review February 18, 2025 22:55

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

@codecov Codecov
Copy link

codecov bot commented Feb 18, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.16%. Comparing base (ae544a1) to head (5a485bc).
Report is 1 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main   #33500   +/-   ##
=======================================
  Coverage   82.16%   82.16%           
=======================================
  Files         119      119           
  Lines        6857     6857           
  Branches     1157     1157           
=======================================
  Hits         5634     5634           
  Misses       1120     1120           
  Partials      103      103
Flag Coverage Δ
suite.unit 82.16% <ø> (ø)

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

Components Coverage Δ
packages/aws-cdk ∅ <ø> (∅)
packages/aws-cdk-lib/core 82.16% <ø> (ø)

GavinZZ
GavinZZ requested changes Feb 18, 2025
View reviewed changes
Copy link
Contributor

@GavinZZ GavinZZ left a comment

Choose a reason for hiding this comment

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

Some general comments:

  1. Add some unit tests for the new class methods
  2. Where's the generated content? I don't see it in this PR. Is that expected?
  3. You might want to add to pr-linter to exempt changes to allow you to modify the module enums file

tools/@aws-cdk/construct-metadata-updater/lib/metadata-updater.ts Outdated
@@ -515,20 +516,28 @@ export class EnumsUpdater extends MetadataUpdater {

// Add to the blueprint
enumBlueprint[enumName] = enumValues;
moduleEnumBlueprint[sourceFile.getFilePath().split("aws-cdk/packages/")[1].replace(".ts", "") + "." + enumName] = enumValues;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not exactly sure the format of JSON you want, but IMO, would it be more readable to split out the enum name and the module name so that we have something like

{
  'aws-cdk-lib.aws-ec2.Instance': {
     'EnumName': [ <values> ],
  }
}

instead of

{
  'aws-cdk-lib.aws-ec2.Instance'.EnumName: [ <values> ],
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Good callout, I had some discussion with Paul on this as well, JSON might be the better way to go with these moving forward. Will update with the better format

Copy link
Contributor

Choose a reason for hiding this comment

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

Either way works, but the actual tool script will parse and format it by module anyways so it doesn't strictly have to be organized in that way here.

@GavinZZ GavinZZ self-assigned this Feb 18, 2025
@Leo10Gama Leo10Gama requested a review from a team as a code owner February 18, 2025 23:16
aws-cdk-automation
aws-cdk-automation previously requested changes Feb 18, 2025
View reviewed changes
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment •
edited
Loading

Choose a reason for hiding this comment

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

(This review is outdated)

aws-cdk-automation
aws-cdk-automation previously requested changes Feb 18, 2025
View reviewed changes
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment •
edited
Loading

Choose a reason for hiding this comment

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

(This review is outdated)

@aws-cdk-automation aws-cdk-automation dismissed their stale review February 18, 2025 23:18

Dismissing outdated PRLinter review.

@Leo10Gama
Copy link
Member Author

Some general comments:

  1. Add some unit tests for the new class methods
  2. Where's the generated content? I don't see it in this PR. Is that expected?
  3. You might want to add to pr-linter to exempt changes to allow you to modify the module enums file
  1. Didn't see the test folder earlier, will add some 👍
  2. I thought I had included it in the PR, but they didn't get committed. Made a commit to include them now.
  3. Will add the linter exemption as well.

@aws-cdk-automation aws-cdk-automation dismissed their stale review February 18, 2025 23:24

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

Leo10Gama
Leo10Gama commented Feb 19, 2025
View reviewed changes
Copy link
Member Author

@Leo10Gama Leo10Gama left a comment

Choose a reason for hiding this comment

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

Did a deep dive on the enum removals, it seems like they are all due to the recent CLI split, and they took their enums with them

packages/aws-cdk-lib/core/lib/analytics-data-source/enums.ts
@@ -224,10 +224,6 @@ export const AWS_CDK_ENUMS: { [key: string]: any } = {
'objectLike',
'arrayWith'
],
'AssetBuildTime': [
Copy link
Member Author

Choose a reason for hiding this comment

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

Removed in commit 8b87a63 under these folders:

packages/@aws-cdk/toolkit/lib/actions/deploy/index.ts
packages/aws-cdk/lib/cli/cdk-toolkit.ts

packages/aws-cdk-lib/core/lib/analytics-data-source/enums.ts
@@ -896,12 +867,6 @@ export const AWS_CDK_ENUMS: { [key: string]: any } = {
'CONTINUE',
'ABANDON'
],
'DefaultSelection': [
Copy link
Member Author

Choose a reason for hiding this comment

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

Removed in commit 8b87a63 under this folder:

packages/aws-cdk/lib/api/cxapp/cloud-assembly.ts

packages/aws-cdk-lib/core/lib/analytics-data-source/enums.ts
@@ -927,14 +892,6 @@ export const AWS_CDK_ENUMS: { [key: string]: any } = {
'CODE_DEPLOY',
'EXTERNAL'
],
'DeploymentState': [
Copy link
Member Author

Choose a reason for hiding this comment

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

Removed in commit 8b87a63 under this folder:

packages/aws-cdk/lib/util/work-graph-types.ts

packages/aws-cdk-lib/core/lib/analytics-data-source/enums.ts
@@ -1177,11 +1133,6 @@ export const AWS_CDK_ENUMS: { [key: string]: any } = {
'SUPERSEDED',
'PARALLEL'
],
'ExtendedStackSelection': [
Copy link
Member Author

Choose a reason for hiding this comment

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

Removed in commit 8b87a63 under these folders:

packages/@aws-cdk/toolkit/lib/api/cloud-assembly/stack-selector.ts
packages/aws-cdk/lib/api/cxapp/cloud-assembly.ts

packages/aws-cdk-lib/core/lib/analytics-data-source/enums.ts
@@ -2907,12 +2837,6 @@ export const AWS_CDK_ENUMS: { [key: string]: any } = {
'StartsWith',
'NotEqual'
],
'RollbackChoice': [
Copy link
Member Author

Choose a reason for hiding this comment

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

Removed in commit 8b87a63 under this folder:

packages/aws-cdk/lib/api/util/cloudformation/stack-status.ts

packages/aws-cdk-lib/core/lib/analytics-data-source/enums.ts
@@ -2989,11 +2913,6 @@ export const AWS_CDK_ENUMS: { [key: string]: any } = {
'AddToLoadBalancer',
'InstanceRefresh'
],
'ScanStatus': [
Copy link
Member Author

Choose a reason for hiding this comment

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

Removed in commit 8b87a63 under this folder:

packages/aws-cdk/lib/commands/migrate.ts

packages/aws-cdk-lib/core/lib/analytics-data-source/enums.ts
@@ -3138,14 +3057,6 @@ export const AWS_CDK_ENUMS: { [key: string]: any } = {
'bar',
'events'
],
'StackSelectionStrategy': [
Copy link
Member Author

Choose a reason for hiding this comment

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

Removed in commit 8b87a63 under this folder:

packages/@aws-cdk/toolkit/lib/api/cloud-assembly/stack-selector.ts

packages/aws-cdk-lib/core/lib/analytics-data-source/enums.ts
@@ -3406,11 +3317,6 @@ export const AWS_CDK_ENUMS: { [key: string]: any } = {
'TcpRetryEvent': [
'connection-error'
],
'TemplateSourceOptions': [
Copy link
Member Author

Choose a reason for hiding this comment

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

Removed in commit 8b87a63 under this folder:

packages/aws-cdk/lib/commands/migrate.ts

packages/aws-cdk-lib/core/lib/analytics-data-source/enums.ts
@@ -1067,7 +1024,6 @@ export const AWS_CDK_ENUMS: { [key: string]: any } = {
'ECS_AL2_NVIDIA'
],
'Effect': [
'Unknown',
Copy link
Member Author

Choose a reason for hiding this comment

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

I think what was happening here was the enum was pulling the values from:

packages/@aws-cdk/cloudformation-diff/lib/iam/statement.ts

But without it, it's now pulling from:

packages/aws-cdk-lib/aws-iam/lib/policy-statement.ts

Which has the same values minus the 'Unknown' being removed here.

Copy link
Contributor

Choose a reason for hiding this comment

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

This may be because of the issue where the old script was not separating enums by module and overwriting when it found one with the same name. @GavinZZ can you confirm if you pushed the fix for this or if it's still the old code causing this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't plan to fix this in enums.ts. I intend to keep the overriden behaviour in the enums.ts file. I believe Leo fixed this in his new generated file.

GavinZZ
GavinZZ requested changes Feb 19, 2025
View reviewed changes
Copy link
Contributor

@GavinZZ GavinZZ left a comment

Choose a reason for hiding this comment

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

Do you plan to update the PR-linter in this PR?

@Leo10Gama
Copy link
Member Author

Do you plan to update the PR-linter in this PR?

I can update it here, yes

@mergify Mergify
Copy link
Contributor

mergify bot commented Feb 19, 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).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@mergify Mergify
Copy link
Contributor

mergify bot commented Feb 19, 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).

@mergify mergify bot merged commit 6f1aa80 into aws:main Feb 19, 2025
20 checks passed
@github-actions GitHub Actions
Copy link

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 19, 2025
@Leo10Gama Leo10Gama deleted the enumlike-parser branch February 19, 2025 22:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@paulhcsun paulhcsun paulhcsun left review comments

@GavinZZ GavinZZ GavinZZ approved these changes

@aws-cdk-automation aws-cdk-automation aws-cdk-automation left review comments

Assignees

@GavinZZ GavinZZ

Labels
contribution/core This is a PR that came from AWS. p2 pr-linter/analytics-metadata-change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Leo10Gama @aws-cdk-automation @GavinZZ @paulhcsun