Skip to content

Commit

Permalink
chore(aws-events): add warning when minute is not defined
Browse files Browse the repository at this point in the history
Per issue #17881, the default value for CRON schedule configuration parameters are  which runs every minute/hour/day.

This change adds a warning to be emitted upon synthesis and deployment, if minute is empty, that the CRON job will run every minute.

Closes #17881.
  • Loading branch information
TheRealAmazonKendra committed Mar 1, 2022
1 parent 6595d04 commit a0c279b
Show file tree
Hide file tree
Showing 6 changed files with 63 additions and 5 deletions.
11 changes: 9 additions & 2 deletions packages/@aws-cdk/aws-events/lib/rule.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { IRole, PolicyStatement, Role, ServicePrincipal } from '@aws-cdk/aws-iam';
import { App, IResource, Lazy, Names, Resource, Stack, Token, PhysicalName, ArnFormat } from '@aws-cdk/core';
import { App, IResource, Lazy, Names, Resource, Stack, Token, PhysicalName, ArnFormat, Annotations, FeatureFlags } from '@aws-cdk/core';
import * as cxapi from '@aws-cdk/cx-api';
import { Node, Construct } from 'constructs';
import { IEventBus } from './event-bus';
import { EventPattern } from './event-pattern';
Expand Down Expand Up @@ -127,8 +128,14 @@ export class Rule extends Resource implements IRule {
throw new Error('Cannot associate rule with \'eventBus\' when using \'schedule\'');
}

if (FeatureFlags.of(this).isEnabled(cxapi.EVENTS_WARNING_CRON_MINUTES_NOT_SET)) {
if (props.schedule?.minuteUndefinedWarning) {
Annotations.of(this).addWarning(props.schedule.minuteUndefinedWarning);
}
}

this.description = props.description;
this.scheduleExpression = props.schedule && props.schedule.expressionString;
this.scheduleExpression = props.schedule?.expressionString;

const resource = new CfnRule(this, 'Resource', {
name: this.physicalName,
Expand Down
11 changes: 9 additions & 2 deletions packages/@aws-cdk/aws-events/lib/schedule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ export abstract class Schedule {
throw new Error('Cannot supply both \'day\' and \'weekDay\', use at most one');
}

const minuteUndefinedWarning = options.minute ?? "When 'minute' is undefined in CronOptions, '*' is used as the default value, scheduling the event for every minute within the supplied parameters.";

const minute = fallback(options.minute, '*');
const hour = fallback(options.hour, '*');
const month = fallback(options.month, '*');
Expand All @@ -51,14 +53,19 @@ 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 LiteralSchedule(`cron(${minute} ${hour} ${day} ${month} ${weekDay} ${year})`, minuteUndefinedWarning);
}

/**
* Retrieve the expression for this schedule
*/
public abstract readonly expressionString: string;

/**
* The warning displayed when the user has not defined the minute field in CronOptions.
*/
public abstract readonly minuteUndefinedWarning?: string;

protected constructor() {
}
}
Expand Down Expand Up @@ -116,7 +123,7 @@ export interface CronOptions {
}

