feat(stepfunctions-tasks): add timeout parameter for EmrCreateCluster#28532
feat(stepfunctions-tasks): add timeout parameter for EmrCreateCluster#28532mergify[bot] merged 10 commits intoaws:mainfrom
timeout parameter for EmrCreateCluster#28532Conversation
timeout as Duration type
aws-cdk-automation
left a comment
There was a problem hiding this comment.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.
timeout as Duration typetimeout as Duration type
timeout as Duration typetimeout as Duration type
timeout as Duration typetimeout parameter as Duration type
|
Exemption Request: Since there is no change in the CloudFormation template for integ testing, no change can be created in the snapshot. |
timeout parameter as Duration typetimeout parameter as Duration type for EmrCreateCluster
timeout parameter as Duration type for EmrCreateClustertimeout parameter for EmrCreateCluster
kaizencc
left a comment
There was a problem hiding this comment.
Some minor thoughts, thanks for this!
| * The value must be between 5 and 1440. | ||
| * The value must be between 5 and 1440 minutes. | ||
| * | ||
| * @default - No timeoutDurationMinutes |
There was a problem hiding this comment.
The default should be to use the value in timeout.
| * | ||
| * You must specify one of `timeout` and `timeoutDurationMinutes`. | ||
| * | ||
| * @default - No timeout |
There was a problem hiding this comment.
The default should be that we use the value in timeoutDurationMinutes
| if (!cdk.Token.isUnresolved(property.timeoutDurationMinutes) && (property.timeoutDurationMinutes < 5 || property.timeoutDurationMinutes > 1440)) { | ||
| throw new Error(`timeoutDurationMinutes must be between 5 and 1440, got ${property.timeoutDurationMinutes}`); | ||
|
|
||
| if (!property.timeout && !property.timeoutDurationMinutes) { |
There was a problem hiding this comment.
I don't like treating timeout and timeoutDurationMinutes separately here, because that will cause difficulty in the future if we are to add more validation. Instead, I'm looking for:
if ((property.timeout && property.timeoutDurationMinutes) || (!property.timeout && !property.timeoutDurationMinutes)) { throw new Error() }
const timeout = property.timeout.toMinutes() ?? property.timeoutDurationMinutes;
if (!cdk.Token.isUnresolved(timeout) && timeout < 5 || timeout > 1440) { ... }✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
Pull request has been modified.
tweak
68ba2c6 to
ad29524
Compare
|
Thanks for your review, I changed. |
packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/emr/private/cluster-utils.ts
Outdated
Show resolved
Hide resolved
packages/aws-cdk-lib/aws-stepfunctions-tasks/test/emr/emr-create-cluster.test.ts
Outdated
Show resolved
Hide resolved
packages/aws-cdk-lib/aws-stepfunctions-tasks/test/emr/emr-create-cluster.test.ts
Outdated
Show resolved
Hide resolved
packages/aws-cdk-lib/aws-stepfunctions-tasks/test/emr/emr-create-cluster.test.ts
Outdated
Show resolved
Hide resolved
|
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). |
…er (aws#28532) This PR adds a new parameter `timeout` as Duration type instead of `timeoutDurationMinutes` because the `timeoutDurationMinutes` is a number type. Originally, `timeoutDurationMinutes` was a **required** parameter, but we have made it **optional** and also made the new parameter **optional** to avoid breaking change. Instead, added a validation to ensure that the value is specified. We discussed this in the following thread: aws#28529 (comment) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
This PR adds a new parameter
timeoutas Duration type instead oftimeoutDurationMinutesbecause thetimeoutDurationMinutesis a number type.Originally,
timeoutDurationMinuteswas a required parameter, but we have made it optional and also made the new parameter optional to avoid breaking change.Instead, added a validation to ensure that the value is specified.
We discussed this in the following thread: #28529 (comment)
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license