Skip to content

Commit

Permalink
fix(events,applicationautoscaling): specifying a schedule rate in sec…
Browse files Browse the repository at this point in the history
…onds results in an error (#13689)

A previous change - b1449a1 - 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*
  • Loading branch information
Niranjan Jayakar committed Mar 23, 2021
1 parent 2c82c1d commit 4d63482
Show file tree
Hide file tree
Showing 5 changed files with 33 additions and 13 deletions.
8 changes: 4 additions & 4 deletions packages/@aws-cdk/aws-applicationautoscaling/lib/schedule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
12 changes: 9 additions & 3 deletions packages/@aws-cdk/aws-applicationautoscaling/test/test.cron.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
},

Expand Down
8 changes: 4 additions & 4 deletions packages/@aws-cdk/aws-events/lib/schedule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
4 changes: 2 additions & 2 deletions packages/@aws-cdk/aws-events/test/test.rule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,15 +70,15 @@ 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();
},

'Millis is not an allowed value for Schedule rate'(test: Test) {
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();
},

Expand Down
14 changes: 14 additions & 0 deletions packages/@aws-cdk/aws-events/test/test.schedule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
},
};

0 comments on commit 4d63482

Please sign in to comment.