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.

Sign up for GitHub

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

Jump to bottom

feat(applicationautoscaling): timezone property for scheduled actions #23123

Closed
wants to merge 8 commits into from
Closed

feat(applicationautoscaling): timezone property for scheduled actions #23123

wants to merge 8 commits into from

Conversation

bora-7
Copy link
Contributor

@bora-7 bora-7 commented Nov 28, 2022 •
edited
Loading

Fixes #22645

All Submissions:

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

@gitpod-io
Copy link

gitpod-io bot commented Nov 28, 2022

@github-actions 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 aws-cdk-automation requested a review from a team November 28, 2022 18:41
aws-cdk-automation
aws-cdk-automation previously requested changes Nov 28, 2022
View reviewed changes
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@bora-7 bora-7 marked this pull request as ready for review December 1, 2022 15:15
@aws-cdk-automation aws-cdk-automation dismissed their stale review December 1, 2022 15:16

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

kaizencc
kaizencc previously requested changes Dec 2, 2022
View reviewed changes
Copy link
Contributor

@kaizencc kaizencc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-applicationautoscaling/lib/scalable-target.ts
*
* @default - No specific time zone.
*/
readonly timezone?: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Show resolved Hide resolved
@kaizencc 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
@mergify mergify bot dismissed kaizencc’s stale review December 5, 2022 17:26

Pull request has been modified.

@bora-7
Copy link
Contributor Author

bora-7 commented Dec 5, 2022

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

kaizencc
kaizencc previously requested changes Dec 6, 2022
View reviewed changes
Copy link
Contributor

@kaizencc kaizencc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

packages/@aws-cdk/aws-dynamodb/README.md Outdated Show resolved Hide resolved
@mergify mergify bot dismissed kaizencc’s stale review December 6, 2022 23:23

Pull request has been modified.

@bora-7
Copy link
Contributor Author

bora-7 commented Dec 7, 2022

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
Copy link
Contributor Author

bora-7 commented Dec 7, 2022

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
Copy link
Collaborator

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
Copy link
Contributor

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
Copy link
Contributor

@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
Copy link
Contributor Author

bora-7 commented Dec 22, 2022

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

TheRealAmazonKendra added a commit that referenced this pull request Feb 13, 2023
@TheRealAmazonKendra
chore: add TimeZones class
d3a6fc2 
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.
@TheRealAmazonKendra TheRealAmazonKendra mentioned this pull request Feb 13, 2023
Merged
mergify bot pushed a commit that referenced this pull request Feb 13, 2023
@TheRealAmazonKendra
chore: add TimeZones class (#24127)
99e1fb8 
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
Copy link
Contributor

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

TheRealAmazonKendra
TheRealAmazonKendra requested changes Feb 13, 2023
View reviewed changes
Copy link
Contributor

@TheRealAmazonKendra TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Putting in changes requested now that updates can be made.

@aws-cdk-automation
Copy link
Collaborator

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
Copy link
Collaborator

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.

@aws-cdk-automation 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pahud pahud pahud left review comments

@TheRealAmazonKendra TheRealAmazonKendra TheRealAmazonKendra requested changes

@aws-cdk-automation aws-cdk-automation aws-cdk-automation left review comments

@kaizencc kaizencc kaizencc left review comments

Assignees
No one assigned
Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

aws-applicationautoscaling, aws-autoscaling: Scheduled actions missing support to specify timezone property
5 participants
@bora-7 @aws-cdk-automation @kaizencc @TheRealAmazonKendra @pahud