class LiteralSchedule extends Schedule {
constructor(public readonly expressionString: string) {
constructor(public readonly expressionString: string, public readonly minuteUndefinedWarning?: string) {
super();
}
}
Expand Down
1 change: 1 addition & 0 deletions packages/@aws-cdk/aws-events/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@
"dependencies": {
"@aws-cdk/aws-iam": "0.0.0",
"@aws-cdk/core": "0.0.0",
"@aws-cdk/cx-api": "0.0.0",
"constructs": "^3.3.69"
},
"homepage": "https://github.com/aws/aws-cdk",
Expand Down
32 changes: 31 additions & 1 deletion packages/@aws-cdk/aws-events/test/rule.test.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,17 @@
/* eslint-disable object-curly-newline */
import { Match, Template } from '@aws-cdk/assertions';
import * as iam from '@aws-cdk/aws-iam';
import { testFutureBehavior, testLegacyBehavior } from '@aws-cdk/cdk-build-tools';
import * as cdk from '@aws-cdk/core';
import * as cxapi from '@aws-cdk/cx-api';
import { EventBus, EventField, IRule, IRuleTarget, RuleTargetConfig, RuleTargetInput, Schedule } from '../lib';
import { Rule } from '../lib/rule';

/* eslint-disable quote-props */

describe('rule', () => {
const flags = { [cxapi.EVENTS_WARNING_CRON_MINUTES_NOT_SET]: true };

test('default rule', () => {
const stack = new cdk.Stack();

Expand All @@ -28,6 +32,32 @@ describe('rule', () => {
});
});

testFutureBehavior('rule displays warning when minutes are not included in schedule and flag is enabled', flags, cdk.App, (app) => {
const stack = new cdk.Stack(app);
const rule = new Rule(stack, 'MyRule', {
schedule: Schedule.cron({
hour: '8',
day: '1',
}),
});
expect(rule.node.metadataEntry).toEqual([{
type: 'aws:cdk:warning',
data: "When 'minute' is undefined in CronOptions, '*' is used as the default value, scheduling the event for every minute within the supplied parameters.",
trace: undefined,
}]);
});

testLegacyBehavior('rule does not display warning when minutes are not included in schedule and flag is disabled', cdk.App, (app) => {
const stack = new cdk.Stack(app);
const rule = new Rule(stack, 'MyRule', {
schedule: Schedule.cron({
hour: '8',
day: '1',
}),
});
expect(rule.node.metadataEntry).toEqual([]);
});

test('can get rule name', () => {
const stack = new cdk.Stack();
const rule = new Rule(stack, 'MyRule', {
Expand Down Expand Up @@ -133,7 +163,7 @@ describe('rule', () => {
});
});

test('fails synthesis if neither eventPattern nor scheudleExpression are specified', () => {
test('fails synthesis if neither eventPattern nor scheduleExpression are specified', () => {
const app = new cdk.App();
const stack = new cdk.Stack(app, 'MyStack');
new Rule(stack, 'Rule');
Expand Down
7 changes: 7 additions & 0 deletions packages/@aws-cdk/aws-events/test/schedule.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,13 @@ describe('schedule', () => {
}).expressionString);
});

test('warning message is included in Schedule when cron does not include minute', () => {
expect(events.Schedule.cron({
hour: '8',
day: '1',
}).minuteUndefinedWarning).toEqual("When 'minute' is undefined in CronOptions, '*' is used as the default value, scheduling the event for every minute within the supplied parameters.");
});

test('rate must be whole number of minutes', () => {
expect(() => {
events.Schedule.rate(Duration.minutes(0.13456));
Expand Down
6 changes: 6 additions & 0 deletions packages/@aws-cdk/cx-api/lib/features.ts
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,11 @@ export const ECS_SERVICE_EXTENSIONS_ENABLE_DEFAULT_LOG_DRIVER = '@aws-cdk-contai
*/
export const EC2_UNIQUE_IMDSV2_LAUNCH_TEMPLATE_NAME = '@aws-cdk/aws-ec2:uniqueImdsv2TemplateName';

/**
* Enable this feature flag to have Events emit a warning upon synthesis or deployment when `minute` is not defined in CronOptions.
*/
export const EVENTS_WARNING_CRON_MINUTES_NOT_SET = '@aws-cdk/aws-events:cron';

/**
* Flag values that should apply for new projects
*
Expand All @@ -240,6 +245,7 @@ export const FUTURE_FLAGS: { [key: string]: boolean } = {
[CLOUDFRONT_DEFAULT_SECURITY_POLICY_TLS_V1_2_2021]: true,
[ECS_SERVICE_EXTENSIONS_ENABLE_DEFAULT_LOG_DRIVER]: true,
[EC2_UNIQUE_IMDSV2_LAUNCH_TEMPLATE_NAME]: true,
[EVENTS_WARNING_CRON_MINUTES_NOT_SET]: true,
};

/**
Expand Down

0 comments on commit a0c279b

Please sign in to comment.