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

chore(lambda): upgrade default runtime for custom resource provider and function to nodejs_20 #29681

Closed
wants to merge 90 commits into from

Conversation

shikha372
Copy link
Contributor

@shikha372 shikha372 commented Apr 1, 2024

Issue # (if applicable)

Closes: #28125
Tracking Ticket: #29786

Reason for this change

Keeping dependencies up to date with latest release in custom resource provider function and lambda default runtime value for Nodejs

Description of changes

Update default runtime defined in HandlerFrameworkModule to update custom resource provider
Update runtime for custom resource provider still using CfnResource.
Update for providers in alpha modules.
Unit Test updated with expected runtime value
Update to expected runtime value in integ test along with snapshots.

Description of how you validated changes

Ran Integration Tests(framework-integ) to validate custom resources behaviour after the upgrade.

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 Apr 1, 2024
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Apr 1, 2024
@shikha372 shikha372 changed the title upgrade default runtime for custom resources to nodejs20 chore: (custom-resource/lambda)Upgrade default runtime for provider and function to nodejs_20 Apr 1, 2024
@aws-cdk-automation aws-cdk-automation requested a review from a team April 1, 2024 20:47
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.

Curious how do you confirm that we didn't miss anywhere?

@shikha372
Copy link
Contributor Author

shikha372 commented Apr 1, 2024

how do you confirm that we didn't miss anywhere?

ran yarn build and yarn test on local under aws-cdk-lib and @aws-cdk/custom-resource-handlers to confirm that unit tests are passing, changes to the unit tests are confirmed on the basis of that.
Also, we verify the generated providers created under aws-cdk-lib and aws-cdk dist folder on running build, these generated handlers have nodejs20 as their runtime providers.
example path: ~/aws-cdk/packages/@aws-cdk/custom-resource-handlers/dist/aws-cloudfront/cross-region-string-param-reader-provider.generated.ts
Second verification will be with the below command under each of this package to confirm nodejs18 is mentioned only in expected places
grep -irl 'nodejs18' or grep -irl 'NODEJS_18_X'

Copy link
Contributor

@colifran colifran left a comment

Choose a reason for hiding this comment

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

Looks good so far! You may be working on it but this should be included too -

@shikha372
Copy link
Contributor Author

shikha372 commented Apr 3, 2024

Looks good so far! You may be working on it but this should be included too -

Added in latest commit

Copy link
Contributor

@TheRealAmazonKendra TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

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

Just a couple of comments but overall I think this is looking good so far. One quick question I have is what the scope of this task is intended to be. We have some testing infra that is likely hardcoded to node18 (not meaning the framework integ tests that I had asked to be split out, other places). So, fi this change is meant to be exhaustive for the repo, some things may have been missed.

@shikha372

This comment was marked as resolved.

Copy link
Contributor

@TheRealAmazonKendra TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

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

Just one quick comment

Copy link
Contributor

@TheRealAmazonKendra TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

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

As long as all the integ tests pass, we should be good.

@shikha372 shikha372 changed the title chore: (custom-resource/lambda)Upgrade default runtime for provider and function to nodejs_20 chore(lambda): Upgrade default runtime for custom resource provider and function to nodejs_20 Apr 10, 2024
aws-cdk-automation

This comment was marked as resolved.

@TheRealAmazonKendra
Copy link
Contributor

Well, I've got some unfortunate news, I was searching for something unrelated to this change and discovered that node20 isn't available in GovCloud, ADC, or China regions so in places where DEFAULT was specifying node18 and anywhere that the version is not configurable to the user still needs to be that.

@colifran
Copy link
Contributor

colifran commented Apr 17, 2024

@TheRealAmazonKendra Given that news it seems like moving forward here doesn't make sense until node20 is available in GovCloud, ADC, and China regions.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@aws-cdk-automation aws-cdk-automation added the pr/do-not-merge This PR should not be merged at this time. label Apr 19, 2024
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.

Updating this to requesting changes as Node20 ins not available in all partitions.

@aws-cdk-automation aws-cdk-automation dismissed their stale review April 19, 2024 19:59

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

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.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

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/do-not-merge This PR should not be merged at this time.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aws-cdk-lib: NODEJS_LATEST still points to nodejs18.x
6 participants