feat(backup): schedule expression timezone for backup plans by jogold · Pull Request #27012 · aws/aws-cdk

jogold · 2023-09-05T13:36:12Z

Add timezone support for schedule expressions in backup plans.

See https://aws.amazon.com/about-aws/whats-new/2023/08/aws-backup-local-time-zone-selections/

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

kaizencc

Hi @jogold, I think we should implement this similar to what we do in scheduler-alpha:

https://github.com/aws/aws-cdk/blob/main/packages/@aws-cdk/aws-scheduler-alpha/lib/schedule-expression.ts

Of course, I still don't understand why we don't just invest time in getting the schedule class into core and I know you've tried exactly that before.

kaizencc · 2023-09-05T13:46:56Z

packages/aws-cdk-lib/aws-backup/lib/rule.ts

+   *
+   * @default - UTC
+   */
+  readonly scheduleExpressionTimezone?: string;


we export a TimeZone prop out of core.

missed that one

kaizencc · 2023-09-05T13:47:45Z

packages/aws-cdk-lib/aws-backup/lib/rule.ts

      throw new Error('`scheduleExpression` must be of type `cron`');
    }

+    if (props.scheduleExpressionTimezone && !props.scheduleExpression) {


there are some scheduleExpressions that don't make sense with a timezone, and this check ignores that.

jogold · 2023-09-05T13:55:57Z

Hi @jogold, I think we should implement this similar to what we do in scheduler-alpha:

main/packages/@aws-cdk/aws-scheduler-alpha/lib/schedule-expression.ts

Of course, I still don't understand why we don't just invest time in getting the schedule class into core and I know you've tried exactly that before.

OK, this means deprecating scheduleExpression in favor of schedule? cronExpression? other name? and you don't want to import the ScheduleExpression class of aws-scheduler-alpha in here?

aws-cdk-automation · 2023-09-05T14:01:58Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 03c186d
Result: FAILED
Build Logs (available for 30 days)

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

kaizencc · 2023-09-05T14:47:16Z

Oof. I didn't realize that backup uses events.Schedule, which itself looks like its being deprecated in favor of AWS EventBridge Scheduler (or at least is pointing everyone in that direction): https://docs.aws.amazon.com/eventbridge/latest/userguide/eb-create-rule-schedule.html. Of course, events.Schedule doesn't support timezone.

I think backup uses events.Schedule because of convenience. There's nothing really tying it to events besides wanting to reuse code. What's more, the docs for backup says that scheduleExpression only accepts CRON expressions so I wonder what happens if you supply events.Schedule.rate('1 minute'). Might be a bug?

I think this necessitates a hard look at how CDK does scheduling and likely means putting a Schedule class in core once and for all. Here's what I think we should do:

create an abstract Schedule in core, with all methods protected. idea is that each module that utilizes a schedule can implement it and expose the APIs that it supports.
in backup, deprecate scheduleExpression in favor of schedule, and create a new backup.Schedule class that implements core.Schedule
in all other modules that use schedule, we refactor the implementation to use core.Schedule and ideally there are no breaking changes here. this might be ambitious but i can't think off the top of my head if there are schedules that wouldn't be refactored cleanly.

I hate this and want to fix it once and for all. What do you think @jogold? If you agree, I can spend some time trying to get the first part right and that should kick start the other changes.

jogold · 2023-09-05T15:02:53Z

that scheduleExpression only accepts CRON expressions so I wonder what happens if you supply events.Schedule.rate('1 minute').

aws-cdk/packages/aws-cdk-lib/aws-backup/lib/rule.ts

Lines 212 to 214 in d69c51a

    
           if (props.scheduleExpression && !/^cron/.test(props.scheduleExpression.expressionString)) { 
        
             throw new Error('`scheduleExpression` must be of type `cron`'); 
        
           }

I hate this and want to fix it once and for all. What do you think @jogold? If you agree, I can spend some time trying to get the first part right and that should kick start the other changes.

Yeah, it seems to be the right thing to do.

kaizencc · 2023-09-05T15:13:45Z

Oh my that's a horrible interface in backup. What we're talking about seems like it will be an improvement across the board. Give me till the end of the week to stand up a PR to put abstract schedule in core?

jogold · 2023-09-13T12:48:56Z

Closing in favor of #27105

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*

jogold and others added 4 commits August 31, 2023 12:51

feat(backup): schedule expression timezone

7b12a28

Merge branch 'aws:main' into backup-tz

35e7b56

tests

cba1471

README

03c186d

github-actions bot added p2 distinguished-contributor [Pilot] contributed 50+ PRs to the CDK labels Sep 5, 2023

aws-cdk-automation requested a review from a team September 5, 2023 13:36

jogold changed the title ~~feat(backup): schedule expression timezone~~ feat(backup): schedule expression timezone for backup plans Sep 5, 2023

kaizencc requested changes Sep 5, 2023

View reviewed changes

jogold mentioned this pull request Sep 8, 2023

fix(applicationautoscaling): add timezone property to scalable target #27052

Closed

kaizencc mentioned this pull request Sep 11, 2023

feat(core): Schedule class #27105

Merged

jogold closed this Sep 13, 2023

jogold deleted the backup-tz branch September 13, 2023 12:49

github-actions bot mentioned this pull request Oct 1, 2023

Monthly PRs metrics report - Sep 2023 #27368

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(backup): schedule expression timezone for backup plans#27012

feat(backup): schedule expression timezone for backup plans#27012
jogold wants to merge 4 commits intoaws:mainfrom
jogold:backup-tz

jogold commented Sep 5, 2023

Uh oh!

kaizencc left a comment

Uh oh!

kaizencc Sep 5, 2023

Uh oh!

jogold Sep 5, 2023

Uh oh!

kaizencc Sep 5, 2023

Uh oh!

jogold commented Sep 5, 2023

Uh oh!

aws-cdk-automation commented Sep 5, 2023

Uh oh!

kaizencc commented Sep 5, 2023

Uh oh!

jogold commented Sep 5, 2023

Uh oh!

kaizencc commented Sep 5, 2023

Uh oh!

jogold commented Sep 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

jogold commented Sep 5, 2023

Uh oh!

kaizencc left a comment

Choose a reason for hiding this comment

Uh oh!

kaizencc Sep 5, 2023

Choose a reason for hiding this comment

Uh oh!

jogold Sep 5, 2023

Choose a reason for hiding this comment

Uh oh!

kaizencc Sep 5, 2023

Choose a reason for hiding this comment

Uh oh!

jogold commented Sep 5, 2023

Uh oh!

aws-cdk-automation commented Sep 5, 2023

AWS CodeBuild CI Report

Uh oh!

kaizencc commented Sep 5, 2023

Uh oh!

jogold commented Sep 5, 2023

Uh oh!

kaizencc commented Sep 5, 2023

Uh oh!

jogold commented Sep 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants