From a0c279b69408afcdc06c94f691e053c62113de45 Mon Sep 17 00:00:00 2001 From: Kendra Neil Date: Mon, 28 Feb 2022 16:52:35 -0800 Subject: [PATCH] chore(aws-events): add warning when minute is not defined 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. --- packages/@aws-cdk/aws-events/lib/rule.ts | 11 +++++-- packages/@aws-cdk/aws-events/lib/schedule.ts | 11 +++++-- packages/@aws-cdk/aws-events/package.json | 1 + .../@aws-cdk/aws-events/test/rule.test.ts | 32 ++++++++++++++++++- .../@aws-cdk/aws-events/test/schedule.test.ts | 7 ++++ packages/@aws-cdk/cx-api/lib/features.ts | 6 ++++ 6 files changed, 63 insertions(+), 5 deletions(-) diff --git a/packages/@aws-cdk/aws-events/lib/rule.ts b/packages/@aws-cdk/aws-events/lib/rule.ts index 232fd5582f9f7..b8c807e36da88 100644 --- a/packages/@aws-cdk/aws-events/lib/rule.ts +++ b/packages/@aws-cdk/aws-events/lib/rule.ts @@ -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'; @@ -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, diff --git a/packages/@aws-cdk/aws-events/lib/schedule.ts b/packages/@aws-cdk/aws-events/lib/schedule.ts index a3fcbaf399283..647da11e47e1a 100644 --- a/packages/@aws-cdk/aws-events/lib/schedule.ts +++ b/packages/@aws-cdk/aws-events/lib/schedule.ts @@ -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, '*'); @@ -51,7 +53,7 @@ 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); } /** @@ -59,6 +61,11 @@ export abstract class 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() { } } @@ -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(); } } diff --git a/packages/@aws-cdk/aws-events/package.json b/packages/@aws-cdk/aws-events/package.json index 377a1c8ac9df9..b77e24e7bc870 100644 --- a/packages/@aws-cdk/aws-events/package.json +++ b/packages/@aws-cdk/aws-events/package.json @@ -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", diff --git a/packages/@aws-cdk/aws-events/test/rule.test.ts b/packages/@aws-cdk/aws-events/test/rule.test.ts index 1cbd776441fce..09dd7b46c1d8d 100644 --- a/packages/@aws-cdk/aws-events/test/rule.test.ts +++ b/packages/@aws-cdk/aws-events/test/rule.test.ts @@ -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(); @@ -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', { @@ -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'); diff --git a/packages/@aws-cdk/aws-events/test/schedule.test.ts b/packages/@aws-cdk/aws-events/test/schedule.test.ts index 9dea322c62d0a..491c67789ded8 100644 --- a/packages/@aws-cdk/aws-events/test/schedule.test.ts +++ b/packages/@aws-cdk/aws-events/test/schedule.test.ts @@ -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)); diff --git a/packages/@aws-cdk/cx-api/lib/features.ts b/packages/@aws-cdk/cx-api/lib/features.ts index 9278717ef1c15..f930680f89f5d 100644 --- a/packages/@aws-cdk/cx-api/lib/features.ts +++ b/packages/@aws-cdk/cx-api/lib/features.ts @@ -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 * @@ -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, }; /**