aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.65k stars 3.91k forks source link

Add validation to `Schedule.expression` #7283

Closed zakwalters closed 4 years ago

zakwalters commented 4 years ago

Currently, in events, autoscaling, and application-autoscaling, Schedule.expression will construct a LiteralSchedule from the string expression that is passed. All three Schedule classes have static methods for the specific expression formats they support. Checking that the string expression matches one of these formats and using the corresponding method (or throwing an error if none match) will:

Use Case

I tried to deploy a scheduled event with a cron expression and got an error at deploy time (with a very vague error message) when it could have been caught earlier.

Proposed Solution

Example for the events schedule

   public static expression(expression: string): Schedule {
-    return new LiteralSchedule(expression);
+    if (expression.trim().startsWith("cron")) {
+      return this.cron(cronOptionsFromString(expression));
+    } else if (expression.trim().startsWith("rate")) {
+      return this.rate(durationFromString(expression));
+    } else {
+      throw new Error("Schedule expression for events must in the format 'rate(expression)' or 'cron(expression)'");
+    }
   }

Other

Current error from CloudFormation:

Parameter ScheduleExpression is not valid.

Side note: now that there are more specific methods than Schedule.expression, the lambda-cron example can be updated to use Schedule.cron. It was copying this example that made it possible for me to specify a dodgy cron expression in the first place.


This is a :rocket: Feature Request

rix0rrr commented 4 years ago

The expression() method serves as an escape hatch mechanism, of sorts. If a random(week) expression gets added by the service, users will be able to take advantage of it immediately, and won't be blocked waiting for us to implement a convenience method.

If you're an application builder, you know what kind of pattern you want and you can call the right factory function of Schedule.

If you're middleware, you shouldn't be taking a string parameter, you should be taking a Schedule parameter.

In both cases, no need for this validation.

What use case am I missing?

zakwalters commented 4 years ago

The "escape hatch" use case makes a lot of sense, fair enough - and even if constructing a LiteralSchedule were the default case rather than an error, there's no guarantee that the other cases don't (wrongly) match the new expression format.

It would still be nice to see the lambda cron example updated, but that's in a different repo, so I'll close this.