feat(custom-resource): add serviceTimeout property for custom resources#30911
feat(custom-resource): add serviceTimeout property for custom resources#30911mergify[bot] merged 19 commits intoaws:mainfrom
Conversation
xazhao
left a comment
There was a problem hiding this comment.
Left a few minor comments. The code look good to me overall.
| 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.
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.
| /* | ||
| * 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.
Curious how did you verify the ServiceTimeout is set to 60?
There was a problem hiding this comment.
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 |
|
Please prioritize merging this, it's a huge pain point when using Custom Resources in cdk If the custom resource has an unhandled exception, you have no choice but to wait around for an hour |
|
Hello, thanks for pinging us on this issue. I apologize for the delay on the update on this issue. I will sync with Kendra internally and share some update on this PR. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #30911 +/- ##
=======================================
Coverage 66.96% 66.96%
=======================================
Files 329 329
Lines 18663 18667 +4
Branches 3258 3260 +2
=======================================
+ Hits 12497 12501 +4
Misses 5839 5839
Partials 327 327
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Kendra is currently out of office, and I haven’t had a chance to hear back from her yet. I’d like to understand her concerns first before proceeding with the PR review. Thank you for your patience! |
✅ Branch has been successfully updated |
Pull request has been modified.
|
@GavinZZ , any updates on this? |
|
@mirodrr2 this PR is ready to merge. I will approve and get it merged. |
|
@Mergifyio update |
✅ Branch has been successfully updated |
|
@xazhao |
|
Thank you for contributing! Your pull request will be updated from main 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 main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
Comments on closed issues and PRs are hard for our team to see. |
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
serviceTimeoutproperty forCustomResourceandAwsCustomResource.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