feat(ecs): add a new API for registering ECS targets #4212

iamhopaul123 · 2019-09-23T22:53:34Z

Add a new API for registering ECS targets, which is also the second part for #3922.

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

mergify · 2019-09-23T22:54:10Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-09-23T22:54:11Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

aws-cdk-automation · 2019-09-23T23:35:17Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

piradeepk · 2019-09-24T13:51:18Z

packages/@aws-cdk/aws-ecs/lib/base/base-service.ts

+          protocol: containerTarget.protocol
+        };
+        for (const targetGroupProp of containerTarget.targetGroups) {
+          if (targetGroupProp.listener instanceof elbv2.ApplicationListener) {


Just for re-usability can we move out the logic for registering based on ApplicationListener and NetworkLister to independent methods, and refer to those methods here?

Pull request has been modified.

aws-cdk-automation · 2019-09-24T18:32:37Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2019-09-24T23:50:44Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

rix0rrr · 2019-09-25T13:43:30Z

packages/@aws-cdk/aws-ecs/lib/base/base-service.ts

+  /**
+   * Container targets of the container.
+   */
+  readonly containerTargets: ContainerTargetProps[]


Not sure I like this name. It feels like introducing a new concept to users that I wonder whether it's worthwhile.

Also: ultimately, we just need a flat list of (container, port, targetGroupName, listener), so why introduce all this hierarchy here? Feels like a flat list of tuples would give a much more convenient API.

Yeah I agree, since for each service for most of cases there will only be one or two targets to be registered. Using a flat list should be a better option.

rix0rrr · 2019-09-25T13:44:21Z

packages/@aws-cdk/aws-ecs/lib/base/base-service.ts

+  /**
+   * Setting to create and attach target groups.
+   */
+  readonly targetGroups: TargetGroupProps [],


I would expect a property named targetGroups to take ITargetGroup objects, not properties to create new target groups.

rix0rrr · 2019-09-25T13:44:48Z

packages/@aws-cdk/aws-ecs/lib/base/base-service.ts

+  /**
+   * Properties for adding new targets to a listener.
+   */
+  readonly addTargetGroupProps?: elbv2.AddApplicationTargetsProps | elbv2.AddNetworkTargetsProps,


No type unions please. If you want unions, create union-like classes instead.

Cannot create an interface that extends these and also listeners below because they both have the properties with the same name but different types. Also, can you please explain the reason why type union is not recommended?

Type unions do not translate well across jsii (to all languages that we target).

Union-like classes are abstract base classes that get constructed through some factory function:

abstract class MyGroupUnion { public static variantOne(): MyGroupUnion { // Return some subclass of MyGroupUnion } public static variantTwo(props: SomeProps) { // Return some subclass of MyGroupUnion } public abstract yieldSomeInterestingProperties(): SomeInterestingProperties; }

rix0rrr · 2019-09-25T13:45:43Z

packages/@aws-cdk/aws-ecs/lib/base/base-service.ts

+  /**
+   * Listener for the target group to attach to [disable-awslint:ref-via-interface].
+   */
+  readonly listener: elbv2.ApplicationListener | elbv2.NetworkListener


Why are the interfaces not good enough?

Can that be fixed?

So it's because INetworkListener does not have the method addTargets, and arn is the only property the interface has.

Sure. Would it be possible to effectively implement addTargets() in the interfaces?

I guess not, huh?

It is still possible to implement addTargets() but it will be a little bit clumsy and reinvent the wheels. Yeah I think you are right.

aws-cdk-automation · 2019-09-25T17:28:13Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2019-09-25T17:52:33Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2019-09-25T19:15:11Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2019-09-26T21:11:33Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

rix0rrr · 2019-09-30T08:11:45Z

packages/@aws-cdk/aws-ecs/lib/base/base-service.ts

+  /**
+   * Create a config for adding target group to ALB listener.
+   */
+  public static applicationListenerConfig(listener: elbv2.ApplicationListener, props?: elbv2.AddApplicationTargetsProps): ListenerConfig {


I would remove the Config at the end of the method name here. Shorter is better, and the "config" part is already implied by the class name.

rix0rrr · 2019-09-30T08:12:08Z

packages/@aws-cdk/aws-ecs/lib/base/base-service.ts

+  /**
+   * Properties for defining an ECS target.
+   */
+  readonly containerTarget: LoadBalancerTargetOptions,


All other interfaces are declared using a semicolon (;) at the end of fields, so please match the existing style.

Also, I think I would be in favor of inlining the LoadBalancerTargetOptions property into the parent struct here. The usability will be a lot better in non-JS languages.

rix0rrr · 2019-09-30T08:12:50Z

packages/@aws-cdk/aws-ecs/lib/base/base-service.ts

+  /**
+   * ID for a target group to be created.
+   */
+  readonly targetGroupId: string,


I would prefer you called this newTargetGroupId to make it abundantly clear you will be creating a new TargetGroup for users, rather than picking an existing Target Group that they've already created.

rix0rrr · 2019-09-30T08:16:51Z

packages/@aws-cdk/aws-ecs/lib/base/base-service.ts

+   *   },
+   * )
+   */
+  public registerContainerTargets(...targets: EcsTarget[]) {


Are you sure that, if you're coming at this cold (and don't have all the state that you have in your head about this change), that the name service.registerContainerTargets is clear enough that it pertains to load balancing? (And not, for example, service discovery?)

For myself, I would be wondering what I'm registering into?

It is hard to find a name that can be really clear enough to figure out what exactly this method does by seeing the name alone. However, I agree the naming can be improved to make it more explicit.

aws-cdk-automation · 2019-09-30T19:17:09Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

Pull request has been modified.

aws-cdk-automation · 2019-10-01T16:58:14Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

piradeepk · 2019-10-02T02:01:37Z

packages/@aws-cdk/aws-ecs/lib/base/base-service.ts

+   * @example
+   *
+   * service.registerLoadBalancerTargets(
+   *   {
+   *     containerTarget: {
+   *       containerName: 'web',
+   *       containerPort: 80,
+   *     },
+   *     targetGroupId: 'ECS',
+   *     listener: ecs.ListenerConfig.applicationListener(listener, {
+   *       protocol: elbv2.ApplicationProtocol.HTTPS
+   *     }),
+   *   },
+   * )


You should add this to the README, not sure if it makes sense to add it here.

Do we have a precedent for having examples in jsdoc strings versus in README?

packages/@aws-cdk/aws-ecs/lib/base/base-service.ts

piradeepk · 2019-10-02T02:04:32Z

packages/@aws-cdk/aws-ecs/lib/base/base-service.ts

+  /**
+   * ID for a target group to be created.
+   */
+  readonly newTargetGroupId: string;


Can we rename this to targetGroupId (new is relative)

Ha, I just made him change it from targetGroupId -> newTargetGroupId.

The thing is, I'm not sure that targetGroupId conveys accurately enough that it will be created, rather than one that already exists that happens to have that ID.

Fair enough, just find newTargetGroup to be ambiguous.

aws-cdk-automation · 2019-10-02T17:04:53Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

SoManyHs

Very small nits, otherwise LGTM!

SoManyHs · 2019-10-03T23:43:27Z

packages/@aws-cdk/aws-ecs/lib/base/base-service.ts

+   * @example
+   *
+   * service.registerLoadBalancerTargets(
+   *   {
+   *     containerTarget: {
+   *       containerName: 'web',
+   *       containerPort: 80,
+   *     },
+   *     targetGroupId: 'ECS',
+   *     listener: ecs.ListenerConfig.applicationListener(listener, {
+   *       protocol: elbv2.ApplicationProtocol.HTTPS
+   *     }),
+   *   },
+   * )


Do we have a precedent for having examples in jsdoc strings versus in README?

SoManyHs · 2019-10-03T23:47:49Z

packages/@aws-cdk/aws-ecs/test/fargate/test.fargate-service.ts

@@ -664,6 +664,321 @@ export = {

        test.done();
      }
+    },
+
+    'allows add load balancers to a service': {


Maybe rephrase to "allows registering multiple target groups"

SoManyHs · 2019-10-03T23:49:45Z

packages/@aws-cdk/aws-ecs/test/fargate/test.fargate-service.ts

+    },
+
+    'allows add load balancers to a service': {
+      'when application load balancers': {


Nit: "with" application load balancers

SoManyHs · 2019-10-03T23:54:34Z

packages/@aws-cdk/aws-ecs/test/fargate/test.fargate-service.ts

+        },
+      },
+
+      'when network load balancers': {


iamhopaul123 · 2019-10-04T00:06:20Z

@SoManyHs

Do we have a precedent for having examples in jsdoc strings versus in README?

I included the example in README as well. And for jsdoc strings, I referred to this. I think it can help when users first use this method and when they hover to the method they can see the example to know how to use the method, which makes it more convenient.

Pull request has been modified.

aws-cdk-automation · 2019-10-04T00:43:50Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

rix0rrr · 2019-10-04T08:08:53Z

Examples could/should be in both.

rix0rrr · 2019-10-04T08:10:29Z

Remove the label when you are ready to merge.

Pull request has been modified.

aws-cdk-automation · 2019-10-04T16:59:58Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

piradeepk

Ship it!

mergify · 2019-10-04T17:26:23Z

Thank you for contributing! Your pull request is now being automatically merged.

iamhopaul123 added the pr/do-not-merge This PR should not be merged at this time. label Sep 23, 2019

iamhopaul123 requested review from SoManyHs and piradeepk September 23, 2019 22:53

piradeepk previously requested changes Sep 24, 2019

View reviewed changes

SomayaB assigned rix0rrr Sep 24, 2019

rix0rrr reviewed Sep 25, 2019

View reviewed changes

iamhopaul123 force-pushed the ecs-load-balaner-new-api branch from 649ed01 to 4d5bb12 Compare September 25, 2019 17:16

iamhopaul123 force-pushed the ecs-load-balaner-new-api branch from 4d5bb12 to 9807911 Compare September 25, 2019 17:41

iamhopaul123 force-pushed the ecs-load-balaner-new-api branch from 9807911 to 0dc3b6b Compare September 25, 2019 18:35

iamhopaul123 force-pushed the ecs-load-balaner-new-api branch from 0dc3b6b to fc28e33 Compare September 26, 2019 20:29

rix0rrr reviewed Sep 30, 2019

View reviewed changes

iamhopaul123 force-pushed the ecs-load-balaner-new-api branch from fc28e33 to e9aa888 Compare September 30, 2019 18:44

rix0rrr previously approved these changes Oct 1, 2019

View reviewed changes

piradeepk reviewed Oct 2, 2019

View reviewed changes

packages/@aws-cdk/aws-ecs/lib/base/base-service.ts Show resolved Hide resolved

piradeepk reviewed Oct 2, 2019

View reviewed changes

packages/@aws-cdk/aws-ecs/lib/base/base-service.ts Show resolved Hide resolved

piradeepk reviewed Oct 2, 2019

View reviewed changes

piradeepk added the @aws-cdk/aws-ecs Related to Amazon Elastic Container label Oct 2, 2019

piradeepk assigned SoManyHs Oct 2, 2019

SoManyHs previously approved these changes Oct 3, 2019

View reviewed changes

iamhopaul123 added 8 commits October 3, 2019 17:07

Add new API to register ECS targets

f569711

Refactor registerContainerTargets

10884f2

Add more unit tests and integ tests

b77386b

Rebase and refactor to flat list for new API input

e62f9dc

Refactor to eliminate type union

e384bf6

Minor refactoring and name change

55b665a

Add more comments and README

f3ff670

Minor change to test name

b0ea719

iamhopaul123 force-pushed the ecs-load-balaner-new-api branch from c514e40 to b0ea719 Compare October 4, 2019 00:11

SoManyHs previously approved these changes Oct 4, 2019

View reviewed changes

rix0rrr previously approved these changes Oct 4, 2019

View reviewed changes

Merge branch 'master' into ecs-load-balaner-new-api

1192730

iamhopaul123 removed the pr/do-not-merge This PR should not be merged at this time. label Oct 4, 2019

piradeepk approved these changes Oct 4, 2019

View reviewed changes

mergify bot merged commit de84c4a into aws:master Oct 4, 2019

sumodgeorge mentioned this pull request Dec 9, 2023

[Snyk] Security upgrade verdaccio from 3.8.1 to 5.29.0 sumodgeorge/aws-cdk#22

Open

sumodgeorge mentioned this pull request Dec 20, 2023

[Snyk] Security upgrade verdaccio from 3.8.1 to 5.29.0 sumodgeorge/aws-cdk#24

Open

feat(ecs): add a new API for registering ECS targets #4212

feat(ecs): add a new API for registering ECS targets #4212

Conversation

iamhopaul123 commented Sep 23, 2019

mergify bot commented Sep 23, 2019

mergify bot commented Sep 23, 2019

aws-cdk-automation commented Sep 23, 2019

AWS CodeBuild CI Report

Choose a reason for hiding this comment

aws-cdk-automation commented Sep 24, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Sep 24, 2019

AWS CodeBuild CI Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamhopaul123 Sep 25, 2019 • edited Loading

Choose a reason for hiding this comment

rix0rrr Sep 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Sep 25, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Sep 25, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Sep 25, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Sep 26, 2019

AWS CodeBuild CI Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Sep 30, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Oct 1, 2019

AWS CodeBuild CI Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Oct 2, 2019

AWS CodeBuild CI Report

SoManyHs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamhopaul123 commented Oct 4, 2019

aws-cdk-automation commented Oct 4, 2019

AWS CodeBuild CI Report

rix0rrr commented Oct 4, 2019

rix0rrr commented Oct 4, 2019

aws-cdk-automation commented Oct 4, 2019

AWS CodeBuild CI Report

piradeepk left a comment

Choose a reason for hiding this comment

mergify bot commented Oct 4, 2019

iamhopaul123 Sep 25, 2019 •

edited

Loading

rix0rrr Sep 26, 2019 •

edited

Loading