change by review

aws · Dec 31, 2023 · 68ba2c6 · 68ba2c6
1 parent 45b4a1b
commit 68ba2c6
Show file tree

Hide file tree

Showing 3 changed files with 14 additions and 58 deletions.
diff --git a/packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/emr/emr-create-cluster.ts b/packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/emr/emr-create-cluster.ts
@@ -734,7 +734,7 @@ export namespace EmrCreateCluster {
      *
      * The value must be between 5 and 1440 minutes.
      *
-     * @default - No timeoutDurationMinutes
+     * @default - the value in `timeout` is used
      *
      * @deprecated - Use `timeout`.
      */
@@ -747,7 +747,7 @@ export namespace EmrCreateCluster {
      *
      * You must specify one of `timeout` and `timeoutDurationMinutes`.
      *
-     * @default - No timeout
+     * @default - the value in `timeoutDurationMinutes` is used
      */
     readonly timeout?: cdk.Duration;
   }

diff --git a/packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/emr/private/cluster-utils.ts b/packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/emr/private/cluster-utils.ts
@@ -151,19 +151,12 @@ function SpotProvisioningSpecificationPropertyToJson(property?: EmrCreateCluster
     return undefined;
   }
 
-  if (!property.timeout && !property.timeoutDurationMinutes) {
-    throw new Error('timeout must be specified');
+  if ((property.timeout && property.timeoutDurationMinutes) || (!property.timeout && !property.timeoutDurationMinutes)) {
+    throw new Error('one of timeout and timeoutDurationMinutes must be specified');
   }
-  if (property.timeout && (property.timeout.toMinutes() < 5 || property.timeout.toMinutes() > 1440)) {
-    throw new Error(`timeout must be between 5 and 1440 minutes, got ${property.timeout.toMinutes()}`);
-  }
-  if (
-    !property.timeout // ignore validation because `timeoutDurationMinutes` is not used if a `timeout` is specified
-    && property.timeoutDurationMinutes
-    && !cdk.Token.isUnresolved(property.timeoutDurationMinutes)
-    && (property.timeoutDurationMinutes < 5 || property.timeoutDurationMinutes > 1440)
-  ) {
-    throw new Error(`timeoutDurationMinutes must be between 5 and 1440 minutes, got ${property.timeoutDurationMinutes}`);
+  const timeout = property.timeout?.toMinutes() ?? property.timeoutDurationMinutes;
+  if (timeout !== undefined && !cdk.Token.isUnresolved(timeout) && (timeout < 5 || timeout > 1440)) {
+    throw new Error(`timeout must be between 5 and 1440 minutes, got ${timeout}`);
   }
 
   return {

diff --git a/packages/aws-cdk-lib/aws-stepfunctions-tasks/test/emr/emr-create-cluster.test.ts b/packages/aws-cdk-lib/aws-stepfunctions-tasks/test/emr/emr-create-cluster.test.ts
@@ -1300,7 +1300,7 @@ test('Throws if timeoutDurationMinutes for Spot instances is less than 5 minutes
   // THEN
   expect(() => {
     task.toStateJson();
-  }).toThrow(/timeoutDurationMinutes must be between 5 and 1440 minutes, got 4/);
+  }).toThrow(/timeout must be between 5 and 1440 minutes, got 4/);
 });
 
 test('Throws if timeoutDurationMinutes for Spot instances is greater than 1440 minutes', () => {
@@ -1328,7 +1328,7 @@ test('Throws if timeoutDurationMinutes for Spot instances is greater than 1440 m
   // THEN
   expect(() => {
     task.toStateJson();
-  }).toThrow(/timeoutDurationMinutes must be between 5 and 1440 minutes, got 1441/);
+  }).toThrow(/timeout must be between 5 and 1440 minutes, got 1441/);
 });
 
 test('Throws if neither timeout nor timeoutDurationMinutes is specified', () => {
@@ -1355,10 +1355,10 @@ test('Throws if neither timeout nor timeoutDurationMinutes is specified', () =>
   // THEN
   expect(() => {
     task.toStateJson();
-  }).toThrow(/timeout must be specified/);
+  }).toThrow(/one of timeout and timeoutDurationMinutes must be specified/);
 });
 
-test('timeout takes precedence if both timeout and timeoutDurationMinutes are specified', () => {
+test('Throws if both timeout and timeoutDurationMinutes are specified', () => {
   // WHEN
   const task = new EmrCreateCluster(stack, 'Task', {
     instances: {
@@ -1382,46 +1382,9 @@ test('timeout takes precedence if both timeout and timeoutDurationMinutes are sp
   });
 
   // THEN
-  expect(stack.resolve(task.toStateJson())).toEqual({
-    Type: 'Task',
-    Resource: {
-      'Fn::Join': [
-        '',
-        [
-          'arn:',
-          {
-            Ref: 'AWS::Partition',
-          },
-          ':states:::elasticmapreduce:createCluster',
-        ],
-      ],
-    },
-    End: true,
-    Parameters: {
-      Name: 'Cluster',
-      Instances: {
-        KeepJobFlowAliveWhenNoSteps: true,
-        InstanceFleets: [{
-          InstanceFleetType: 'MASTER',
-          LaunchSpecifications: {
-            SpotSpecification: {
-              TimeoutAction: 'TERMINATE_CLUSTER',
-              TimeoutDurationMinutes: 5,
-            },
-          },
-          Name: 'Main',
-          TargetSpotCapacity: 1,
-        }],
-      },
-      VisibleToAllUsers: true,
-      JobFlowRole: {
-        Ref: 'ClusterRoleD9CA7471',
-      },
-      ServiceRole: {
-        Ref: 'ServiceRole4288B192',
-      },
-    },
-  });
+  expect(() => {
+    task.toStateJson();
+  }).toThrow(/one of timeout and timeoutDurationMinutes must be specified/);
 });
 
 test('Throws if both bidPrice and bidPriceAsPercentageOfOnDemandPrice are specified', () => {