-
Notifications
You must be signed in to change notification settings - Fork 4.1k
feat(lambda): add LATEST to ParamsAndSecretsVersions #34367
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
base: main
Are you sure you want to change the base?
feat(lambda): add LATEST to ParamsAndSecretsVersions #34367
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(This review is outdated)
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution! I've just added some minor modification to approve.
You're defining the latest correspondence relationships in fact-tables.ts
, but to keep it up-to-date, we need to continuously update this file, which seems like it could result in breaking changes. I'd like you to discuss with the maintainers whether this approach is acceptable or not.
}, | ||
}, | ||
}); | ||
expect(() => app.synth()).not.toThrow(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expect(() => app.synth()).not.toThrow(); |
}, | ||
}, | ||
}); | ||
expect(() => app.synth()).not.toThrow(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expect(() => app.synth()).not.toThrow(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the review.
I fixed the issues you pointed out in the following commit:
bc421c0
And, as you mentioned, fact-tables.ts
will need to be continuously updated.
However, I couldn't come up with a better solution than this to add LATEST to ParamsAndSecretsVersions.
Although I'd like to discuss this with the maintainers, I'm not sure who they are.
If you know, could you let me know who I should reach out to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the console UI, you can only select a fixed AWS-Parameters-and-Secrets-Lambda-Extension
version of the layer — there's no option to choose something like "latest" to always fetch the newest version.
For that reason, I believe the CDK should align with this behavior. Since the concept of a dynamically updated latest version isn't supported in the Lambda console, implementing such behavior in CDK is also challenging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't come up with a better solution
I also have no better idea!
If you know, could you let me know who I should reach out to?
I'm not sure too. After my approval, the needs-maintainer-review
label would be attached and one of maintainers will be assigned. Therefore, please wait until that time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@badmintoncryer
I fix through this commit
050292a
ed1ab42
to
bc421c0
Compare
@okasyun Could you please re-run your integ test? @aws-cdk-testing/framework-integ: Snapshot Results:
@aws-cdk-testing/framework-integ: Tests: 1 failed, 1186 total
@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src1098740116/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/aws-lambda/test/integ.params-and-secrets.js
@aws-cdk-testing/framework-integ: Error: Some tests failed!
@aws-cdk-testing/framework-integ: To re-run failed tests run: integ-runner --update-on-failed
@aws-cdk-testing/framework-integ: at main (/codebuild/output/src1098740116/src/github.com/aws/aws-cdk/node_modules/@aws-cdk/integ-runner/lib/index.js:10279:13)
@aws-cdk-testing/framework-integ: Error: integ-runner exited with error code 1
@aws-cdk-testing/framework-integ: Tests failed. Total time (2m19.0s) | integ-runner (2m8.0s) | /codebuild/output/src1098740116/src/github.com/aws/aws-cdk/node_modules/jest/bin/jest.js (9.9s)
@aws-cdk-testing/framework-integ: !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! |
@okasyun Could you please follow below steps?
|
…ion to latest versions
8020755
to
4528bc8
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution.
I made a couple of suggestions (took some inspiration from #32783)
V1_0_103 = '1.0.103', | ||
/** | ||
* latest version | ||
* | ||
*/ | ||
LATEST = 'latest', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
V1_0_103 = '1.0.103', | |
/** | |
* latest version | |
* | |
*/ | |
LATEST = 'latest', | |
V1_0_103 = '1.0.103', | |
/** | |
* Version 1.0.104 | |
*/ | |
V1_0_104 = '1.0.104', | |
/** | |
* latest version | |
*/ | |
LATEST = '1.0.104', |
@@ -1243,6 +1243,84 @@ export const PARAMS_AND_SECRETS_LAMBDA_LAYER_ARNS: { [version: string]: { [arch: | |||
'us-west-2': 'arn:aws:lambda:us-west-2:345057560386:layer:AWS-Parameters-and-Secrets-Lambda-Extension-Arm64:4', | |||
}, | |||
}, | |||
'latest': { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest to keep the version logic and to keep the new entry in 1st position:
'latest': { | |
'1.0.104': { |
Issue # (if applicable)
Closes #31772.
Reason for this change
calling
will always return the lambda layer with a version of 4 for us-east-1. the CDK documentation states that this is the latest version, while the documentation in lambda shows that 16 is the latest in this region.
Not only us-east-1 but also other resions have the same problems.
Description of changes
add property
latest
to ParamsAndSecretsVersions.Parameters and Secrets Lambda Extension ARN can be referenced by using this
latest
.Description of how you validated changes
I created unit tests and integration tests.
Both were successful.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license