From 5d6233164611d69ac1bf5c73e1518eb14dbace8d Mon Sep 17 00:00:00 2001 From: Niranjan Jayakar Date: Mon, 22 Mar 2021 18:49:42 +0000 Subject: [PATCH] fix(events,applicationautoscaling): specifying a schedule rate in seconds results in an error (#13689) A previous change - b1449a17 - introduced a validation that should have been applied only when Duration is specified as a token. Instead, it was applied for non token values as well. Adjusting the validation so it only applies to when the duration is a token. fixes #13566 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- .../aws-applicationautoscaling/lib/schedule.ts | 8 ++++---- .../aws-applicationautoscaling/test/test.cron.ts | 12 +++++++++--- packages/@aws-cdk/aws-events/lib/schedule.ts | 8 ++++---- packages/@aws-cdk/aws-events/test/test.rule.ts | 4 ++-- packages/@aws-cdk/aws-events/test/test.schedule.ts | 14 ++++++++++++++ 5 files changed, 33 insertions(+), 13 deletions(-) diff --git a/packages/@aws-cdk/aws-applicationautoscaling/lib/schedule.ts b/packages/@aws-cdk/aws-applicationautoscaling/lib/schedule.ts index cdeb00be9a426..3bd74c1e4f681 100644 --- a/packages/@aws-cdk/aws-applicationautoscaling/lib/schedule.ts +++ b/packages/@aws-cdk/aws-applicationautoscaling/lib/schedule.ts @@ -17,11 +17,11 @@ export abstract class Schedule { * Construct a schedule from an interval and a time unit */ public static rate(duration: Duration): Schedule { - const validDurationUnit = ['minute', 'minutes', 'hour', 'hours', 'day', 'days']; - if (!validDurationUnit.includes(duration.unitLabel())) { - throw new Error("Allowed unit for scheduling is: 'minute', 'minutes', 'hour', 'hours', 'day' or 'days'"); - } if (duration.isUnresolved()) { + const validDurationUnit = ['minute', 'minutes', 'hour', 'hours', 'day', 'days']; + if (!validDurationUnit.includes(duration.unitLabel())) { + throw new Error("Allowed units for scheduling are: 'minute', 'minutes', 'hour', 'hours', 'day' or 'days'"); + } return new LiteralSchedule(`rate(${duration.formatTokenToNumber()})`); } if (duration.toSeconds() === 0) { diff --git a/packages/@aws-cdk/aws-applicationautoscaling/test/test.cron.ts b/packages/@aws-cdk/aws-applicationautoscaling/test/test.cron.ts index ca0fb69812b10..324b2f8243c97 100644 --- a/packages/@aws-cdk/aws-applicationautoscaling/test/test.cron.ts +++ b/packages/@aws-cdk/aws-applicationautoscaling/test/test.cron.ts @@ -20,10 +20,16 @@ export = { test.done(); }, - 'rate must not be in seconds'(test: Test) { + 'rate can be in seconds'(test: Test) { + const duration = appscaling.Schedule.rate(Duration.seconds(120)); + test.equal('rate(2 minutes)', duration.expressionString); + test.done(); + }, + + 'rate must not be in seconds when specified as a token'(test: Test) { test.throws(() => { - appscaling.Schedule.rate(Duration.seconds(1)); - }, /Allowed unit for scheduling is: 'minute', 'minutes', 'hour', 'hours', 'day' or 'days'/); + appscaling.Schedule.rate(Duration.seconds(Lazy.number({ produce: () => 5 }))); + }, /Allowed units for scheduling/); test.done(); }, diff --git a/packages/@aws-cdk/aws-events/lib/schedule.ts b/packages/@aws-cdk/aws-events/lib/schedule.ts index c12afde30aa01..a3fcbaf399283 100644 --- a/packages/@aws-cdk/aws-events/lib/schedule.ts +++ b/packages/@aws-cdk/aws-events/lib/schedule.ts @@ -17,11 +17,11 @@ export abstract class Schedule { * Construct a schedule from an interval and a time unit */ public static rate(duration: Duration): Schedule { - const validDurationUnit = ['minute', 'minutes', 'hour', 'hours', 'day', 'days']; - if (validDurationUnit.indexOf(duration.unitLabel()) === -1) { - throw new Error('Allowed unit for scheduling is: \'minute\', \'minutes\', \'hour\', \'hours\', \'day\', \'days\''); - } if (duration.isUnresolved()) { + const validDurationUnit = ['minute', 'minutes', 'hour', 'hours', 'day', 'days']; + if (validDurationUnit.indexOf(duration.unitLabel()) === -1) { + throw new Error("Allowed units for scheduling are: 'minute', 'minutes', 'hour', 'hours', 'day', 'days'"); + } return new LiteralSchedule(`rate(${duration.formatTokenToNumber()})`); } if (duration.toSeconds() === 0) { diff --git a/packages/@aws-cdk/aws-events/test/test.rule.ts b/packages/@aws-cdk/aws-events/test/test.rule.ts index 72fe340242924..23fe7e2a2ce3e 100644 --- a/packages/@aws-cdk/aws-events/test/test.rule.ts +++ b/packages/@aws-cdk/aws-events/test/test.rule.ts @@ -70,7 +70,7 @@ export = { 'Seconds is not an allowed value for Schedule rate'(test: Test) { const lazyDuration = cdk.Duration.seconds(cdk.Lazy.number({ produce: () => 5 })); - test.throws(() => Schedule.rate(lazyDuration), /Allowed unit for scheduling is: 'minute', 'minutes', 'hour', 'hours', 'day', 'days'/); + test.throws(() => Schedule.rate(lazyDuration), /Allowed units for scheduling/i); test.done(); }, @@ -78,7 +78,7 @@ export = { const lazyDuration = cdk.Duration.millis(cdk.Lazy.number({ produce: () => 5 })); // THEN - test.throws(() => Schedule.rate(lazyDuration), /Allowed unit for scheduling is: 'minute', 'minutes', 'hour', 'hours', 'day', 'days'/); + test.throws(() => Schedule.rate(lazyDuration), /Allowed units for scheduling/i); test.done(); }, diff --git a/packages/@aws-cdk/aws-events/test/test.schedule.ts b/packages/@aws-cdk/aws-events/test/test.schedule.ts index e9c24a51c92b2..2d9728e743202 100644 --- a/packages/@aws-cdk/aws-events/test/test.schedule.ts +++ b/packages/@aws-cdk/aws-events/test/test.schedule.ts @@ -80,4 +80,18 @@ export = { .expressionString); test.done(); }, + + 'rate can be in seconds'(test: Test) { + test.equal('rate(2 minutes)', + events.Schedule.rate(Duration.seconds(120)) + .expressionString); + test.done(); + }, + + 'rate must not be in seconds when specified as a token'(test: Test) { + test.throws(() => { + events.Schedule.rate(Duration.seconds(Lazy.number({ produce: () => 5 }))); + }, /Allowed units for scheduling/); + test.done(); + }, };