feat(ecs-patterns) expose multiple target groups, listeners, and load balancers by michaeldrey · Pull Request #20770 · aws/aws-cdk

michaeldrey · 2022-06-16T23:16:21Z

This commit will expose targetGroups, listeners, and loadBalancers properties allowing developers access to those resources.

The downside to this is that now we have targetGroups, targetGroup, listeners, listener, and loadBalancers, loadBalancer exposes as a property. In my opinion this can cause confusion for developers.

** Potential breaking change **
When trying to protect targetGroup, listener, and loadBalancer I discovered that this would cause breaking changes on down developers who use them to reference node.Id and other attributes.

Only thing I could do was mark them as deprecated as you get the same functionality on the new properties. Open to feedback on this approach and will update the README based on the direction we go.

I need to update the network Integ tests some more, but looking for feedback on the above

closes #14735

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

gitpod-io · 2022-06-16T23:16:28Z

TheRealAmazonKendra · 2022-08-01T19:38:03Z

So, I have a couple things that I need clarification on. I keep coming back to this PR and feeling unsure of how to respond because I don't get why we didn't just make these arrays in the first place. Seems like a weird design decision there and that makes me think there had to be some reason for it? Mostly I'm concerned that I'm missing context here. I've read through the original PR for this feature and I'm kind of stumped.

You noted that this is a potentially breaking change but that by deprecating them it is not breaking. Am I understanding that correctly? If so, this is the route I think is correct.

michaeldrey · 2022-08-01T20:01:29Z

I keep coming back to this PR and feeling unsure of how to respond because I don't get why we didn't just make these arrays in the first place. Seems like a weird design decision there and that makes me think there had to be some reason for it?

It seems weird to me too especially when it's a multi resource set-up, you'd want to access all the resources, not the first one in the array. There could be history in the PR from when this was merged, but i haven't dug that deep into it

breaking change

Admittedly it's been a little while since I opened this so i'll go through this again, but my original idea was going to update the properties themselves to be an array, but this broke integration tests for other services who were already escape hatching to the underlying resources via the tagetgroup.node.id property, it would force them to change to targetgroup[0].node.id.

TheRealAmazonKendra · 2022-08-02T06:48:41Z

In terms of keeping it from being a breaking change, I do like your approach. If you think it would be better to take the other approach, however, it could be done under a feature flag. I'm fine with whatever you think is the better path. Go ahead and assign this to me when it's ready for review (per your notes in the PR about needing to do some additional updates)

TheRealAmazonKendra · 2022-08-02T16:59:22Z

A big thanks to @mrgrain who did dug into this with me. It turns out that we released this before multiple target groups was supported. Given that, I like this current path and will re-review today.

TheRealAmazonKendra

Looks like there is still some logic to be updated with the new fields or I wouldn't have bothered with the neurotic suggestions to fix spacing. But, like, it's all that. This looks good so far. Gotta say, you're really killing it with all these ecs-patterns updates!

packages/@aws-cdk/aws-ecs-patterns/lib/base/application-multiple-target-groups-service-base.ts

packages/@aws-cdk/aws-ecs-patterns/lib/base/network-multiple-target-groups-service-base.ts

packages/@aws-cdk/aws-ecs-patterns/lib/ecs/application-multiple-target-groups-ecs-service.ts

packages/@aws-cdk/aws-ecs-patterns/lib/ecs/network-multiple-target-groups-ecs-service.ts

.../@aws-cdk/aws-ecs-patterns/lib/fargate/application-multiple-target-groups-fargate-service.ts

...ages/@aws-cdk/aws-ecs-patterns/lib/fargate/network-multiple-target-groups-fargate-service.ts

…le-target-groups-service-base.ts Co-authored-by: Kendra Neil <53584728+TheRealAmazonKendra@users.noreply.github.com>