-
Notifications
You must be signed in to change notification settings - Fork 4k
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
feat(ecs-service-extensions): Target tracking policies for Service Extensions #17101
Conversation
packages/@aws-cdk-containers/ecs-service-extensions/lib/extensions/http-load-balancer.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk-containers/ecs-service-extensions/lib/extensions/http-load-balancer.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk-containers/ecs-service-extensions/lib/service.ts
Outdated
Show resolved
Hide resolved
}); | ||
|
||
// THEN | ||
expect(stack).toHaveResourceLike('AWS::ApplicationAutoScaling::ScalingPolicy', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we somehow print that maxTaskCount
is 5 here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed probably can't tell from this resource but the other ones like ecs service
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I meant to say can we have an expect statement to print that maxTaskCount
is 5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm the unit test is usually meant to check if the CFN template we generate is expected, instead of if the construct itself. maxTaskCount
is a field of the construct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I didn't include a check for maxCapacity
of the ScalableTarget
here because we configure it in the service construct but it makes sense to add a check for it here as well! Will update!
new Service(stack, 'my-service', { | ||
environment, | ||
serviceDescription, | ||
autoScaleTaskCount: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if we send autoScaleTaskCount: {}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maxTaskCount
is a required field. If autoScaleTaskCount
is undefined then we error out i guess? https://github.com/aws/aws-cdk/pull/17101/files#diff-77a32c641e0209aa8836c64a4f58132a141ad27eab31be695d247e8f4015a8a0R135
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But does it consider {}
as undefined is the question
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{}
is not undefined
and it will be invalid input and not pass the static check i think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm that makes sense 👍🏼
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me, just have couple of questions. 🎉
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). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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). |
…tensions (#17101) ---- This PR adds `desiredCount`, `targetCpuUtilization` and `targetMemoryUtilization` to the service construct. It also adds `requestsPerTarget` to the `HttpLoadBalancerExtension` props to allow adding target tracking policy based on the ALB request count. It will be followed by another PR to configure queue auto scaling for the SQS Queues in the `QueueExtension`. *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
…ension (#17802) ---- This PR deprecates one of the existing `scaleOnCpuUtilization` extension. We recommend users to configure task auto scaling using the [`autoScaleTaskCount`](https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk-containers/ecs-service-extensions/lib/service.ts#L61) in the `Service` construct. Related PR: #17101 *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
…tensions (aws#17101) ---- This PR adds `desiredCount`, `targetCpuUtilization` and `targetMemoryUtilization` to the service construct. It also adds `requestsPerTarget` to the `HttpLoadBalancerExtension` props to allow adding target tracking policy based on the ALB request count. It will be followed by another PR to configure queue auto scaling for the SQS Queues in the `QueueExtension`. *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
…ension (aws#17802) ---- This PR deprecates one of the existing `scaleOnCpuUtilization` extension. We recommend users to configure task auto scaling using the [`autoScaleTaskCount`](https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk-containers/ecs-service-extensions/lib/service.ts#L61) in the `Service` construct. Related PR: aws#17101 *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
This PR adds
desiredCount
,targetCpuUtilization
andtargetMemoryUtilization
to the service construct. It also addsrequestsPerTarget
to theHttpLoadBalancerExtension
props to allow adding target tracking policy based on the ALB request count.It will be followed by another PR to configure queue auto scaling for the SQS Queues in the
QueueExtension
.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license