Skip to content
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

(applicationautoscaling): Support timezone on ScalableTarget.scaleOnSchedule #27754

Closed
2 tasks
tl24 opened this issue Oct 30, 2023 · 5 comments · Fixed by #29116
Closed
2 tasks

(applicationautoscaling): Support timezone on ScalableTarget.scaleOnSchedule #27754

tl24 opened this issue Oct 30, 2023 · 5 comments · Fixed by #29116
Labels
@aws-cdk/aws-applicationautoscaling Related to AWS Application Auto Scaling effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2

Comments

@tl24
Copy link

tl24 commented Oct 30, 2023

Describe the feature

Cloudformation ScalableTarget ScheduledAction supports a timezone property. The timezone argument is not available when calling ScalableTarget.scaleOnSchedule:
https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_applicationautoscaling.ScalableTarget.html#scalewbronwbrscheduleid-action
https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_applicationautoscaling.ScalingSchedule.html

Use Case

Cron time expressions have to be entered in UTC. For timezones observing Daylight Savings Time you have to adjust the schedule when DST starts and again when it ends to get proper scheduling.

Proposed Solution

Add the timezone property to the ScalingSchedule type that is passed to scaleOnSchedule

Other Information

I am using the python library, so I would have no way to add this to the json object for ScalingSchedule like you could in JS/TS since the autogenerated code turns these into individual arguments to the scaleOnSchedule call.

Acknowledgements

  • I may be able to implement this feature request
  • This feature might incur a breaking change

CDK version used

2.103.1

Environment details (OS name and version, etc.)

Ubuntu 20.04

@tl24 tl24 added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Oct 30, 2023
@github-actions github-actions bot added the @aws-cdk/aws-applicationautoscaling Related to AWS Application Auto Scaling label Oct 30, 2023
@msambol
Copy link
Contributor

msambol commented Oct 30, 2023

I'll take this.

@msambol
Copy link
Contributor

msambol commented Oct 30, 2023

On second thought, I'm going to wait for the CDK team to comment. There are definitely some assumptions made regarding UTC:

function formatISO(date?: Date) {
if (!date) { return undefined; }
return date.getUTCFullYear() +
'-' + pad(date.getUTCMonth() + 1) +
'-' + pad(date.getUTCDate()) +
'T' + pad(date.getUTCHours()) +
':' + pad(date.getUTCMinutes()) +
':' + pad(date.getUTCSeconds());
function pad(num: number) {
if (num < 10) {
return '0' + num;
}
return num;
}
}

@pahud pahud added p2 effort/medium Medium work item – several days of effort and removed needs-triage This issue or PR still needs to be triaged. labels Oct 31, 2023
@sothawo
Copy link

sothawo commented Jan 15, 2024

This is an old topic, there was some attempt mentioned in #22645 but up to date still no support for this. Sometimes it seems cdk is the ugly sibling of cfn that noone wants do deal with. Eventbridge seems to have the same issue #21181.

Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

1 similar comment
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

GavinZZ pushed a commit that referenced this issue 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
Labels
@aws-cdk/aws-applicationautoscaling Related to AWS Application Auto Scaling effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants