feat(ecs): add availabilityZoneRebalancing property to EcsService#32259
feat(ecs): add availabilityZoneRebalancing property to EcsService#32259ren-yamanashi wants to merge 3 commits intoaws:mainfrom
availabilityZoneRebalancing property to EcsService#32259Conversation
github-actions
bot
added
the
repeat-contributor
…vailability-zone-rebalancing
aws-cdk-automation
left a comment
aws-cdk-automation
left a comment
There was a problem hiding this comment.
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.
A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.
ren-yamanashi
changed the title
availabilityZoneRebalancing property to EcsServiceavailabilityZoneRebalancing property to EcsService
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #32259 +/- ##
=======================================
Coverage 77.17% 77.17%
=======================================
Files 105 105
Lines 7169 7169
Branches 1315 1315
=======================================
Hits 5533 5533
Misses 1455 1455
Partials 181 181
Flags with carried forward coverage won't be shown. Click here to find out more.
|
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
|
||
| /** | ||
| * Indicates whether to use Availability Zone rebalancing for the service | ||
| * | ||
| * For more information, see [Amazon ECS Service Rebalancing](https://docs.aws.amazon.com/AmazonECS/latest/developerguide/service-rebalancing.html) | ||
| * @default - DISABLED | ||
| */ | ||
| readonly availabilityZoneRebalancing?: AvailabilityZoneRebalancing; |
There was a problem hiding this comment.
I don't think it's correct to put it here. You can't use this feature with EXTERNAL services, and ExternalService extends BaseService. I think it should be a parameter of FargateService and Ec2Service instead.
| Not available for the following: | ||
|
|
||
| - Use the `Daemon` strategy | ||
| - Use the `EXTERNAL` boot type (ECS Anywhere) | ||
| - Uses 100% for the `maximumPercent` value. | ||
| - Uses a Classic Load Balancer | ||
| - Uses the `attribute:ecs.availability-zone` as a task placement constraint |
There was a problem hiding this comment.
Can't we validate these things and throw at synth time?
|
Please link to #32226. |
|
I asked you those questions because I was partway through implementing this myself when I saw this PR. I decided to keep going and opened #32263. |
Thank you for your comment. I apologies, I missed the relevant issue #32226 After reviewing your comment and #32263, I believe that #32263 is more appropriate and will close this MR. |
|
Comments on closed issues and PRs are hard for our team to see. |
3 participants
Reason for this change
We can set
availabilityZoneRebalancingfor a Service from Ecs, but this was not supported in the AWS CDK L2 construct.Description of changes
Add
availabilityZoneRebalancingproperty to BaseServiceOptions and set in the CfnServiceDescription of how you validated changes
Added both unit and integration tests.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license