chore(aws-events): add warning when minute is not defined #19197

TheRealAmazonKendra · 2022-03-01T01:43:57Z

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.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

gitpod-io · 2022-03-01T01:44:00Z

rix0rrr

Thanks for picking this up!

rix0rrr · 2022-03-02T16:44:29Z

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.";


Suggested change

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 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." : undefined;

a ?? b means: a if it has a value, otherwise b.

Well done on the non-cryptic error message! One more thing I like to see in error messages: a recommended course of action. This message now tells the user (in a clear and detailed way, which is good) that something is wrong, or might be wrong, but doesn't really tell them what to do about it.

How about something like:

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.

Oops. That was not the logic I intended (per first comment). Good catch. Though, since this is no longer the correct place to define this warning, I'm not committing the change. Since we don't actually need the flag, this logic can exist in the rule package.

Per second comment, I like that message better. I'll go with that suggestion.

rix0rrr · 2022-03-02T16:48:32Z

packages/@aws-cdk/aws-events/lib/schedule.ts

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


I don't like it very much to expose a new public field, just for potentially having or not having a warning. It's also very specific to this particular problem, what if we have other things we might to emit warnings for in the future? Would we then add public abstract readonly thisHappensTooOftenWarning?: string and public abstract readonly wrongWeekDayWarning?: string ?

Are you doing it this way because in order to evaluate the feature flag, you need a scope and therefore it cannot be done during construction of the Schedule instance?

I can think of two possible alternatives:

Don't have the feature flag on the warning in the first place. It's not really breaking behavior, so no need to gate it with a flag.

Use a Lazy.uncached*: when the Lazy gets evaluated, you will have a scope that you can check for feature flags on.

Yep, this was just to add the feature flag due to misunderstanding your suggestion. I'm removing this.

rix0rrr · 2022-03-02T16:50:12Z

packages/@aws-cdk/cx-api/lib/features.ts

+/**
+ * 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';


Do you think we need a flag to enable the warning? It wouldn't really change behavior, so it feels unnecessarily safe.

I was thinking we would add a feature flag to actually change behavior:

I think we can agree that the behavior should have been: an undefined minute should evaluate to 0, not to * (Can we? Maybe not?). We can make that the NEW default behavior, and put that default behavior behind a feature flag.

I'm also fine not doing that at all and sticking to a warning though.

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.

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.

This commit updates PR #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.

rix0rrr · 2022-03-14T13:57:45Z

Thanks!!

mergify · 2022-03-14T13:58:10Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

…undefined

aws-cdk-automation · 2022-03-14T14:43:13Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: b36956e
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2022-03-14T14:43:30Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

My latest attempt at schedule unification across modules. The following modules use a version of schedule, which this PR aims to unify: - aws-scheduler-alpha - aws-events - aws-application-autoscaling - aws-autoscaling - aws-synthetics-alpha - aws-backup The idea is to have a single source of truth, `core.Schedule` that is exposed and meant to be extended by modules that use a schedule. This is to avoid breaking changes -- every module that currently exports a schedule class continues to do so. Each module can customize their schedule class to its liking, for example, whether or not to support `schedule.at` or `cronOptions.timeZone`. This PR will fix inconsistencies like: - `backup.scheduleExpression` depending on `events.Schedule`, which is semi-deprecated by the Events team (they want people to use the Schedule class in `aws-scheduler-alpha`). - `aws-scheduler-alpha` depending on `events.Schedule` as well. - `backup.scheduleExpression` allowing `schedule.rate(duration)` to be specified (synth-time error) when we know that backup schedules only can be cron expressions. - having to implement the new `timeZone` property in all instances of schedules - avoids us from having to perform maintenance in multiple places like #19197 - `timeZone` property existing directly on a construct when it only pertains to `cron` expressions. This is an anomaly because we typically do not want construct-level properties to only be impactful depending on other properties. [See superseded PRs] Challenges: - subtle differences in expressions that are accepted. This is solved by `core.Schedule` only exposing `protected` APIs, which are then picked by the consuming modules to be exposed as `public`. - subtle difference in `cron` expressions accepted. I do some magic in `aws-autoscaling` to get the cron expression returned there to be as expected. Supersedes #27052 and #27012 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

github-actions bot added the @aws-cdk/aws-events Related to CloudWatch Events label Mar 1, 2022

github-actions bot assigned rix0rrr Mar 1, 2022

mergify bot added the contribution/core This is a PR that came from AWS. label Mar 1, 2022

rix0rrr requested changes Mar 2, 2022

View reviewed changes

rix0rrr added feature-request A feature should be added or improved. p1 and removed p1 feature-request A feature should be added or improved. @aws-cdk/aws-events Related to CloudWatch Events contribution/core This is a PR that came from AWS. labels Mar 4, 2022

mergify bot added the contribution/core This is a PR that came from AWS. label Mar 4, 2022

rix0rrr added feature-request A feature should be added or improved. p1 and removed contribution/core This is a PR that came from AWS. labels Mar 4, 2022

mergify bot added the contribution/core This is a PR that came from AWS. label Mar 4, 2022

TheRealAmazonKendra force-pushed the TheRealAmazonKendra/events-warning-minute-undefined branch from a0c279b to 0f7cbcd Compare March 11, 2022 16:53

rix0rrr approved these changes Mar 14, 2022

View reviewed changes

Merge branch 'master' into TheRealAmazonKendra/events-warning-minute-…

b36956e

…undefined

mergify bot merged commit 885d0c2 into master Mar 14, 2022

mergify bot deleted the TheRealAmazonKendra/events-warning-minute-undefined branch March 14, 2022 14:43

kaizencc mentioned this pull request Sep 11, 2023

feat(core): Schedule class #27105

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(aws-events): add warning when minute is not defined #19197

chore(aws-events): add warning when minute is not defined #19197

TheRealAmazonKendra commented Mar 1, 2022

gitpod-io bot commented Mar 1, 2022

rix0rrr left a comment

rix0rrr Mar 2, 2022

rix0rrr Mar 2, 2022

TheRealAmazonKendra Mar 8, 2022 •

edited

Loading

rix0rrr Mar 2, 2022

TheRealAmazonKendra Mar 8, 2022

rix0rrr Mar 2, 2022

rix0rrr commented Mar 14, 2022

mergify bot commented Mar 14, 2022

aws-cdk-automation commented Mar 14, 2022

mergify bot commented Mar 14, 2022

	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 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." : undefined;

chore(aws-events): add warning when minute is not defined #19197

chore(aws-events): add warning when minute is not defined #19197

Conversation

TheRealAmazonKendra commented Mar 1, 2022

gitpod-io bot commented Mar 1, 2022

rix0rrr left a comment

Choose a reason for hiding this comment

rix0rrr Mar 2, 2022

Choose a reason for hiding this comment

rix0rrr Mar 2, 2022

Choose a reason for hiding this comment

TheRealAmazonKendra Mar 8, 2022 • edited Loading

Choose a reason for hiding this comment

rix0rrr Mar 2, 2022

Choose a reason for hiding this comment

TheRealAmazonKendra Mar 8, 2022

Choose a reason for hiding this comment

rix0rrr Mar 2, 2022

Choose a reason for hiding this comment

rix0rrr commented Mar 14, 2022

mergify bot commented Mar 14, 2022

aws-cdk-automation commented Mar 14, 2022

AWS CodeBuild CI Report

mergify bot commented Mar 14, 2022

TheRealAmazonKendra Mar 8, 2022 •

edited

Loading