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: add construct library for Application AutoScaling #933

Merged
merged 11 commits into from
Oct 25, 2018
Merged

feat: add construct library for Application AutoScaling #933

merged 11 commits into from
Oct 25, 2018

Conversation

rix0rrr
Copy link
Contributor

@rix0rrr rix0rrr commented Oct 15, 2018 •
edited
Loading

Adds a library for application autoscaling. Should be consumed and re-exposed in a better way by other services. To start with, the DynamoDB implementation now uses it.

BREAKING CHANGE: The API for enabling AutoScaling for DynamoDB tables has changed.

Fixes #856, #861, #640, #644.

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

@rix0rrr
Copy link
Contributor Author

rix0rrr commented Oct 15, 2018

If you're wondering why this new package doesn't have any integ tests: it's because the dependency arrows would point the wrong way. But the library is integ tested in each individual service package that exposes autoscaling constructs.

@rix0rrr
Copy link
Contributor Author

rix0rrr commented Oct 16, 2018

'nother bug in JSII, looks like:

@aws-cdk/aws-iam: [ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.6.1:compile (default-compile) on project iam: Compilation failure: Compilation failure:
@aws-cdk/aws-iam: [ERROR] /tmp/jsii-pacmak-codeUbHgJB/src/main/java/software/amazon/awscdk/services/iam/IRole.java:[26,69] variable _principal is already defined in class software.amazon.awscdk.services.iam.IRole.Builder
@aws-cdk/aws-iam: [ERROR] /tmp/jsii-pacmak-codeUbHgJB/src/main/java/software/amazon/awscdk/services/iam/IRole.java:[52,24] method withPrincipal(software.amazon.awscdk.services.iam.PolicyPrincipal) is already defined in class

@rix0rrr
Copy link
Contributor Author

rix0rrr commented Oct 16, 2018

Blocked by aws/jsii#264

@eladb
Copy link
Contributor

eladb commented Oct 16, 2018

Can you work around this somehow? Perhaps add a fake method to the interface so it's not identified as a datatype?

@rix0rrr
Copy link
Contributor Author

rix0rrr commented Oct 16, 2018

Blocked on aws/jsii#265

@rix0rrr
Copy link
Contributor Author

rix0rrr commented Oct 16, 2018

Can you work around this somehow? Perhaps add a fake method to the interface so it's not identified as a datatype?

I could, but I don't really want to. I'd rather fix the root cause.

@rix0rrr
Copy link
Contributor Author

rix0rrr commented Oct 16, 2018

Also fixes #673.

jungseoklee
jungseoklee reviewed Oct 16, 2018
View reviewed changes
Copy link
Contributor

@jungseoklee jungseoklee left a comment

Choose a reason for hiding this comment

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

Great thanks for the work!!

I really like this change. Only a single comment on IAM role from functionality perspective.

packages/@aws-cdk/aws-applicationautoscaling/lib/scalable-target.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-applicationautoscaling/README.md Show resolved Hide resolved
packages/@aws-cdk/aws-applicationautoscaling/README.md Show resolved Hide resolved
packages/@aws-cdk/aws-applicationautoscaling/lib/scalable-target.ts Show resolved Hide resolved
packages/@aws-cdk/aws-applicationautoscaling/lib/target-tracking-scaling-policy.ts Show resolved Hide resolved
@rix0rrr rix0rrr self-assigned this Oct 23, 2018
RomainMuller
RomainMuller approved these changes Oct 23, 2018
View reviewed changes
Copy link
Contributor

@RomainMuller RomainMuller left a comment

Choose a reason for hiding this comment

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

Some minor stuff, but that's pretty good overall. Quite a pice of work! 🤙🏻

packages/@aws-cdk/aws-applicationautoscaling/lib/base-scalable-attribute.ts Show resolved Hide resolved
packages/@aws-cdk/aws-applicationautoscaling/lib/cron.ts Show resolved Hide resolved
packages/@aws-cdk/aws-applicationautoscaling/lib/cron.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-applicationautoscaling/lib/scalable-target.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-applicationautoscaling/lib/scalable-target.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-applicationautoscaling/lib/scalable-target.ts
/**
* When to perform this action.
*
* Support formats:
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we not type those? Feels like the expression domain is pretty tight and we could use real language types instead of strings...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yyyyeaaahhh... I was going back and forth on that. This has the advantage of being simple (as in: being close to what people expect and works the same as the other APIs). My plan to paper over that was to make helper classes that will generate this string for you. Seems better than to force people to instantiate a class if they already know what string they want?

packages/@aws-cdk/aws-applicationautoscaling/package.json Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-applicationautoscaling/package.json Outdated Show resolved Hide resolved
@rix0rrr rix0rrr merged commit 7861c6f into master Oct 25, 2018
@rix0rrr rix0rrr deleted the huijbers/application-autoscaling branch October 25, 2018 12:11
@eladb
Copy link
Contributor

eladb commented Oct 25, 2018

Congratulations! 🎉 beautiful design!

rix0rrr pushed a commit that referenced this pull request Oct 26, 2018
v0.14.0
5a21488 
__IMPORTANT NOTE__: when upgrading to this version of the CDK framework, you must also upgrade
your installation the CDK Toolkit to the matching version:

```shell
$ npm i -g aws-cdk
$ cdk --version
0.14.0 (build ...)
```

Bug Fixes
=========

* remove CloudFormation property renames ([#973](#973)) ([3f86603](3f86603)), closes [#852](#852)
* **aws-ec2:** fix retention of all egress traffic rule ([#998](#998)) ([b9d5b43](b9d5b43)), closes [#987](#987)
* **aws-s3-deployment:** avoid deletion during update using physical ids ([#1006](#1006)) ([bca99c6](bca99c6)), closes [#981](#981) [#981](#981)
* **cloudformation-diff:** ignore changes to DependsOn ([#1005](#1005)) ([3605f9c](3605f9c)), closes [#274](#274)
* **cloudformation-diff:** track replacements ([#1003](#1003)) ([a83ac5f](a83ac5f)), closes [#1001](#1001)
* **docs:** fix EC2 readme for "natgatway" configuration ([#994](#994)) ([0b1e7cc](0b1e7cc))
* **docs:** updates to contribution guide ([#997](#997)) ([b42e742](b42e742))
* **iam:** Merge multiple principals correctly ([#983](#983)) ([3fc5c8c](3fc5c8c)), closes [#924](#924) [#916](#916) [#958](#958)

Features
=========

* add construct library for Application AutoScaling ([#933](#933)) ([7861c6f](7861c6f)), closes [#856](#856) [#861](#861) [#640](#640) [#644](#644)
* add HostedZone context provider ([#823](#823)) ([1626c37](1626c37))
* **assert:** haveResource lists failing properties ([#1016](#1016)) ([7f6f3fd](7f6f3fd))
* **aws-cdk:** add CDK app version negotiation ([#988](#988)) ([db4e718](db4e718)), closes [#891](#891)
* **aws-codebuild:** Introduce a CodePipeline test Action. ([#873](#873)) ([770f9aa](770f9aa))
* **aws-sqs:** Add grantXxx() methods ([#1004](#1004)) ([8c90350](8c90350))
* **core:** Pre-concatenate Fn::Join ([#967](#967)) ([33c32a8](33c32a8)), closes [#916](#916) [#958](#958)

BREAKING CHANGES
=========

* DynamoDB AutoScaling: Instead of `addReadAutoScaling()`, call `autoScaleReadCapacity()`, and similar for write scaling.
* CloudFormation resource usage: If you use L1s, you may need to change some `XxxName` properties back into `Name`. These will match the CloudFormation property names.
* You must use the matching `aws-cdk` toolkit when upgrading to this version, or context providers will cease to work. All existing cached context values in `cdk.json` will be invalidated and refreshed.
@rix0rrr rix0rrr mentioned this pull request Oct 26, 2018
Merged
rix0rrr added a commit that referenced this pull request Oct 26, 2018
@rix0rrr
v0.14.0 (#1021)
e283711 
__IMPORTANT NOTE__: when upgrading to this version of the CDK framework, you must also upgrade
your installation the CDK Toolkit to the matching version:

```shell
$ npm i -g aws-cdk
$ cdk --version
0.14.0 (build ...)
```

Bug Fixes
=========

* remove CloudFormation property renames ([#973](#973)) ([3f86603](3f86603)), closes [#852](#852)
* **aws-ec2:** fix retention of all egress traffic rule ([#998](#998)) ([b9d5b43](b9d5b43)), closes [#987](#987)
* **aws-s3-deployment:** avoid deletion during update using physical ids ([#1006](#1006)) ([bca99c6](bca99c6)), closes [#981](#981) [#981](#981)
* **cloudformation-diff:** ignore changes to DependsOn ([#1005](#1005)) ([3605f9c](3605f9c)), closes [#274](#274)
* **cloudformation-diff:** track replacements ([#1003](#1003)) ([a83ac5f](a83ac5f)), closes [#1001](#1001)
* **docs:** fix EC2 readme for "natgatway" configuration ([#994](#994)) ([0b1e7cc](0b1e7cc))
* **docs:** updates to contribution guide ([#997](#997)) ([b42e742](b42e742))
* **iam:** Merge multiple principals correctly ([#983](#983)) ([3fc5c8c](3fc5c8c)), closes [#924](#924) [#916](#916) [#958](#958)

Features
=========

* add construct library for Application AutoScaling ([#933](#933)) ([7861c6f](7861c6f)), closes [#856](#856) [#861](#861) [#640](#640) [#644](#644)
* add HostedZone context provider ([#823](#823)) ([1626c37](1626c37))
* **assert:** haveResource lists failing properties ([#1016](#1016)) ([7f6f3fd](7f6f3fd))
* **aws-cdk:** add CDK app version negotiation ([#988](#988)) ([db4e718](db4e718)), closes [#891](#891)
* **aws-codebuild:** Introduce a CodePipeline test Action. ([#873](#873)) ([770f9aa](770f9aa))
* **aws-sqs:** Add grantXxx() methods ([#1004](#1004)) ([8c90350](8c90350))
* **core:** Pre-concatenate Fn::Join ([#967](#967)) ([33c32a8](33c32a8)), closes [#916](#916) [#958](#958)

BREAKING CHANGES
=========

* DynamoDB AutoScaling: Instead of `addReadAutoScaling()`, call `autoScaleReadCapacity()`, and similar for write scaling.
* CloudFormation resource usage: If you use L1s, you may need to change some `XxxName` properties back into `Name`. These will match the CloudFormation property names.
* You must use the matching `aws-cdk` toolkit when upgrading to this version, or context providers will cease to work. All existing cached context values in `cdk.json` will be invalidated and refreshed.
jonparker pushed a commit to jonparker/aws-cdk that referenced this pull request Oct 29, 2018
@rix0rrr @jonparker
v0.14.0 (aws#1021)
1846741 
__IMPORTANT NOTE__: when upgrading to this version of the CDK framework, you must also upgrade
your installation the CDK Toolkit to the matching version:

```shell
$ npm i -g aws-cdk
$ cdk --version
0.14.0 (build ...)
```

Bug Fixes
=========

* remove CloudFormation property renames ([aws#973](aws#973)) ([3f86603](aws@3f86603)), closes [aws#852](aws#852)
* **aws-ec2:** fix retention of all egress traffic rule ([aws#998](aws#998)) ([b9d5b43](aws@b9d5b43)), closes [aws#987](aws#987)
* **aws-s3-deployment:** avoid deletion during update using physical ids ([aws#1006](aws#1006)) ([bca99c6](aws@bca99c6)), closes [aws#981](aws#981) [aws#981](aws#981)
* **cloudformation-diff:** ignore changes to DependsOn ([aws#1005](aws#1005)) ([3605f9c](aws@3605f9c)), closes [aws#274](aws#274)
* **cloudformation-diff:** track replacements ([aws#1003](aws#1003)) ([a83ac5f](aws@a83ac5f)), closes [aws#1001](aws#1001)
* **docs:** fix EC2 readme for "natgatway" configuration ([aws#994](aws#994)) ([0b1e7cc](aws@0b1e7cc))
* **docs:** updates to contribution guide ([aws#997](aws#997)) ([b42e742](aws@b42e742))
* **iam:** Merge multiple principals correctly ([aws#983](aws#983)) ([3fc5c8c](aws@3fc5c8c)), closes [aws#924](aws#924) [aws#916](aws#916) [aws#958](aws#958)

Features
=========

* add construct library for Application AutoScaling ([aws#933](aws#933)) ([7861c6f](aws@7861c6f)), closes [aws#856](aws#856) [aws#861](aws#861) [aws#640](aws#640) [aws#644](aws#644)
* add HostedZone context provider ([aws#823](aws#823)) ([1626c37](aws@1626c37))
* **assert:** haveResource lists failing properties ([aws#1016](aws#1016)) ([7f6f3fd](aws@7f6f3fd))
* **aws-cdk:** add CDK app version negotiation ([aws#988](aws#988)) ([db4e718](aws@db4e718)), closes [aws#891](aws#891)
* **aws-codebuild:** Introduce a CodePipeline test Action. ([aws#873](aws#873)) ([770f9aa](aws@770f9aa))
* **aws-sqs:** Add grantXxx() methods ([aws#1004](aws#1004)) ([8c90350](aws@8c90350))
* **core:** Pre-concatenate Fn::Join ([aws#967](aws#967)) ([33c32a8](aws@33c32a8)), closes [aws#916](aws#916) [aws#958](aws#958)

BREAKING CHANGES
=========

* DynamoDB AutoScaling: Instead of `addReadAutoScaling()`, call `autoScaleReadCapacity()`, and similar for write scaling.
* CloudFormation resource usage: If you use L1s, you may need to change some `XxxName` properties back into `Name`. These will match the CloudFormation property names.
* You must use the matching `aws-cdk` toolkit when upgrading to this version, or context providers will cease to work. All existing cached context values in `cdk.json` will be invalidated and refreshed.
@NGL321 NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jungseoklee jungseoklee jungseoklee left review comments

@RomainMuller RomainMuller RomainMuller approved these changes

@eladb eladb Awaiting requested review from eladb

Assignees

@rix0rrr rix0rrr

Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Musings on AutoScaling
5 participants
@rix0rrr @eladb @RomainMuller @jungseoklee @NGL321