chore(events): add warning when minute is not defined

This commit updates PR aws#19197 based off feedback. It removes the unnecessary feature flag and refactors the addition of the warning. It also adds the same changes to aws-applicationautoscaling and aws-autoscaling due to cross dependencies and adds tests where Schedule class for each of these packages is used.
TheRealAmazonKendra · Mar 11, 2022 · 6e977e1 · 6e977e1
1 parent 6724dc0
commit 6e977e1
Show file tree

Hide file tree

Showing 18 changed files with 564 additions and 306 deletions.
diff --git a/packages/@aws-cdk/aws-applicationautoscaling/lib/scalable-target.ts b/packages/@aws-cdk/aws-applicationautoscaling/lib/scalable-target.ts
@@ -148,6 +148,10 @@ export class ScalableTarget extends Resource implements IScalableTarget {
     if (action.minCapacity === undefined && action.maxCapacity === undefined) {
       throw new Error(`You must supply at least one of minCapacity or maxCapacity, got ${JSON.stringify(action)}`);
     }
+
+    // add a warning on synth when minute is not defined in a cron schedule
+    action.schedule._bind(this);
+
     this.actions.push({
       scheduledActionName: id,
       schedule: action.schedule.expressionString,

diff --git a/packages/@aws-cdk/aws-applicationautoscaling/lib/schedule.ts b/packages/@aws-cdk/aws-applicationautoscaling/lib/schedule.ts
@@ -1,4 +1,5 @@
-import { Duration } from '@aws-cdk/core';
+import { Annotations, Duration } from '@aws-cdk/core';
+import { Construct } from 'constructs';
 
 /**
  * Schedule for scheduled scaling actions
@@ -58,16 +59,29 @@ export abstract class Schedule {
     const day = fallback(options.day, options.weekDay !== undefined ? '?' : '*');
     const weekDay = fallback(options.weekDay, '?');
 
-    return new LiteralSchedule(`cron(${minute} ${hour} ${day} ${month} ${weekDay} ${year})`);
+    return new class extends Schedule {
+      public readonly expressionString: string = `cron(${minute} ${hour} ${day} ${month} ${weekDay} ${year})`;
+      public _bind(scope: Construct) {
+        if (!options.minute) {
+          Annotations.of(scope).addWarning('cron: If you don\'t pass \'minute\', by default the event runs every minute. Pass \'minute: \'*\'\' if that\'s what you intend, or \'minute: 0\' to run once per hour instead.');
+        }
+        return new LiteralSchedule(this.expressionString);
+      }
+    };
   }
 
   /**
    * Retrieve the expression for this schedule
    */
   public abstract readonly expressionString: string;
 
-  protected constructor() {
-  }
+  protected constructor() {}
+
+  /**
+   *
+   * @internal
+   */
+  public abstract _bind(scope: Construct): void;
 }
 
 /**
@@ -126,6 +140,8 @@ class LiteralSchedule extends Schedule {
   constructor(public readonly expressionString: string) {
     super();
   }
+
+  public _bind() {}
 }
 
 function fallback<T>(x: T | undefined, def: T): T {

diff --git a/packages/@aws-cdk/aws-applicationautoscaling/test/scalable-target.test.ts b/packages/@aws-cdk/aws-applicationautoscaling/test/scalable-target.test.ts
@@ -1,4 +1,4 @@
-import { Match, Template } from '@aws-cdk/assertions';
+import { Annotations, Match, Template } from '@aws-cdk/assertions';
 import * as cloudwatch from '@aws-cdk/aws-cloudwatch';
 import * as iam from '@aws-cdk/aws-iam';
 import * as cdk from '@aws-cdk/core';
@@ -79,6 +79,53 @@ describe('scalable target', () => {
     });
   });
 
+  test('scheduled scaling shows warning when minute is not defined in cron', () => {
+    // GIVEN
+    const stack = new cdk.Stack();
+    const target = createScalableTarget(stack);
+
+    // WHEN
+    target.scaleOnSchedule('ScaleUp', {
+      schedule: appscaling.Schedule.cron({
+        hour: '8',
+        day: '1',
+      }),
+      maxCapacity: 50,
+      minCapacity: 1,
+    });
+
+    // THEN
+    expect(target.node.metadataEntry).toEqual([{
+      type: 'aws:cdk:warning',
+      data: 'cron: If you don\'t pass \'minute\', by default the event runs every minute. Pass \'minute: \'*\'\' if that\'s what you intend, or \'minute: 0\' to run once per hour instead.',
+      trace: undefined,
+    }]);
+    Annotations.fromStack(stack).hasWarning('/Default/Target', "cron: If you don't pass 'minute', by default the event runs every minute. Pass 'minute: '*'' if that's what you intend, or 'minute: 0' to run once per hour instead.");
+
+  });
+
+  test('scheduled scaling shows no warning when minute is * in cron', () => {
+    // GIVEN
+    const stack = new cdk.Stack();
+    const target = createScalableTarget(stack);
+
+    // WHEN
+    target.scaleOnSchedule('ScaleUp', {
+      schedule: appscaling.Schedule.cron({
+        hour: '8',
+        day: '1',
+        minute: '*',
+      }),
+      maxCapacity: 50,
+      minCapacity: 1,
+    });
+
+    // THEN
+    expect(target.node.metadataEntry).toEqual([]);
+    const annotations = Annotations.fromStack(stack).findWarning('*', Match.anyValue());
+    expect(annotations.length).toBe(0);
+  });
+
   test('step scaling on MathExpression', () => {
     // GIVEN
     const stack = new cdk.Stack();

diff --git a/packages/@aws-cdk/aws-autoscaling/lib/schedule.ts b/packages/@aws-cdk/aws-autoscaling/lib/schedule.ts
@@ -1,3 +1,6 @@
+import { Annotations } from '@aws-cdk/core';
+import { Construct } from 'constructs';
+
 /**
  * Schedule for scheduled scaling actions
  */
@@ -26,16 +29,29 @@ export abstract class Schedule {
     const day = fallback(options.day, '*');
     const weekDay = fallback(options.weekDay, '*');
 
-    return new LiteralSchedule(`${minute} ${hour} ${day} ${month} ${weekDay}`);
+    return new class extends Schedule {
+      public readonly expressionString: string = `${minute} ${hour} ${day} ${month} ${weekDay}`;
+      public _bind(scope: Construct) {
+        if (!options.minute) {
+          Annotations.of(scope).addWarning('cron: If you don\'t pass \'minute\', by default the event runs every minute. Pass \'minute: \'*\'\' if that\'s what you intend, or \'minute: 0\' to run once per hour instead.');
+        }
+        return new LiteralSchedule(this.expressionString);
+      }
+    };
   }
 
   /**
    * Retrieve the expression for this schedule
    */
   public abstract readonly expressionString: string;
 
-  protected constructor() {
-  }
+  protected constructor() {}
+
+  /**
+   *
+   * @internal
+   */
+  public abstract _bind(scope: Construct): void;
 }
 
 /**
@@ -87,6 +103,8 @@ class LiteralSchedule extends Schedule {
   constructor(public readonly expressionString: string) {
     super();
   }
+
+  public _bind(): void {}
 }
 
 function fallback<T>(x: T | undefined, def: T): T {

diff --git a/packages/@aws-cdk/aws-autoscaling/lib/scheduled-action.ts b/packages/@aws-cdk/aws-autoscaling/lib/scheduled-action.ts
@@ -97,6 +97,9 @@ export class ScheduledAction extends Resource {
       throw new Error('At least one of minCapacity, maxCapacity, or desiredCapacity is required');
     }
 
+    // add a warning on synth when minute is not defined in a cron schedule
+    props.schedule._bind(this);
+
     new CfnScheduledAction(this, 'Resource', {
       autoScalingGroupName: props.autoScalingGroup.autoScalingGroupName,
       startTime: formatISO(props.startTime),

diff --git a/packages/@aws-cdk/aws-autoscaling/test/scheduled-action.test.ts b/packages/@aws-cdk/aws-autoscaling/test/scheduled-action.test.ts
@@ -1,4 +1,4 @@
-import { Template } from '@aws-cdk/assertions';
+import { Annotations, Match, Template } from '@aws-cdk/assertions';
 import * as ec2 from '@aws-cdk/aws-ec2';
 import { describeDeprecated } from '@aws-cdk/cdk-build-tools';
 import * as cdk from '@aws-cdk/core';
@@ -120,6 +120,40 @@ describeDeprecated('scheduled action', () => {
       },
     });
   });
+
+  test('scheduled scaling shows warning when minute is not defined in cron', () => {
+    // GIVEN
+    const stack = new cdk.Stack();
+    const asg = makeAutoScalingGroup(stack);
+
+    // WHEN
+    asg.scaleOnSchedule('ScaleOutInTheMorning', {
+      schedule: autoscaling.Schedule.cron({ hour: '8' }),
+      minCapacity: 10,
+    });
+
+    // THEN
+    Annotations.fromStack(stack).hasWarning('/Default/ASG/ScheduledActionScaleOutInTheMorning', "cron: If you don't pass 'minute', by default the event runs every minute. Pass 'minute: '*'' if that's what you intend, or 'minute: 0' to run once per hour instead.");
+  });
+
+  test('scheduled scaling shows no warning when minute is * in cron', () => {
+    // GIVEN
+    const stack = new cdk.Stack();
+    const asg = makeAutoScalingGroup(stack);
+
+    // WHEN
+    asg.scaleOnSchedule('ScaleOutInTheMorning', {
+      schedule: autoscaling.Schedule.cron({
+        hour: '8',
+        minute: '*',
+      }),
+      minCapacity: 10,
+    });
+
+    // THEN
+    const annotations = Annotations.fromStack(stack).findWarning('*', Match.anyValue());
+    expect(annotations.length).toBe(0);
+  });
 });
 
 function makeAutoScalingGroup(scope: constructs.Construct) {

diff --git a/packages/@aws-cdk/aws-dynamodb/test/dynamodb.test.ts b/packages/@aws-cdk/aws-dynamodb/test/dynamodb.test.ts
@@ -1,4 +1,4 @@
-import { Match, Template } from '@aws-cdk/assertions';
+import { Annotations, Match, Template } from '@aws-cdk/assertions';
 import * as appscaling from '@aws-cdk/aws-applicationautoscaling';
 import * as iam from '@aws-cdk/aws-iam';
 import * as kinesis from '@aws-cdk/aws-kinesis';
@@ -1622,6 +1622,47 @@ test('can autoscale on a schedule', () => {
   });
 });
 
+test('scheduled scaling shows warning when minute is not defined in cron', () => {
+  // GIVEN
+  const stack = new Stack();
+  const table = new Table(stack, CONSTRUCT_NAME, {
+    readCapacity: 42,
+    writeCapacity: 1337,
+    partitionKey: { name: 'Hash', type: AttributeType.STRING },
+  });
+
+  // WHEN
+  const scaling = table.autoScaleReadCapacity({ minCapacity: 1, maxCapacity: 100 });
+  scaling.scaleOnSchedule('SaveMoneyByNotScalingUp', {
+    schedule: appscaling.Schedule.cron({}),
+    maxCapacity: 10,
+  });
+
+  // THEN
+  Annotations.fromStack(stack).hasWarning('/Default/MyTable/ReadScaling/Target', "cron: If you don't pass 'minute', by default the event runs every minute. Pass 'minute: '*'' if that's what you intend, or 'minute: 0' to run once per hour instead.");
+});
+
+test('scheduled scaling shows no warning when minute is * in cron', () => {
+  // GIVEN
+  const stack = new Stack();
+  const table = new Table(stack, CONSTRUCT_NAME, {
+    readCapacity: 42,
+    writeCapacity: 1337,
+    partitionKey: { name: 'Hash', type: AttributeType.STRING },
+  });
+
+  // WHEN
+  const scaling = table.autoScaleReadCapacity({ minCapacity: 1, maxCapacity: 100 });
+  scaling.scaleOnSchedule('SaveMoneyByNotScalingUp', {
+    schedule: appscaling.Schedule.cron({ minute: '*' }),
+    maxCapacity: 10,
+  });
+
+  // THEN
+  const annotations = Annotations.fromStack(stack).findWarning('*', Match.anyValue());
+  expect(annotations.length).toBe(0);
+});
+
 describe('metrics', () => {
   test('Can use metricConsumedReadCapacityUnits on a Dynamodb Table', () => {
     // GIVEN

diff --git a/packages/@aws-cdk/aws-ecs-patterns/lib/base/scheduled-task-base.ts b/packages/@aws-cdk/aws-ecs-patterns/lib/base/scheduled-task-base.ts
@@ -170,6 +170,9 @@ export abstract class ScheduledTaskBase extends CoreConstruct {
       ruleName: props.ruleName,
       enabled: props.enabled,
     });
+
+    // add a warning on synth when minute is not defined in a cron schedule
+    props.schedule._bind(scope);
   }
 
   /**

diff --git a/packages/@aws-cdk/aws-ecs-patterns/test/ec2/scheduled-ecs-task.test.ts b/packages/@aws-cdk/aws-ecs-patterns/test/ec2/scheduled-ecs-task.test.ts
@@ -1,4 +1,4 @@
-import { Template } from '@aws-cdk/assertions';
+import { Annotations, Match, Template } from '@aws-cdk/assertions';
 import { AutoScalingGroup } from '@aws-cdk/aws-autoscaling';
 import * as ec2 from '@aws-cdk/aws-ec2';
 import { MachineImage } from '@aws-cdk/aws-ec2';
@@ -347,3 +347,48 @@ test('Scheduled Ec2 Task - exposes ECS Task', () => {
   // THEN
   expect(scheduledEc2Task.task).toBeDefined();
 });
+
+test('Scheduled Ec2 Task shows warning when minute is not defined in cron', () => {
+  // GIVEN
+  const stack = new cdk.Stack();
+  const vpc = new ec2.Vpc(stack, 'Vpc', { maxAzs: 1 });
+  const cluster = new ecs.Cluster(stack, 'EcsCluster', { vpc });
+
+  new ScheduledEc2Task(stack, 'ScheduledEc2Task', {
+    cluster,
+    scheduledEc2TaskImageOptions: {
+      image: ecs.ContainerImage.fromRegistry('henk'),
+      memoryLimitMiB: 512,
+    },
+    schedule: events.Schedule.cron({}),
+  });
+
+  // THEN
+  expect(stack.node.metadataEntry).toEqual([{
+    type: 'aws:cdk:warning',
+    data: 'cron: If you don\'t pass \'minute\', by default the event runs every minute. Pass \'minute: \'*\'\' if that\'s what you intend, or \'minute: 0\' to run once per hour instead.',
+    trace: undefined,
+  }]);
+  Annotations.fromStack(stack).hasWarning('/Default', "cron: If you don't pass 'minute', by default the event runs every minute. Pass 'minute: '*'' if that's what you intend, or 'minute: 0' to run once per hour instead.");
+});
+
+test('Scheduled Ec2 Task shows no warning when minute is * in cron', () => {
+  // GIVEN
+  const stack = new cdk.Stack();
+  const vpc = new ec2.Vpc(stack, 'Vpc', { maxAzs: 1 });
+  const cluster = new ecs.Cluster(stack, 'EcsCluster', { vpc });
+
+  new ScheduledEc2Task(stack, 'ScheduledEc2Task', {
+    cluster,
+    scheduledEc2TaskImageOptions: {
+      image: ecs.ContainerImage.fromRegistry('henk'),
+      memoryLimitMiB: 512,
+    },
+    schedule: events.Schedule.cron({ minute: '*' }),
+  });
+
+  // THEN
+  expect(stack.node.metadataEntry).toEqual([]);
+  const annotations = Annotations.fromStack(stack).findWarning('*', Match.anyValue());
+  expect(annotations.length).toBe(0);
+});
diff --git a/packages/@aws-cdk/aws-ecs-patterns/test/fargate/scheduled-fargate-task.test.ts b/packages/@aws-cdk/aws-ecs-patterns/test/fargate/scheduled-fargate-task.test.ts
@@ -1,4 +1,4 @@
-import { Match, Template } from '@aws-cdk/assertions';
+import { Annotations, Match, Template } from '@aws-cdk/assertions';
 import * as ec2 from '@aws-cdk/aws-ec2';
 import * as ecs from '@aws-cdk/aws-ecs';
 import * as events from '@aws-cdk/aws-events';
@@ -410,3 +410,48 @@ test('Scheduled Fargate Task - exposes ECS Task', () => {
   // THEN
   expect(scheduledFargateTask.task).toBeDefined();
 });
+
+test('Scheduled Fargate Task shows warning when minute is not defined in cron', () => {
+  // GIVEN
+  const stack = new cdk.Stack();
+  const vpc = new ec2.Vpc(stack, 'Vpc', { maxAzs: 1 });
+  const cluster = new ecs.Cluster(stack, 'EcsCluster', { vpc });
+
+  new ScheduledFargateTask(stack, 'ScheduledFargateTask', {
+    cluster,
+    scheduledFargateTaskImageOptions: {
+      image: ecs.ContainerImage.fromRegistry('henk'),
+      memoryLimitMiB: 512,
+    },
+    schedule: events.Schedule.cron({}),
+  });
+
+  // THEN
+  expect(stack.node.metadataEntry).toEqual([{
+    type: 'aws:cdk:warning',
+    data: 'cron: If you don\'t pass \'minute\', by default the event runs every minute. Pass \'minute: \'*\'\' if that\'s what you intend, or \'minute: 0\' to run once per hour instead.',
+    trace: undefined,
+  }]);
+  Annotations.fromStack(stack).hasWarning('/Default', "cron: If you don't pass 'minute', by default the event runs every minute. Pass 'minute: '*'' if that's what you intend, or 'minute: 0' to run once per hour instead.");
+});
+
+test('Scheduled Fargate Task shows no warning when minute is * in cron', () => {
+  // GIVEN
+  const stack = new cdk.Stack();
+  const vpc = new ec2.Vpc(stack, 'Vpc', { maxAzs: 1 });
+  const cluster = new ecs.Cluster(stack, 'EcsCluster', { vpc });
+
+  new ScheduledFargateTask(stack, 'ScheduledFargateTask', {
+    cluster,
+    scheduledFargateTaskImageOptions: {
+      image: ecs.ContainerImage.fromRegistry('henk'),
+      memoryLimitMiB: 512,
+    },
+    schedule: events.Schedule.cron({ minute: '*' }),
+  });
+
+  // THEN
+  expect(stack.node.metadataEntry).toEqual([]);
+  const annotations = Annotations.fromStack(stack).findWarning('*', Match.anyValue());
+  expect(annotations.length).toBe(0);
+});