feat(applicationautoscaling): timezone property for scheduled actions by bora-7 · Pull Request #23123 · aws/aws-cdk

bora-7 · 2022-11-28T18:40:53Z

Fixes #22645

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

… aws-applciationautoscaling

gitpod-io · 2022-11-28T18:41:04Z

aws-cdk-automation

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.

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

kaizencc

Hi @bora-7, thanks for contributing this! I have a few concerns regarding the typing of timezone as described in the comments.

packages/@aws-cdk/aws-applicationautoscaling/lib/scalable-target.ts

kaizencc · 2022-12-02T15:13:28Z

packages/@aws-cdk/aws-applicationautoscaling/lib/scalable-target.ts

+   *
+   * @default - No specific time zone.
+   */
+  readonly timezone?: string;


I don't necessarily love typing this as a string, because I think we can do better, but something similar was already introduced in #17330. Can we at least have the documentation explicitly state what kind of string we're looking for here, like we do in #17330?

packages/@aws-cdk/aws-applicationautoscaling/test/scalable-target.test.ts

Pull request has been modified.

bora-7 · 2022-12-05T18:16:32Z

Hi @kaizencc , I've updated the description. Does it make more sense now?

kaizencc

Hi @bora-7! One small nit and I think this is good to go. Thanks!

packages/@aws-cdk/aws-dynamodb/README.md

Pull request has been modified.

bora-7 · 2022-12-07T10:08:37Z

Hi @kaizencc , I've modified the readme to add the code snippet, and now the build fails. Do you know why? Thanks for all your help.

bora-7 · 2022-12-07T19:20:31Z

I fixed the build error now! @kaizencc let me know if it looks good or if there's any additional changes you want me to add.

aws-cdk-automation · 2022-12-07T19:44:39Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: f28889c
Result: SUCCEEDED
Build Logs (available for 30 days)

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

kaizencc · 2022-12-12T23:24:07Z

Hi @bora-7! sorry for the delay. I need to think a bit more regarding the typing of timezones as a string. That might be the simplest path forward, but maybe not the best. I'm going to bring it up to the team and see, so please give me a few more days.

kaizencc · 2022-12-20T16:53:33Z

@bora-7 update: we discussed this internally and think there is value to have a timezone enum-like class in core. I hope to implement this this week and then we can update this PR to use that. Sorry for the delay -- hoping that we can deliver a strong API design for our users :).

bora-7 · 2022-12-22T10:55:25Z

Hi @kaizencc , that sounds good. Let me know when the timezone enum-like class is out and I'll work on updating the PR!

This adds the time zones in the 2022g release of the Time Zone Database. It has been tested in the context of the tests written in #23123, but I didn't commit them here because I didn't want to overwrite that change.

This adds the time zones in the 2022g release of the Time Zone Database. It has been tested in the context of the tests written in #23123, but I didn't commit them here because I didn't want to overwrite that change. I couldn't decide whether this should be a chore or a feat. I'm open to either. Closes #<issue number here>. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

TheRealAmazonKendra · 2023-02-13T17:44:28Z

TimeZone class is ready to go! See the linked PR.

TheRealAmazonKendra

Putting in changes requested now that updates can be made.

aws-cdk-automation · 2023-03-07T00:09:27Z

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

aws-cdk-automation · 2023-03-15T00:08:45Z

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

bora-7 added 4 commits November 21, 2022 18:01

add timezone property

07f9df7

add test to see if timezone property works

6c773b7

integration test to dynamodb, where we check for timezone property in…

1332fbc

… aws-applciationautoscaling

update README

d730de0

github-actions bot added effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels Nov 28, 2022

aws-cdk-automation requested a review from a team November 28, 2022 18:41

aws-cdk-automation previously requested changes Nov 28, 2022

View reviewed changes

added snapshot

b914e06

bora-7 marked this pull request as ready for review December 1, 2022 15:15

kaizencc previously requested changes Dec 2, 2022

View reviewed changes

kaizencc changed the title ~~feat(aws-applicationautoscaling): add option to specify timezone property to scheduled actions~~ feat(applicationautoscaling): timezone property for scheduled actions Dec 2, 2022

update description of timezone

ab5c749

kaizencc previously requested changes Dec 6, 2022

View reviewed changes

packages/@aws-cdk/aws-dynamodb/README.md Outdated Show resolved Hide resolved

update the test and write it in md

9972fbf

update tests

f28889c

TheRealAmazonKendra mentioned this pull request Feb 13, 2023

chore: add TimeZones class #24127

Merged

TheRealAmazonKendra requested changes Feb 13, 2023

View reviewed changes

aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Mar 15, 2023

aws-cdk-automation closed this Mar 15, 2023

Conversation

bora-7 commented Nov 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

All Submissions:

Adding new Unconventional Dependencies:

New Features

Uh oh!

gitpod-io bot commented Nov 28, 2022

Uh oh!

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Uh oh!

kaizencc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kaizencc Dec 2, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bora-7 commented Dec 5, 2022

Uh oh!

kaizencc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bora-7 commented Dec 7, 2022

Uh oh!

bora-7 commented Dec 7, 2022

Uh oh!

aws-cdk-automation commented Dec 7, 2022

AWS CodeBuild CI Report

Uh oh!

kaizencc commented Dec 12, 2022

Uh oh!

kaizencc commented Dec 20, 2022

Uh oh!

bora-7 commented Dec 22, 2022

Uh oh!

TheRealAmazonKendra commented Feb 13, 2023

Uh oh!

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

Uh oh!

aws-cdk-automation commented Mar 7, 2023

Uh oh!

aws-cdk-automation commented Mar 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bora-7 commented Nov 28, 2022 •

edited

Loading