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(aws-cdk-lib): use new L1 codegen #26318

Merged
merged 11 commits into from
Jul 13, 2023
Merged

Conversation

mrgrain
Copy link
Contributor

@mrgrain mrgrain commented Jul 11, 2023

Replaces the existing cfn2ts code generator with the new spec2cdk generator based on @cdklabs/typewriter and the separately published service spec database @aws-cdk/aws-service-spec.

Other neccesary changes are:

  • Lowered coverage threshold for branches to 35% since the newly generated code is more expressive. Arguably we should not include generated files in the coverage calculation, but that's a bigger change.
  • Asset hashes in cfn-include test changed due to props now being ordered alphabetic. I've manually verified that the templates are otherwise identical.
  • Removing port property from neptune-alpha as this has been removed upstream and is not functional according to the service team.
  • Switched synthetics-alpha tests to use testDeprecated() for anything using the Canary. This is due to the removal of the deleteLambdaResourcesOnCanaryDeletion prop in the CFN spec. The new codegen marks removed props as deprecated. We will replace this feature with a custom resource soon.

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

@gitpod-io
Copy link

gitpod-io bot commented Jul 11, 2023

@github-actions github-actions bot added the p2 label Jul 11, 2023
@aws-cdk-automation aws-cdk-automation requested a review from a team July 11, 2023 11:08
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Jul 11, 2023
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 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.

@mrgrain mrgrain added pr-linter/exempt-readme The PR linter will not require README changes pr-linter/exempt-test The PR linter will not require test changes pr-linter/exempt-integ-test The PR linter will not require integ test changes labels Jul 11, 2023
@mrgrain
Copy link
Contributor Author

mrgrain commented Jul 11, 2023

Exempting from linter rules since passing all existing tests is proof this works.
No README change required either since this is an internal tool.

@aws-cdk-automation aws-cdk-automation dismissed their stale review July 11, 2023 11:33

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

@mrgrain mrgrain force-pushed the mrgrain/feat/use-new-service-spec branch 3 times, most recently from 665519b to 322f2e2 Compare July 12, 2023 13:09
@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jul 12, 2023
@mergify
Copy link
Contributor

mergify bot commented Jul 12, 2023

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

@mrgrain mrgrain self-assigned this Jul 12, 2023
@mrgrain mrgrain force-pushed the mrgrain/feat/use-new-service-spec branch from 322f2e2 to e1f906b Compare July 12, 2023 15:32
@mergify
Copy link
Contributor

mergify bot commented Jul 12, 2023

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
Copy link
Contributor

mergify bot commented Jul 12, 2023

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: f19ced0
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify
Copy link
Contributor

mergify bot commented Jul 13, 2023

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 f15ed23 into main Jul 13, 2023
@mergify mergify bot deleted the mrgrain/feat/use-new-service-spec branch July 13, 2023 08:35
colifran pushed a commit that referenced this pull request Jul 17, 2023
Replaces the existing `cfn2ts` code generator with the new `spec2cdk` generator based on `@cdklabs/typewriter` and the separately published service spec database `@aws-cdk/aws-service-spec`.

Other neccesary changes are:

- Lowered coverage threshold for branches to 35% since the newly generated code is more expressive. Arguably we should not include generated files in the coverage calculation, but that's a bigger change. 
- Asset hashes in `cfn-include` test changed due to props now being ordered alphabetic. I've manually verified that the templates are otherwise identical.
- Removing `port` property from `neptune-alpha` as this has been removed upstream and is not functional according to the service team.
- Switched `synthetics-alpha` tests to use `testDeprecated()` for anything using the `Canary`. This is due to the removal of the `deleteLambdaResourcesOnCanaryDeletion` prop in the CFN spec. The new codegen marks removed props as deprecated. We will replace this feature with a custom resource soon.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
bmoffatt pushed a commit to bmoffatt/aws-cdk that referenced this pull request Jul 29, 2023
Replaces the existing `cfn2ts` code generator with the new `spec2cdk` generator based on `@cdklabs/typewriter` and the separately published service spec database `@aws-cdk/aws-service-spec`.

Other neccesary changes are:

- Lowered coverage threshold for branches to 35% since the newly generated code is more expressive. Arguably we should not include generated files in the coverage calculation, but that's a bigger change. 
- Asset hashes in `cfn-include` test changed due to props now being ordered alphabetic. I've manually verified that the templates are otherwise identical.
- Removing `port` property from `neptune-alpha` as this has been removed upstream and is not functional according to the service team.
- Switched `synthetics-alpha` tests to use `testDeprecated()` for anything using the `Canary`. This is due to the removal of the `deleteLambdaResourcesOnCanaryDeletion` prop in the CFN spec. The new codegen marks removed props as deprecated. We will replace this feature with a custom resource soon.

----

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

@mrgrain any good reason why removing the port support is a necessity?

We use it and want to be able to customize neptune port, IMHO changing such functionality without backward compatibility damages Netpune credibility and functionality.

@mrgrain
Copy link
Contributor Author

mrgrain commented Oct 23, 2023

Hey @bissli82 Thanks for reaching out! Looks like Neptune now offers a replacement DBPort for this. 🎉
LMK if you want to contribute this back in, otherwise I'll try to get to it this week.

Please also note that aws-cdk/aws-neptune-alpha is currently an experimental module does not guarantee backwards compatibility. However this call-out is entirely reasonable of you!

https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-neptune-dbcluster.html#cfn-neptune-dbcluster-port

image

@bissli82
Copy link

Thanks!
We use alpha since we don't want to work with L1 constructs, and are heavily invested in CDK.
Should we expect dbport available in alpha as well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS. p2 pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. pr-linter/exempt-integ-test The PR linter will not require integ test changes pr-linter/exempt-readme The PR linter will not require README changes pr-linter/exempt-test The PR linter will not require test changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants