feat(stepfunctions-tasks): add `timeout` parameter for EmrCreateCluster #28532

go-to-k · 2023-12-30T14:02:52Z

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: #28529 (comment)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

aws-cdk-automation

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.

go-to-k · 2023-12-30T14:06:30Z

Exemption Request: Since there is no change in the CloudFormation template for integ testing, no change can be created in the snapshot.

kaizencc

Some minor thoughts, thanks for this!

kaizencc · 2023-12-30T21:31:33Z

packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/emr/emr-create-cluster.ts

-     * The value must be between 5 and 1440.
+     * The value must be between 5 and 1440 minutes.
+     *
+     * @default - No timeoutDurationMinutes


The default should be to use the value in timeout.

kaizencc · 2023-12-30T21:31:46Z

packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/emr/emr-create-cluster.ts

+     *
+     * You must specify one of `timeout` and `timeoutDurationMinutes`.
+     *
+     * @default - No timeout


The default should be that we use the value in timeoutDurationMinutes

kaizencc · 2023-12-30T21:35:57Z

packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/emr/private/cluster-utils.ts

-  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) {


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

go-to-k · 2023-12-31T04:40:27Z

@kaizencc

Thanks for your review, I changed.

packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/emr/private/cluster-utils.ts

packages/aws-cdk-lib/aws-stepfunctions-tasks/test/emr/emr-create-cluster.test.ts

kaizencc

Hopefully my little update doesn't break the build :). Thanks @go-to-k

mergify · 2024-01-02T11:42:20Z

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-cdk-automation · 2024-01-02T12:08:31Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 5b49248
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2024-01-02T12:08:54Z

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*

feat(stepfunctions-tasks): add parameter as Duration type

ed0bb6e

aws-cdk-automation requested a review from a team December 30, 2023 14:03

github-actions bot added distinguished-contributor [Pilot] contributed 50+ PRs to the CDK p2 labels Dec 30, 2023

go-to-k mentioned this pull request Dec 30, 2023

feat(stepfunctions-tasks): add validations for EmrCreateCluster #28529

Merged

go-to-k changed the title ~~feat(stepfunctions-tasks): add parameter as Duration type~~ feat(stepfunctions-tasks): add parameter timeout as Duration type Dec 30, 2023

aws-cdk-automation previously requested changes Dec 30, 2023

View reviewed changes

go-to-k changed the title ~~feat(stepfunctions-tasks): add parameter timeout as Duration type~~ feat(stepfunctions-tasks): add new parameter timeout as Duration type Dec 30, 2023

go-to-k changed the title ~~feat(stepfunctions-tasks): add new parameter timeout as Duration type~~ feat(stepfunctions-tasks): add parameter timeout as Duration type Dec 30, 2023

go-to-k changed the title ~~feat(stepfunctions-tasks): add parameter timeout as Duration type~~ feat(stepfunctions-tasks): add timeout parameter as Duration type Dec 30, 2023

aws-cdk-automation added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Dec 30, 2023

go-to-k changed the title ~~feat(stepfunctions-tasks): add timeout parameter as Duration type~~ feat(stepfunctions-tasks): add timeout parameter as Duration type for EmrCreateCluster Dec 30, 2023

go-to-k changed the title ~~feat(stepfunctions-tasks): add timeout parameter as Duration type for EmrCreateCluster~~ feat(stepfunctions-tasks): add timeout parameter for EmrCreateCluster Dec 30, 2023

go-to-k added 2 commits December 30, 2023 23:10

Merge branch 'main' into feat/sfn-tasks-emr-new-timeout

0e25bf3

change docs

e06ab39

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Dec 30, 2023

go-to-k added 2 commits December 30, 2023 23:51

change branch condition

f3840cc

change unit tests

45b4a1b

kaizencc previously requested changes Dec 30, 2023

View reviewed changes

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Dec 30, 2023

kaizencc added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Dec 30, 2023

change by review

ad29524

tweak

go-to-k force-pushed the feat/sfn-tasks-emr-new-timeout branch from 68ba2c6 to ad29524 Compare December 31, 2023 04:04

Merge branch 'main' into feat/sfn-tasks-emr-new-timeout

c1b020b

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Dec 31, 2023

go-to-k requested a review from kaizencc December 31, 2023 04:40

github-actions bot mentioned this pull request Jan 1, 2024

Monthly PR metrics report - Dec 2023 #28544

Closed

kaizencc reviewed Jan 2, 2024

View reviewed changes

kaizencc added 2 commits January 2, 2024 12:15

Apply suggestions from code review

b42f6c8

Merge branch 'main' into feat/sfn-tasks-emr-new-timeout

6a8c4ae

kaizencc approved these changes Jan 2, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jan 2, 2024

Merge branch 'main' into feat/sfn-tasks-emr-new-timeout

5b49248

mergify bot merged commit ca91626 into aws:main Jan 2, 2024
9 checks passed

go-to-k deleted the feat/sfn-tasks-emr-new-timeout branch January 2, 2024 12:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(stepfunctions-tasks): add `timeout` parameter for EmrCreateCluster #28532

feat(stepfunctions-tasks): add `timeout` parameter for EmrCreateCluster #28532

go-to-k commented Dec 30, 2023 •

edited

aws-cdk-automation left a comment

go-to-k commented Dec 30, 2023

kaizencc left a comment

kaizencc Dec 30, 2023

kaizencc Dec 30, 2023

kaizencc Dec 30, 2023

go-to-k commented Dec 31, 2023

kaizencc left a comment

mergify bot commented Jan 2, 2024

aws-cdk-automation commented Jan 2, 2024

mergify bot commented Jan 2, 2024

feat(stepfunctions-tasks): add timeout parameter for EmrCreateCluster #28532

feat(stepfunctions-tasks): add timeout parameter for EmrCreateCluster #28532

Conversation

go-to-k commented Dec 30, 2023 • edited

aws-cdk-automation left a comment

Choose a reason for hiding this comment

go-to-k commented Dec 30, 2023

kaizencc left a comment

Choose a reason for hiding this comment

kaizencc Dec 30, 2023

Choose a reason for hiding this comment

kaizencc Dec 30, 2023

Choose a reason for hiding this comment

kaizencc Dec 30, 2023

Choose a reason for hiding this comment

go-to-k commented Dec 31, 2023

kaizencc left a comment

Choose a reason for hiding this comment

mergify bot commented Jan 2, 2024

aws-cdk-automation commented Jan 2, 2024

AWS CodeBuild CI Report

mergify bot commented Jan 2, 2024

feat(stepfunctions-tasks): add `timeout` parameter for EmrCreateCluster #28532

feat(stepfunctions-tasks): add `timeout` parameter for EmrCreateCluster #28532

go-to-k commented Dec 30, 2023 •

edited