-
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(custom-resources): use latest SDK in AwsCustomResource #5442
Conversation
Add a `useLatestSdk` prop to `AwsCustomResource`. When set to `true`, the latest v2 of AWS SDK JS will be installed when a new container is initialized for the Lambda function. Subsequent executions resuing this container will skip installation. Increase default timeout to 60 seconds when `useLatestSdk` is set to `true`. Closes aws#2689 Closes aws#5063
Could also be applied for |
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.
Nice touch
* to note that this will apply to **all** `AwsCustomResource`s in the stack | ||
* even if only one instance sets this parameters to `true`. | ||
* | ||
* @default false |
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.
Wondering if this should be the default, because why not?
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.
Agree. If this is the default then the prop should be removed because to disable it it must be set to false
on all AwsCusomResource
?
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.
You mean from backwards compatibility perspective?
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 mean, do we want to offer a way to opt out from this feature because it will not be possible to opt out for some instances of AwsCustomResource
only.
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.
Let's just make this the only behavior (remove the option to opt-out). I don't see value in using an old SDK...
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.
Done, added a fallback to the normal sdk (e.g. NPM unavailable).
Does this need documentation? Do you want to add this to AwsApi
in @aws-cdk/aws-events-targets
? In this PR?
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
@@ -149,7 +149,7 @@ export interface AwsCustomResourceProps { | |||
/** | |||
* The timeout for the Lambda function implementing this custom resource. | |||
* | |||
* @default Duration.seconds(30) | |||
* @default Duration.seconds(60) |
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.
curious what the rationale behind the increase is? Is it only to cover the time it'll take to install the latest SDK or were there any other determining factors?
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.
It's indeed only to cover installation time
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Install the latest v2 of AWS SDK JS when a new container is initialized
for the Lambda function. Subsequent executions reusing this container will
skip installation.
Increase default timeout to 60 seconds.
Closes #2689
Closes #5063
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license