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

fix: update default runtime to support CDK < 2.105.0 #497

Merged
merged 1 commit into from
Sep 22, 2024

Conversation

blimmer
Copy link
Contributor

@blimmer blimmer commented Jan 24, 2024

As of version 3.x, this project now uses the Runtime.PROVIDED_AL2023 runtime. However, this enumerated value does not exist in older versions of aws-cdk, prior to [email protected]. If you use a version older than that, you get this cryptic error:

TypeError: Cannot read properties of undefined (reading 'name')
    at new Function (/home/runner/work/cdk-bug-reports/cdk-bug-reports/node_modules/aws-cdk-lib/aws-lambda/lib/function.js:1:11075)
    at SingletonFunction.ensureLambda (/home/runner/work/cdk-bug-reports/cdk-bug-reports/node_modules/aws-cdk-lib/aws-lambda/lib/singleton-lambda.js:1:2886)
    at new SingletonFunction (/home/runner/work/cdk-bug-reports/cdk-bug-reports/node_modules/aws-cdk-lib/aws-lambda/lib/singleton-lambda.js:1:866)
    at new ECRDeployment (/home/runner/work/cdk-bug-reports/cdk-bug-reports/node_modules/cdk-ecr-deployment/src/index.ts:140:20)
    at new TestCdkVersionStack (/home/runner/work/cdk-bug-reports/cdk-bug-reports/lib/test-cdk-version-stack.ts:9:5)
    at Object.<anonymous> (/home/runner/work/cdk-bug-reports/cdk-bug-reports/bin/test-cdk-version.ts:7:1)
    at Module._compile (node:internal/modules/cjs/loader:1376:14)
    at Module.m._compile (/home/runner/work/cdk-bug-reports/cdk-bug-reports/node_modules/ts-node/src/index.ts:1618:23)
    at Module._extensions..js (node:internal/modules/cjs/loader:1435:10)
    at Object.require.extensions.<computed> [as .ts] (/home/runner/work/cdk-bug-reports/cdk-bug-reports/node_modules/ts-node/src/index.ts:1621:12)

Therefore, we're not using the static version, instead new-ing it up: https://github.com/aws/aws-cdk/blob/b49032b3a6e549783b45492ffc76880fbcd58e68/packages/aws-cdk-lib/aws-lambda/lib/runtime.ts#L300-L303

.projenrc.ts Outdated
@@ -26,8 +26,6 @@ const project = new CdklabsConstructLibrary({
'hpagent',
],
deps: [
'aws-cdk-lib@^2.0.0',
'constructs@^10.0.5',
Copy link
Contributor Author

@blimmer blimmer Jan 24, 2024

Choose a reason for hiding this comment

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

You shouldn't need to specify this. It's controlled by the upstream projen config and the peerDependencies it produces. In fact, this caused problems for me because yarn used a newer aws-cdk-lib version than what's specified in your peer deps.

@blimmer blimmer marked this pull request as ready for review January 24, 2024 22:22
@blimmer
Copy link
Contributor Author

blimmer commented Jan 24, 2024

cc @SamStephens @fabiozig @scanlonp , who authored/approved #424

@blimmer blimmer changed the title fix: set minimum CDK version fix: set minimum CDK version to a 2.105.0 for Runtime.PROVIDED_AL2023 Jan 24, 2024
@blimmer
Copy link
Contributor Author

blimmer commented Jan 24, 2024

Alternatively, you could new-up the Runtime yourself to have additional compatibility:

https://github.com/aws/aws-cdk/blob/169fd91e135556b8efb59d631acaf9a3426eaa53/packages/aws-cdk-lib/aws-lambda/lib/runtime.ts#L284-L287

It still probably makes sense to spot-check your code and have a more specific minimum version, still, though.

@fabiozig
Copy link

I also had the same problem and I had to change the version manually in my projects. I also agree with changing the dependency to peerDependencies, and devDependencies only. Even cdk documentation suggests this approach:

For projects that are CDK libraries in NPM, declare them both under the devDependencies and peerDependencies sections. To make sure your library is compatible with the widest range of CDK versions: pick the minimum aws-cdk-lib version that your library requires; declare a range dependency with a caret on that version in peerDependencies, and declare a point version dependency on that version in devDependencies.

https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib-readme.html

Could this be a breaking change in case the dependencies are removed?

@mrgrain
Copy link
Contributor

mrgrain commented Sep 17, 2024

I reckon we should keep everything else as it is and change the default to new Runtime('provided.al2023', RuntimeFamily.OTHER)

auto-merge was automatically disabled September 22, 2024 01:22

Head branch was pushed to by a user without write access

@blimmer blimmer force-pushed the set-minimum-version branch from e1adc15 to fb16893 Compare September 22, 2024 01:22
@blimmer blimmer changed the title fix: set minimum CDK version to a 2.105.0 for Runtime.PROVIDED_AL2023 fix: update default runtime to support CDK < 1.105.0 Sep 22, 2024
@blimmer
Copy link
Contributor Author

blimmer commented Sep 22, 2024

Sounds good @mrgrain - I updated this PR as requested.

@blimmer blimmer force-pushed the set-minimum-version branch from fb16893 to 75195dd Compare September 22, 2024 01:24
@blimmer blimmer changed the title fix: update default runtime to support CDK < 1.105.0 fix: update default runtime to support CDK < 2.105.0 Sep 22, 2024
@mrgrain mrgrain added this pull request to the merge queue Sep 22, 2024
Merged via the queue into cdklabs:main with commit 941015e Sep 22, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants