feat(ecs): add support for EC2 Capacity Providers by otterley · Pull Request #14386 · aws/aws-cdk

otterley · 2021-04-27T01:54:17Z

Add support for EC2 Capacity Providers.

I tried to make the UX reasonably decent without disrupting existing stacks. There are some new types being created here, including ICapacityProvider (interface type) and its concrete type EC2CapacityProvider (which is a class users can instantiate). There are also a couple of singleton classes for FargateCapacityProvider and FargateSpotCapacityProvider.

Closes #5471

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

gitpod-io · 2021-04-27T01:54:19Z

SoManyHs

Thanks so much for this contribution! Had a few questions, but otherwise looking really good!

packages/@aws-cdk/aws-ecs/lib/cluster.ts

SoManyHs · 2021-04-30T00:39:56Z

packages/@aws-cdk/aws-ecs/lib/cluster.ts

+      autoScalingGroupProvider: {
+        autoScalingGroupArn: this.autoScalingGroup.autoScalingGroupName,
+        managedScaling: {
+          status: (props.enableManagedScaling === undefined || props.enableManagedScaling) ? 'ENABLED' : 'DISABLED',


What happens if enabledManagedScaling is not enabled but the other managedScaling fields are specified in the props? Could we add a test for that?

I've reworked this code, LMK what you think.

packages/@aws-cdk/aws-ecs/test/ec2/ec2-service.test.ts

packages/@aws-cdk/aws-ecs/test/ecs-cluster.test.ts

gitpod-io · 2021-05-04T17:27:30Z

otterley · 2021-05-11T16:48:50Z

@SoManyHs Ready for second review, thanks!

SoManyHs

Looks great! The high number of comments are mostly renaming EC2 -> Asg as previously discussed -- also a few clarifications on tests.

SoManyHs · 2021-05-11T17:59:32Z

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

 Task auto-scaling is powered by *Application Auto-Scaling*.
 See that section for details.

-## Instance Auto-Scaling


I think we can keep this section to include the bit on scaling on a metric -- this still applies in the context of ASG capacity providers.

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

packages/@aws-cdk/aws-ecs/test/cluster.test.ts

packages/@aws-cdk/aws-ecs/lib/cluster.ts

Co-authored-by: Hsing-Hui Hsu <hsinghui@amazon.com>

otterley · 2021-05-11T23:39:30Z

@SoManyHs Ready for the next review pass!

packages/@aws-cdk/aws-ecs/lib/cluster.ts

SoManyHs

LGTM! Thanks so much! 🚀

Pull request has been modified.

aws-cdk-automation · 2021-05-12T20:49:37Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: 41899a9
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2021-05-12T20:49:50Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

Add support for EC2 Capacity Providers. I tried to make the UX reasonably decent without disrupting existing stacks. There are some new types being created here, including `ICapacityProvider` (interface type) and its concrete type `EC2CapacityProvider` (which is a class users can instantiate). There are also a couple of singleton classes for `FargateCapacityProvider` and `FargateSpotCapacityProvider`. Closes aws#5471 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

The `Cluster.addCapacity` method was deprecated in #14386 as part of the introduction of `Cluster.addAsgCapacityProvider`. However, the corresponding `ClusterProps.capacity` property and `AddCapacityOptions` interface were not deprecated, leading to a confusing mismash of deprecated and undeprecated usage. The README for ECS still heavily references `Cluster.addCapacity`, further leading to potential confusion for users just following the module's guidance. As part of cleaning up deprecated usage as part of the lead-up to the V2 launch, opting to un-deprecate the `addCapacity` method rather than deprecating the other two elements and rewriting the README.

The `Cluster.addCapacity` method was deprecated in #14386 as part of the introduction of `Cluster.addAsgCapacityProvider`. However, the corresponding `ClusterProps.capacity` property and `AddCapacityOptions` interface were not deprecated, leading to a confusing mismash of deprecated and undeprecated usage. The README for ECS still heavily references `Cluster.addCapacity`, further leading to potential confusion for users just following the module's guidance. As part of cleaning up deprecated usage as part of the lead-up to the V2 launch, opting to un-deprecate the `addCapacity` method rather than deprecating the other two elements and rewriting the README. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

The `Cluster.addCapacity` method was deprecated in #14386 as part of the introduction of `Cluster.addAsgCapacityProvider`. However, the corresponding `ClusterProps.capacity` property and `AddCapacityOptions` interface were not deprecated, leading to a confusing mismash of deprecated and undeprecated usage. The README for ECS still heavily references `Cluster.addCapacity`, further leading to potential confusion for users just following the module's guidance. As part of cleaning up deprecated usage as part of the lead-up to the V2 launch, opting to un-deprecate the `addCapacity` method rather than deprecating the other two elements and rewriting the README. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* (cherry picked from commit 765c274)

The `Cluster.addCapacity` method was deprecated in aws#14386 as part of the introduction of `Cluster.addAsgCapacityProvider`. However, the corresponding `ClusterProps.capacity` property and `AddCapacityOptions` interface were not deprecated, leading to a confusing mismash of deprecated and undeprecated usage. The README for ECS still heavily references `Cluster.addCapacity`, further leading to potential confusion for users just following the module's guidance. As part of cleaning up deprecated usage as part of the lead-up to the V2 launch, opting to un-deprecate the `addCapacity` method rather than deprecating the other two elements and rewriting the README. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

github-actions bot added the @aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud label Apr 27, 2021

github-actions bot assigned rix0rrr Apr 27, 2021

SoManyHs self-assigned this Apr 28, 2021

SoManyHs added the @aws-cdk/aws-ecs Related to Amazon Elastic Container label Apr 28, 2021

SoManyHs reviewed Apr 30, 2021

View reviewed changes

SoManyHs added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label May 4, 2021

github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label May 5, 2021

SoManyHs added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label May 5, 2021

SoManyHs mentioned this pull request May 10, 2021

Can't attach CfnAutoScalingGroup to an ECS Cluster #8005

Closed

2 tasks

otterley force-pushed the ecs-ec2-capacity-provider branch 3 times, most recently from 1917213 to 7f6597b Compare May 11, 2021 04:03

github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label May 11, 2021

SoManyHs reviewed May 11, 2021

View reviewed changes

packages/@aws-cdk/aws-ecs/lib/cluster.ts Show resolved Hide resolved

otterley and others added 8 commits May 11, 2021 14:43

feat(ecs): add support for EC2 Capacity Providers

01840f4

Apply suggestions from code review

a31c441

Co-authored-by: Hsing-Hui Hsu <hsinghui@amazon.com>

Update per reviewer feedback

e7fc0bf

Update packages/@aws-cdk/aws-ecs/README.md

1b9a860

Co-authored-by: Hsing-Hui Hsu <hsinghui@amazon.com>

Update packages/@aws-cdk/aws-ecs/README.md

a75f3c6

Co-authored-by: Hsing-Hui Hsu <hsinghui@amazon.com>

Update packages/@aws-cdk/aws-ecs/README.md

287a6d9

Co-authored-by: Hsing-Hui Hsu <hsinghui@amazon.com>

Update packages/@aws-cdk/aws-ecs/README.md

ccb9dc1

Co-authored-by: Hsing-Hui Hsu <hsinghui@amazon.com>

Update packages/@aws-cdk/aws-ecs/test/cluster.test.ts

3627bd9

Co-authored-by: Hsing-Hui Hsu <hsinghui@amazon.com>

otterley force-pushed the ecs-ec2-capacity-provider branch 4 times, most recently from 5876422 to cfb0453 Compare May 11, 2021 23:26

otterley force-pushed the ecs-ec2-capacity-provider branch 5 times, most recently from b3fcb11 to b06f7db Compare May 11, 2021 23:36

SoManyHs reviewed May 12, 2021

View reviewed changes

packages/@aws-cdk/aws-ecs/lib/cluster.ts Outdated Show resolved Hide resolved

Updated to reflect review feedback

ecc9966

otterley force-pushed the ecs-ec2-capacity-provider branch from b06f7db to ecc9966 Compare May 12, 2021 19:08

SoManyHs previously approved these changes May 12, 2021

View reviewed changes

Merge branch 'master' into ecs-ec2-capacity-provider

41899a9

SoManyHs approved these changes May 12, 2021

View reviewed changes

mergify bot merged commit 114f7cc into aws:master May 12, 2021

This was referenced May 13, 2021

ECS Cluster.addCapacity does not set autoscalingGroup property #5893

Closed

fix: fix typo in cluster.ts : supproted -> supported #14600

Closed

SoManyHs mentioned this pull request Jun 9, 2021

feat(ecs): Autoscaling Group Capacity Provider #9192

Closed

njlynch mentioned this pull request Nov 23, 2021

chore(ecs): undeprecate Cluster.addCapacity #17652

Merged

Conversation

otterley commented Apr 27, 2021 • edited by SoManyHs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gitpod-io bot commented Apr 27, 2021

Uh oh!

SoManyHs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SoManyHs Apr 30, 2021

Choose a reason for hiding this comment

Uh oh!

otterley May 10, 2021

Choose a reason for hiding this comment

Uh oh!

otterley May 11, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gitpod-io bot commented May 4, 2021

Uh oh!

otterley commented May 11, 2021

Uh oh!

SoManyHs left a comment

Choose a reason for hiding this comment

Uh oh!

SoManyHs May 11, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

otterley commented May 11, 2021

Uh oh!

Uh oh!

SoManyHs left a comment

Choose a reason for hiding this comment

Uh oh!

aws-cdk-automation commented May 12, 2021

AWS CodeBuild CI Report

Uh oh!

mergify bot commented May 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

otterley commented Apr 27, 2021 •

edited by SoManyHs

Loading