-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(backup): schedule expression timezone for backup plans #27012
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jogold, I think we should implement this similar to what we do in scheduler-alpha
:
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.
* | ||
* @default - UTC | ||
*/ | ||
readonly scheduleExpressionTimezone?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we export a TimeZone
prop out of core.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missed that one
@@ -213,6 +224,10 @@ export class BackupPlanRule { | |||
throw new Error('`scheduleExpression` must be of type `cron`'); | |||
} | |||
|
|||
if (props.scheduleExpressionTimezone && !props.scheduleExpression) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are some scheduleExpression
s that don't make sense with a timezone, and this check ignores that.
OK, this means deprecating |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Oof. I didn't realize that backup uses I think 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:
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. |
aws-cdk/packages/aws-cdk-lib/aws-backup/lib/rule.ts Lines 212 to 214 in d69c51a
Yeah, it seems to be the right thing to do. |
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? |
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*
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