-
Notifications
You must be signed in to change notification settings - Fork 4k
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(lambda): add cloudwatch lambda insights arm support #17665
Conversation
Adding builtin support for the new ARM64 CloudWatch insights Lambda layers which were [announced](https://aws.amazon.com/about-aws/whats-new/2021/11/amazon-cloudwatch-lambda-insights-functions-graviton2/) yesterday.
if (!versionExists) { | ||
throw new Error(`Insights version ${insightsVersion} does not exist.`); | ||
} | ||
|
||
class InsightsVersion extends LambdaInsightsVersion { | ||
public readonly layerVersionArn = Lazy.uncachedString({ |
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.
Not sure if it ok to just remove this? It will only be correct if you are using x86. Also not sure under what circumstances you would want to use this property. From what I can tell it is only used internally.
also fixes #17133
/** | ||
* The system architectures compatible with this lambda function. | ||
*/ | ||
readonly architecture?: Architecture; |
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.
Can't we default this to X86
?
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 past it looks like we decided to intentionally not default this. See this comment and this comment
@@ -717,7 +719,7 @@ export class Function extends FunctionBase { | |||
kmsKeyArn: props.environmentEncryption?.keyArn, | |||
fileSystemConfigs, | |||
codeSigningConfigArn: props.codeSigningConfig?.codeSigningConfigArn, | |||
architectures: architecture ? [architecture.name] : undefined, | |||
architectures: this._architecture ? [this._architecture.name] : undefined, |
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.
Ruh roh. Looks like we'll be able to have more than one?
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.
CloudFormation for some reason accepts a list with a max length of 1. We originally accepted a list, but then deprecated that in favor of a single value #16849.
Conditional approval after minor changes. |
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Adding builtin support for the new ARM64 CloudWatch insights Lambda layers which were [announced](https://aws.amazon.com/about-aws/whats-new/2021/11/amazon-cloudwatch-lambda-insights-functions-graviton2/) yesterday. also fixes aws#17133 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Adding builtin support for the new ARM64 CloudWatch insights Lambda
layers which were announced
yesterday.
also fixes #17133
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license