fix(applicationautoscaling): add timezone property to scalable target #27052
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
beginning-contributor
aws-cdk-automation
previously requested changes
Sep 7, 2023
There was a problem hiding this comment.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
aws-cdk-automation
added
the
pr/needs-community-review
|
Yep. Going to put this in draft mode and hopefully make a PR for |
aws-cdk-automation
removed
the
pr/needs-community-review
mergify bot
pushed a commit
that referenced
this pull request
Sep 20, 2023
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*
1 task
mergify bot
pushed a commit
that referenced
this pull request
Feb 15, 2024
Closes #22645 Closes #27754 Spiritual successor of #27052 Somewhat related to #21181 but that might be another PR down the road. @pahud ✋ Please review. I'm not particularly fond of how `aws-autoscaling` module ([here](https://github.com/aws/aws-cdk/blob/256cca4017a80f8643c5f5a5999a2ce0383eebf0/packages/aws-cdk-lib/aws-autoscaling/lib/scheduled-action.ts#L21)) is not using `cdk.TimeZone` class, hence why used it in this PR instead. I think we should we change `aws-autoscaling` implementation to do the same? It would be a breaking change... and most likely a brand new PR. LMK what you think. ✌️ Also, I may be slightly OCD but I kinda like better `timezone` vs `timeZone`, but I went with latter one to follow what `aws-autoscaling` did. cc-ing @kaizencc for his input too 🙌 ... possibly related to #27105 ### Reason for this change Timezones have been supported in `AWS::ApplicationAutoScaling::ScalableTarget ScheduledAction` for a while now. https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-applicationautoscaling-scalabletarget-scheduledaction.html#cfn-applicationautoscaling-scalabletarget-scheduledaction-timezone ### Description of changes Just added the support for `timezones` in `scalableTarget.scaleOnSchedule` ### Description of how you validated changes Added unit tests for this feature. ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
GavinZZ
pushed a commit
that referenced
this pull request
Feb 22, 2024
Closes #22645 Closes #27754 Spiritual successor of #27052 Somewhat related to #21181 but that might be another PR down the road. @pahud ✋ Please review. I'm not particularly fond of how `aws-autoscaling` module ([here](https://github.com/aws/aws-cdk/blob/256cca4017a80f8643c5f5a5999a2ce0383eebf0/packages/aws-cdk-lib/aws-autoscaling/lib/scheduled-action.ts#L21)) is not using `cdk.TimeZone` class, hence why used it in this PR instead. I think we should we change `aws-autoscaling` implementation to do the same? It would be a breaking change... and most likely a brand new PR. LMK what you think. ✌️ Also, I may be slightly OCD but I kinda like better `timezone` vs `timeZone`, but I went with latter one to follow what `aws-autoscaling` did. cc-ing @kaizencc for his input too 🙌 ... possibly related to #27105 ### Reason for this change Timezones have been supported in `AWS::ApplicationAutoScaling::ScalableTarget ScheduledAction` for a while now. https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-applicationautoscaling-scalabletarget-scheduledaction.html#cfn-applicationautoscaling-scalabletarget-scheduledaction-timezone ### Description of changes Just added the support for `timezones` in `scalableTarget.scaleOnSchedule` ### Description of how you validated changes Added unit tests for this feature. ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add
timeZoneproperty to ScalableTargethttps://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-applicationautoscaling-scalabletarget-scheduledaction.html#aws-properties-applicationautoscaling-scalabletarget-scheduledaction-properties
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license