refactor(events/appscaling): make Schedule.rate take a Duration (a…

…ws#2966) Use our shiny new ability to calculate with time to specify cron rates.
jonife · Jun 21, 2019 · c221181 · c221181
1 parent 2d50c18
commit c221181
Show file tree

Hide file tree

Showing 19 changed files with 130 additions and 73 deletions.
diff --git a/packages/@aws-cdk/aws-applicationautoscaling/lib/schedule.ts b/packages/@aws-cdk/aws-applicationautoscaling/lib/schedule.ts
@@ -1,3 +1,5 @@
+import { Duration } from '@aws-cdk/cdk';
+
 /**
  * Schedule for scheduled scaling actions
  */
@@ -14,10 +16,15 @@ export abstract class Schedule {
   /**
    * Construct a schedule from an interval and a time unit
    */
-  public static rate(interval: number, unit: TimeUnit): Schedule {
-    const unitStr = interval !== 1 ? `${unit}s` : unit;
+  public static rate(duration: Duration): Schedule {
+    if (duration.toSeconds() === 0) {
+      throw new Error('Duration cannot be 0');
+    }
 
-    return new LiteralSchedule(`rate(${interval} ${unitStr})`);
+    let rate = maybeRate(duration.toDays({ integral: false }), 'day');
+    if (rate === undefined) { rate = maybeRate(duration.toHours({ integral: false }), 'hour'); }
+    if (rate === undefined) { rate = makeRate(duration.toMinutes({ integral: true }), 'minute'); }
+    return new LiteralSchedule(rate);
   }
 
   /**
@@ -56,26 +63,6 @@ export abstract class Schedule {
   }
 }
 
-/**
- * What unit to interpret the rate in
- */
-export enum TimeUnit {
-  /**
-   * The rate is in minutes
-   */
-  Minute = 'minute',
-
-  /**
-   * The rate is in hours
-   */
-  Hour = 'hour',
-
-  /**
-   * The rate is in days
-   */
-  Day = 'day'
-}
-
 /**
  * Options to configure a cron expression
  *
@@ -154,4 +141,19 @@ function formatISO(date?: Date) {
     }
     return num;
   }
+}
+
+/**
+ * Return the rate if the rate is whole number
+ */
+function maybeRate(interval: number, singular: string) {
+  if (interval === 0 || !Number.isInteger(interval)) { return undefined; }
+  return makeRate(interval, singular);
+}
+
+/**
+ * Return 'rate(${interval} ${singular}(s))` for the interval
+ */
+function makeRate(interval: number, singular: string) {
+  return interval === 1 ? `rate(1 ${singular})` : `rate(${interval} ${singular}s)`;
 }
diff --git a/packages/@aws-cdk/aws-applicationautoscaling/test/test.cron.ts b/packages/@aws-cdk/aws-applicationautoscaling/test/test.cron.ts
@@ -1,3 +1,4 @@
+import { Duration } from '@aws-cdk/cdk';
 import { Test } from 'nodeunit';
 import appscaling = require('../lib');
 
@@ -10,5 +11,19 @@ export = {
   'test utc cron, hour and minute'(test: Test) {
     test.equals(appscaling.Schedule.cron({ hour: '18', minute: '24' }).expressionString, 'cron(24 18 * * ? *)');
     test.done();
-  }
+  },
+
+  'rate must be whole number of minutes'(test: Test) {
+    test.throws(() => {
+      appscaling.Schedule.rate(Duration.seconds(12345));
+    }, /'12345 seconds' cannot be converted into a whole number of minutes/);
+    test.done();
+  },
+
+  'rate cannot be 0'(test: Test) {
+    test.throws(() => {
+      appscaling.Schedule.rate(Duration.days(0));
+    }, /Duration cannot be 0/);
+    test.done();
+  },
 };
diff --git a/packages/@aws-cdk/aws-applicationautoscaling/test/test.scalable-target.ts b/packages/@aws-cdk/aws-applicationautoscaling/test/test.scalable-target.ts
@@ -1,5 +1,6 @@
 import { expect, haveResource } from '@aws-cdk/assert';
 import cdk = require('@aws-cdk/cdk');
+import { Duration } from '@aws-cdk/cdk';
 import { Test } from 'nodeunit';
 import appscaling = require('../lib');
 import { createScalableTarget } from './util';
@@ -37,7 +38,7 @@ export = {
 
     // WHEN
     target.scaleOnSchedule('ScaleUp', {
-      schedule: appscaling.Schedule.rate(1, appscaling.TimeUnit.Minute),
+      schedule: appscaling.Schedule.rate(Duration.minutes(1)),
       maxCapacity: 50,
       minCapacity: 1,
     });

diff --git a/packages/@aws-cdk/aws-ecs-patterns/test/ec2/integ.scheduled-ecs-task.lit.ts b/packages/@aws-cdk/aws-ecs-patterns/test/ec2/integ.scheduled-ecs-task.lit.ts
@@ -27,7 +27,7 @@ class EventStack extends cdk.Stack {
       memoryLimitMiB: 512,
       cpu: 1,
       environment: { name: 'TRIGGER', value: 'CloudWatch Events' },
-      schedule: events.Schedule.rate(1, events.TimeUnit.Minute),
+      schedule: events.Schedule.rate(cdk.Duration.minutes(1)),
     });
     /// !hide
   }

diff --git a/packages/@aws-cdk/aws-events-targets/test/ecs/integ.event-ec2-task.lit.ts b/packages/@aws-cdk/aws-events-targets/test/ecs/integ.event-ec2-task.lit.ts
@@ -30,7 +30,7 @@ class EventStack extends cdk.Stack {
 
     // An Rule that describes the event trigger (in this case a scheduled run)
     const rule = new events.Rule(this, 'Rule', {
-      schedule: events.Schedule.rate(1, events.TimeUnit.Minute),
+      schedule: events.Schedule.rate(cdk.Duration.minutes(1)),
     });
 
     // Use EcsTask as the target of the Rule

diff --git a/packages/@aws-cdk/aws-events-targets/test/ecs/integ.event-fargate-task.ts b/packages/@aws-cdk/aws-events-targets/test/ecs/integ.event-fargate-task.ts
@@ -26,7 +26,7 @@ class EventStack extends cdk.Stack {
 
     // A rule that describes the event trigger (in this case a scheduled run)
     const rule = new events.Rule(this, 'Rule', {
-      schedule: events.Schedule.rate(1, events.TimeUnit.Minute),
+      schedule: events.Schedule.rate(cdk.Duration.minutes(1)),
     });
 
     // Use EcsTask as the target of the Rule

diff --git a/packages/@aws-cdk/aws-events-targets/test/lambda/integ.events.ts b/packages/@aws-cdk/aws-events-targets/test/lambda/integ.events.ts
@@ -14,12 +14,12 @@ const fn = new lambda.Function(stack, 'MyFunc', {
 });
 
 const timer = new events.Rule(stack, 'Timer', {
-  schedule: events.Schedule.rate(1, events.TimeUnit.Minute),
+  schedule: events.Schedule.rate(cdk.Duration.minutes(1)),
 });
 timer.addTarget(new targets.LambdaFunction(fn));
 
 const timer2 = new events.Rule(stack, 'Timer2', {
-  schedule: events.Schedule.rate(2, events.TimeUnit.Minute),
+  schedule: events.Schedule.rate(cdk.Duration.minutes(2)),
 });
 timer2.addTarget(new targets.LambdaFunction(fn));
 

diff --git a/packages/@aws-cdk/aws-events-targets/test/lambda/lambda.test.ts b/packages/@aws-cdk/aws-events-targets/test/lambda/lambda.test.ts
@@ -9,10 +9,10 @@ test('use lambda as an event rule target', () => {
   const stack = new cdk.Stack();
   const fn = newTestLambda(stack);
   const rule1 = new events.Rule(stack, 'Rule', {
-    schedule: events.Schedule.rate(1, events.TimeUnit.Minute),
+    schedule: events.Schedule.rate(cdk.Duration.minutes(1)),
   });
   const rule2 = new events.Rule(stack, 'Rule2', {
-    schedule: events.Schedule.rate(5, events.TimeUnit.Minute),
+    schedule: events.Schedule.rate(cdk.Duration.minutes(5)),
   });
 
   // WHEN

diff --git a/packages/@aws-cdk/aws-events-targets/test/sns/integ.sns-event-rule-target.ts b/packages/@aws-cdk/aws-events-targets/test/sns/integ.sns-event-rule-target.ts
@@ -16,7 +16,7 @@ const stack = new cdk.Stack(app, 'aws-cdk-sns-event-target');
 
 const topic = new sns.Topic(stack, 'MyTopic');
 const event = new events.Rule(stack, 'EveryMinute', {
-  schedule: events.Schedule.rate(1, events.TimeUnit.Minute),
+  schedule: events.Schedule.rate(cdk.Duration.minutes(1)),
 });
 
 const queue = new sqs.Queue(stack, 'MyQueue');

diff --git a/packages/@aws-cdk/aws-events-targets/test/sns/sns.test.ts b/packages/@aws-cdk/aws-events-targets/test/sns/sns.test.ts
@@ -1,15 +1,15 @@
 import { expect, haveResource } from '@aws-cdk/assert';
 import events = require('@aws-cdk/aws-events');
 import sns = require('@aws-cdk/aws-sns');
-import { Stack } from '@aws-cdk/cdk';
+import { Duration, Stack } from '@aws-cdk/cdk';
 import targets = require('../../lib');
 
 test('sns topic as an event rule target', () => {
   // GIVEN
   const stack = new Stack();
   const topic = new sns.Topic(stack, 'MyTopic');
   const rule = new events.Rule(stack, 'MyRule', {
-    schedule: events.Schedule.rate(1, events.TimeUnit.Hour),
+    schedule: events.Schedule.rate(Duration.hours(1)),
   });
 
   // WHEN
@@ -52,7 +52,7 @@ test('multiple uses of a topic as a target results in a single policy statement'
   // WHEN
   for (let i = 0; i < 5; ++i) {
     const rule = new events.Rule(stack, `Rule${i}`, {
-      schedule: events.Schedule.rate(1, events.TimeUnit.Hour),
+      schedule: events.Schedule.rate(Duration.hours(1)),
     });
     rule.addTarget(new targets.SnsTopic(topic));
   }

diff --git a/packages/@aws-cdk/aws-events-targets/test/sqs/integ.sqs-event-rule-target.ts b/packages/@aws-cdk/aws-events-targets/test/sqs/integ.sqs-event-rule-target.ts
@@ -13,7 +13,7 @@ const app = new cdk.App();
 const stack = new cdk.Stack(app, 'aws-cdk-sqs-event-target');
 
 const event = new events.Rule(stack, 'MyRule', {
-  schedule: events.Schedule.rate(1, events.TimeUnit.Minute),
+  schedule: events.Schedule.rate(cdk.Duration.minutes(1)),
 });
 
 const queue = new sqs.Queue(stack, 'MyQueue');

diff --git a/packages/@aws-cdk/aws-events-targets/test/sqs/sqs.test.ts b/packages/@aws-cdk/aws-events-targets/test/sqs/sqs.test.ts
@@ -1,15 +1,15 @@
 import { expect, haveResource } from '@aws-cdk/assert';
 import events = require('@aws-cdk/aws-events');
 import sqs = require('@aws-cdk/aws-sqs');
-import { Stack } from '@aws-cdk/cdk';
+import { Duration, Stack } from '@aws-cdk/cdk';
 import targets = require('../../lib');
 
 test('sns topic as an event rule target', () => {
   // GIVEN
   const stack = new Stack();
   const queue = new sqs.Queue(stack, 'MyQueue');
   const rule = new events.Rule(stack, 'MyRule', {
-    schedule: events.Schedule.rate(1, events.TimeUnit.Hour),
+    schedule: events.Schedule.rate(Duration.hours(1)),
   });
 
   // WHEN
@@ -75,7 +75,7 @@ test('multiple uses of a queue as a target results in multi policy statement bec
   // WHEN
   for (let i = 0; i < 2; ++i) {
     const rule = new events.Rule(stack, `Rule${i}`, {
-      schedule: events.Schedule.rate(1, events.TimeUnit.Hour),
+      schedule: events.Schedule.rate(Duration.hours(1)),
     });
     rule.addTarget(new targets.SqsQueue(queue));
   }

diff --git a/packages/@aws-cdk/aws-events-targets/test/stepfunctions/statemachine.test.ts b/packages/@aws-cdk/aws-events-targets/test/stepfunctions/statemachine.test.ts
@@ -8,7 +8,7 @@ test('State machine can be used as Event Rule target', () => {
   // GIVEN
   const stack = new cdk.Stack();
   const rule = new events.Rule(stack, 'Rule', {
-    schedule: events.Schedule.rate(1, events.TimeUnit.Minute),
+    schedule: events.Schedule.rate(cdk.Duration.minutes(1)),
   });
   const stateMachine = new sfn.StateMachine(stack, 'SM', {
     definition: new sfn.Wait(stack, 'Hello', { time: sfn.WaitTime.duration(cdk.Duration.seconds(10)) })

diff --git a/packages/@aws-cdk/aws-events/lib/schedule.ts b/packages/@aws-cdk/aws-events/lib/schedule.ts
@@ -1,3 +1,5 @@
+import { Duration } from "@aws-cdk/cdk";
+
 /**
  * Schedule for scheduled event rules
  */
@@ -14,10 +16,15 @@ export abstract class Schedule {
   /**
    * Construct a schedule from an interval and a time unit
    */
-  public static rate(interval: number, unit: TimeUnit): Schedule {
-    const unitStr = interval !== 1 ? `${unit}s` : unit;
+  public static rate(duration: Duration): Schedule {
+    if (duration.toSeconds() === 0) {
+      throw new Error('Duration cannot be 0');
+    }
 
-    return new LiteralSchedule(`rate(${interval} ${unitStr})`);
+    let rate = maybeRate(duration.toDays({ integral: false }), 'day');
+    if (rate === undefined) { rate = maybeRate(duration.toHours({ integral: false }), 'hour'); }
+    if (rate === undefined) { rate = makeRate(duration.toMinutes({ integral: true }), 'minute'); }
+    return new LiteralSchedule(rate);
   }
 
   /**
@@ -129,4 +136,19 @@ class LiteralSchedule extends Schedule {
 
 function fallback<T>(x: T | undefined, def: T): T {
   return x === undefined ? def : x;
+}
+
+/**
+ * Return the rate if the rate is whole number
+ */
+function maybeRate(interval: number, singular: string) {
+  if (interval === 0 || !Number.isInteger(interval)) { return undefined; }
+  return makeRate(interval, singular);
+}
+
+/**
+ * Return 'rate(${interval} ${singular}(s))` for the interval
+ */
+function makeRate(interval: number, singular: string) {
+  return interval === 1 ? `rate(1 ${singular})` : `rate(${interval} ${singular}s)`;
 }
diff --git a/packages/@aws-cdk/aws-events/test/test.input.ts b/packages/@aws-cdk/aws-events/test/test.input.ts
@@ -1,8 +1,8 @@
 import { expect, haveResourceLike } from '@aws-cdk/assert';
 import cdk = require('@aws-cdk/cdk');
-import { Stack } from '@aws-cdk/cdk';
+import { Duration, Stack } from '@aws-cdk/cdk';
 import { Test } from 'nodeunit';
-import { IRuleTarget, RuleTargetInput, Schedule, TimeUnit } from '../lib';
+import { IRuleTarget, RuleTargetInput, Schedule } from '../lib';
 import { Rule } from '../lib/rule';
 
 export = {
@@ -11,7 +11,7 @@ export = {
       // GIVEN
       const stack = new Stack();
       const rule = new Rule(stack, 'Rule', {
-        schedule: Schedule.rate(1, TimeUnit.Minute),
+        schedule: Schedule.rate(Duration.minutes(1)),
       });
 
       // WHEN
@@ -34,7 +34,7 @@ export = {
       // GIVEN
       const stack = new Stack();
       const rule = new Rule(stack, 'Rule', {
-        schedule: Schedule.rate(1, TimeUnit.Minute),
+        schedule: Schedule.rate(Duration.minutes(1)),
       });
 
       // WHEN
@@ -56,7 +56,7 @@ export = {
       // GIVEN
       const stack = new Stack();
       const rule = new Rule(stack, 'Rule', {
-        schedule: Schedule.rate(1, TimeUnit.Minute),
+        schedule: Schedule.rate(Duration.minutes(1)),
       });
 
       // WHEN
@@ -78,7 +78,7 @@ export = {
       // GIVEN
       const stack = new Stack();
       const rule = new Rule(stack, 'Rule', {
-        schedule: Schedule.rate(1, TimeUnit.Minute),
+        schedule: Schedule.rate(Duration.minutes(1)),
       });
 
       const world = cdk.Lazy.stringValue({ produce: () => 'world' });

diff --git a/packages/@aws-cdk/aws-events/test/test.rule.ts b/packages/@aws-cdk/aws-events/test/test.rule.ts
@@ -4,7 +4,7 @@ import { ServicePrincipal } from '@aws-cdk/aws-iam';
 import cdk = require('@aws-cdk/cdk');
 import { Stack } from '@aws-cdk/cdk';
 import { Test } from 'nodeunit';
-import { EventField, IRule, IRuleTarget, RuleTargetInput, Schedule, TimeUnit } from '../lib';
+import { EventField, IRule, IRuleTarget, RuleTargetInput, Schedule } from '../lib';
 import { Rule } from '../lib/rule';
 
 // tslint:disable:object-literal-key-quotes
@@ -14,7 +14,7 @@ export = {
     const stack = new cdk.Stack();
 
     new Rule(stack, 'MyRule', {
-        schedule: Schedule.rate(10, TimeUnit.Minute),
+      schedule: Schedule.rate(cdk.Duration.minutes(10)),
     });
 
     expect(stack).toMatch({
@@ -38,7 +38,7 @@ export = {
     // WHEN
     new Rule(stack, 'MyRule', {
       ruleName: cdk.PhysicalName.of('PhysicalName'),
-      schedule: Schedule.rate(10, TimeUnit.Minute),
+      schedule: Schedule.rate(cdk.Duration.minutes(10)),
     });
 
     // THEN
@@ -174,7 +174,7 @@ export = {
 
     const rule = new Rule(stack, 'EventRule', {
       targets: [ t1 ],
-      schedule: Schedule.rate(5, TimeUnit.Minute),
+      schedule: Schedule.rate(cdk.Duration.minutes(5)),
     });
 
     rule.addTarget(t2);
@@ -216,7 +216,7 @@ export = {
     const stack = new cdk.Stack();
 
     const rule = new Rule(stack, 'EventRule', {
-      schedule: Schedule.rate(1, TimeUnit.Minute),
+      schedule: Schedule.rate(cdk.Duration.minutes(1)),
     });
 
     // a plain string should just be stringified (i.e. double quotes added and escaped)
@@ -301,7 +301,7 @@ export = {
     const stack = new cdk.Stack();
 
     const rule = new Rule(stack, 'EventRule', {
-      schedule: Schedule.rate(1, TimeUnit.Minute),
+      schedule: Schedule.rate(cdk.Duration.minutes(1)),
     });
 
     const role = new iam.Role(stack, 'SomeRole', {