-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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-resource): add serviceTimeout property for custom resources #30911
base: main
Are you sure you want to change the base?
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.
Left a few minor comments. The code look good to me overall.
@@ -55,6 +56,13 @@ export interface CustomResourceProps { | |||
*/ | |||
readonly serviceToken: string; | |||
|
|||
/** | |||
* The maximum time, in seconds, that can elapse before a custom resource operation times out |
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.
Do we need to mention the unit is seconds
here? Since the type is Duration
, customers should be able to use different units in Duration
.
Also might be good to mention the range is 1 to 3600 seconds here.
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.
@xazhao
Thanks.
I've updated docs.
/* | ||
* Stack verification steps: | ||
* - Deploy with `--no-clean` | ||
* - Verify that `ServiceTimeout` is set to 60 in the CloudWatch Logs for the Lambda function that creates custom resources. |
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 how did you verify the ServiceTimeout
is set to 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.
Nvm I see you explained it in the PR description.
df369dc
to
d29130b
Compare
I don't think we can accept this PR at this time, not because of anything wrong with your code but because of problems with AwsCustomResource. I'm going to put the do-not-close and do-not-merge label on this one while we work to assess whether this is actually safe to accept and/or resolve the other issues at play here. |
Ran into wanting this feature (currently waiting for things to timeout hah), what's the concern with |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Issue # (if applicable)
Closes #30517.
Reason for this change
L2 construct does not support setting
serviceTimeout
.Enabling customizable timeouts is useful when using custom resources, as the current default timeout is set to 3600 seconds.
Ref: AWS CloudFormation accelerates dev-test cycle with adjustable timeouts for custom resources
Description of changes
Add
serviceTimeout
property forCustomResource
andAwsCustomResource
.Description of how you validated changes
Add unit tests and integ tests.
Additionally, I confirmed that ServiceTimeout is set by checking the CloudWatch Logs of the Lambda function that generates custom resources.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license