feat(ecs-pattens): pass healthy percent & deregistration delay params #28627
Conversation
github-actions
bot
added
beginning-contributor
cheruvian
force-pushed
the
ecs-patterns-props
branch
from
436e55f
to
df9505c
Compare
cheruvian
force-pushed
the
ecs-patterns-props
branch
from
df9505c
to
0cdf6dc
Compare
aws-cdk-automation
added
the
pr/needs-community-review
|
Any thoughts on this now that the build is passing @kylelaker? |
|
Found another one that I need today
Can someone from AWS please advise if |
| * | ||
| * @default - 0 if daemon, otherwise 50 | ||
| */ | ||
| readonly minHealthyPercent?: number; |
There was a problem hiding this comment.
could you add an @see here with a link to the docs?
There was a problem hiding this comment.
also, I think I'd prefer this to be minimumHealthyPercent to match CloudFormation.
There was a problem hiding this comment.
Not clear which docs link you'd prefer (CDK, CF or ECS), chose this: https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_DeploymentConfiguration.html
I also changed the naming per your recommendation to get this approved, however I don't love this since it diverges from the usage/naming used throughout the CDK (already been established in BaseServiceOptions)
https://github.com/aws/aws-cdk/blob/v2.133.0/packages/aws-cdk-lib/aws-ecs/lib/base/base-service.ts#L298
https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_ecs.FargateService.html#minhealthypercent
If CDK should match the CF naming then it should be updated everywhere rather than piecemeal IMO. This also makes it more of a challenge to re-use BaseServiceOptions similar to the above recommendation to just use ApplciationTargetProps. Are there any thoughts on this from the CDK team?
There was a problem hiding this comment.
@cheruvian I didn't see it in BaseServiceOptions. Apologies, you can change it to make that.
There was a problem hiding this comment.
Rest LGTM, if you want to change this back.
There was a problem hiding this comment.
Any thoughts on whether this should actually just extend Omit<BaseServiceOptions, ...>?
cheruvian
force-pushed
the
ecs-patterns-props
branch
from
89f0a89
to
7216c15
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
| * After this period, the cookie is considered stale. The minimum value is | ||
| * 1 second and the maximum value is 7 days (604800 seconds). | ||
| * | ||
| * @default Duration.days(1) |
There was a problem hiding this comment.
This should actually be Stickiness is disabled. See #29726
| targetGroups: [ | ||
| { | ||
| containerPort: 80, | ||
| deregistrationDelay: Duration.seconds(10), | ||
| stickinessCookieDuration: Duration.minutes(4), |
There was a problem hiding this comment.
can you add an integration test for NLB as well?
TheRealAmazonKendra
changed the title
There was a problem hiding this comment.
A couple things here:
Firstly +1 to the comments inline.
Secondly, from the amounts of updates in the integ test files, it looks like they were manually updated and that the test wasn't actually run. Please remove the manual updates and run the test with the --update-on-failed flag and then add all the resulting changes to this PR.
aws-cdk-automation
removed
the
pr/needs-community-review
|
This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week. |
|
This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error. |
aws-cdk-automation
added
the
closed-for-staleness
The
ecs-patternsfor multiple target groups expose a subset of the available properties for each of the lower level constructs form which it is composed. This subset seems to be manually curated today, however it excludes a number of production critical properties.This change adds 2 such important properties that I have needed recently.
minHealthyPercentfromAWS::ECS::ServicederegistrationDelayfromAWS::ElasticLoadBalancingV2::TargetGroup'sTargetGroupAttributesI believe it may be possible/beneficial to operate on a block list rather than allow list of properties and include all the properties of the included child constructs (including docs, etc...) except for those that don't make sense in a multiple target group context. However, it wasn't clear if this change would be approved and as such I have considered this out of scope for this change, if that is acceptable or desired, I propose that as a different / additional scope of work.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license