-
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
Changes from 8 commits
2bc2bff
3d55ebd
fed2265
8f49607
983017c
2218fb2
da105c8
709e5b5
d953753
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -72,4 +72,72 @@ describe('http load balancer', () => { | |
|
||
}); | ||
|
||
test('allows scaling on request count for the HTTP load balancer', () => { | ||
// GIVEN | ||
const stack = new cdk.Stack(); | ||
|
||
// WHEN | ||
const environment = new Environment(stack, 'production'); | ||
const serviceDescription = new ServiceDescription(); | ||
|
||
serviceDescription.add(new Container({ | ||
cpu: 256, | ||
memoryMiB: 512, | ||
trafficPort: 80, | ||
image: ecs.ContainerImage.fromRegistry('nathanpeck/name'), | ||
})); | ||
|
||
serviceDescription.add(new HttpLoadBalancerExtension({ requestsPerTarget: 100 })); | ||
|
||
new Service(stack, 'my-service', { | ||
environment, | ||
serviceDescription, | ||
autoScaleTaskCount: { | ||
maxTaskCount: 5, | ||
}, | ||
}); | ||
|
||
// THEN | ||
expect(stack).toHaveResourceLike('AWS::ApplicationAutoScaling::ScalableTarget', { | ||
MaxCapacity: 5, | ||
MinCapacity: 1, | ||
}); | ||
|
||
expect(stack).toHaveResourceLike('AWS::ApplicationAutoScaling::ScalingPolicy', { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we somehow print that There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes I didn't include a check for |
||
PolicyType: 'TargetTrackingScaling', | ||
TargetTrackingScalingPolicyConfiguration: { | ||
PredefinedMetricSpecification: { | ||
PredefinedMetricType: 'ALBRequestCountPerTarget', | ||
}, | ||
TargetValue: 100, | ||
}, | ||
}); | ||
}); | ||
|
||
test('should error when adding scaling policy if scaling target has not been configured', () => { | ||
// GIVEN | ||
const stack = new cdk.Stack(); | ||
|
||
// WHEN | ||
const environment = new Environment(stack, 'production'); | ||
const serviceDescription = new ServiceDescription(); | ||
|
||
serviceDescription.add(new Container({ | ||
cpu: 256, | ||
memoryMiB: 512, | ||
trafficPort: 80, | ||
image: ecs.ContainerImage.fromRegistry('nathanpeck/name'), | ||
})); | ||
|
||
serviceDescription.add(new HttpLoadBalancerExtension({ requestsPerTarget: 100 })); | ||
|
||
// THEN | ||
expect(() => { | ||
new Service(stack, 'my-service', { | ||
environment, | ||
serviceDescription, | ||
}); | ||
}).toThrow(/Auto scaling target for the service 'my-service' hasn't been configured. Please use Service construct to configure 'minTaskCount' and 'maxTaskCount'./); | ||
}); | ||
|
||
}); |
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. IfautoScaleTaskCount
is undefined then we error out i guess? https://github.com/aws/aws-cdk/pull/17101/files#diff-77a32c641e0209aa8836c64a4f58132a141ad27eab31be695d247e8f4015a8a0R135There 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 questionThere 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 notundefined
and it will be invalid input and not pass the static check i thinkThere 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 👍🏼