feat(codedeploy): supporting the creation of deployment groups with multiple load balancers #27474

nicolastremblay1 · 2023-10-10T04:29:08Z

Summary

This change adds multiple load balancer support for CodeDeploy deployment groups.

There is a currently a loadBalancer field in the ServerDeploymentGroup, adding loadBalancers might be a good field to extend the functionality to create a deployment group with multiple load balancers.

Closes #27407

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

aws-cdk-automation

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.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

lpizzinidev

The implementation looks correct, anyway, some changes are needed in my opinion.
Also:

Can you update the title to specify the module? (eg feat(codedeploy) ...) (see guidelines)
Can you update the description of the PR and remove the duplicate reference to the target issue?

Thanks 💪

packages/aws-cdk-lib/aws-codedeploy/lib/server/deployment-group.ts

packages/aws-cdk-lib/aws-codedeploy/README.md

packages/aws-cdk-lib/aws-codedeploy/test/server/deployment-group.test.ts

packages/aws-cdk-lib/aws-codedeploy/lib/server/deployment-group.ts

lpizzinidev · 2023-10-10T13:12:26Z

...s/@aws-cdk-testing/framework-integ/test/aws-codedeploy/test/server/integ.deployment-group.ts

@@ -22,10 +23,29 @@ elb.addListener({
  externalPort: 80,
 });

+const alb = new lb2.ApplicationLoadBalancer(stack, 'ALB', {


A separate integration test should be added for the new attribute.

packages/aws-cdk-lib/aws-codedeploy/lib/server/deployment-group.ts

packages/aws-cdk-lib/aws-codedeploy/README.md

…p.ts Co-authored-by: Luca Pizzini <[email protected]>

Co-authored-by: Luca Pizzini <[email protected]>

…up.test.ts Co-authored-by: Luca Pizzini <[email protected]>

lpizzinidev

Thanks!
Just some minor adjustments and a missing test case to be added.

Also, the integration test is failing because it seems that you have not executed it.
You should:

Rollback changes to the snapshots of integ.deployment-group
Execute integ.deployment-group-multiple-load-balancers locally (see guidelines)
Push the generated snapshots

lpizzinidev · 2023-10-11T06:58:44Z

packages/aws-cdk-lib/aws-codedeploy/lib/server/deployment-group.ts

@@ -168,9 +168,19 @@ export interface ServerDeploymentGroupProps {
   * or an Application Load Balancer / Network Load Balancer Target Group.
   *
   * @default - Deployment Group will not have a load balancer defined.
+   * @deprecated - Deprecated in favor of loadBalancers.


Suggested change

* @deprecated - Deprecated in favor of loadBalancers.

* @deprecated - Use `loadBalancers` instead.

More concise

lpizzinidev · 2023-10-11T07:02:34Z

packages/aws-cdk-lib/aws-codedeploy/lib/server/deployment-group.ts

+    this.loadBalancers = props.loadBalancers || (props.loadBalancer ? [props.loadBalancer]: undefined);
+
+    if (this.loadBalancers && this.loadBalancers.length === 0) {
+      throw new Error('Cannot set empty loadBalancers array');


Suggested change

throw new Error('Cannot set empty loadBalancers array');

throw new Error('loadBalancers must be a non-empty array');

Formatting. Also, a unit test needs to be added to verify that the error is thrown if loadBalancers: [].

lpizzinidev · 2023-10-11T07:05:07Z

packages/aws-cdk-lib/aws-codedeploy/lib/server/deployment-group.ts

-          ],
-        };
-    }
+    return loadBalancers.reduce((accumulator : {


Suggested change

return loadBalancers.reduce((accumulator : {

return loadBalancers?.reduce((accumulator : {

After if statement removal.

lpizzinidev · 2023-10-11T07:06:07Z

packages/aws-cdk-lib/aws-codedeploy/lib/server/deployment-group.ts

+
+    if (!loadBalancers) {
      return undefined;
    }



Suggested change

if (!loadBalancers) {

return undefined;

}

Unnecessary.

lpizzinidev · 2023-10-11T07:10:24Z

packages/aws-cdk-lib/aws-codedeploy/test/server/deployment-group.test.ts

@@ -212,6 +213,70 @@ describe('CodeDeploy Server Deployment Group', () => {
    });
  });

+  test('can be created with multiple ALB Target Groups as the load balancers', () => {


Suggested change

test('can be created with multiple ALB Target Groups as the load balancers', () => {

test('can be created with multiple Target Groups as the load balancers', () => {

You can specify also CLBs and NLBs.

lpizzinidev · 2023-10-11T07:21:59Z

packages/aws-cdk-lib/aws-codedeploy/README.md

@@ -150,6 +150,29 @@ const deploymentGroup = new codedeploy.ServerDeploymentGroup(this, 'DeploymentGr
 });
 ```

+To provide multiple Elastic Load Balancers as target groups use the `loadBalancers` parameter:


Suggested change

To provide multiple Elastic Load Balancers as target groups use the `loadBalancers` parameter:

The `loadBalancer` property has been deprecated. To provide multiple Elastic Load Balancers

as target groups use the `loadBalancers` parameter:

Note about deprecation.

lpizzinidev

The build is still failing but I note now that you opened the PR from your main branch.
Merge requests opened from the main branch cannot be merged.
You should close this PR and open a new one from a separate branch.
Thanks 💪

lpizzinidev · 2023-10-19T08:02:49Z

packages/aws-cdk-lib/aws-codedeploy/test/server/deployment-group.test.ts

+  test.each<{ clbCount: number, albCount: number, nlbCount: number, TargetGroupInfoList: string[], ElbInfoList: string[] }>([
+    { clbCount: 0, albCount: 0, nlbCount: 1, TargetGroupInfoList: ['NLBListenerNLB0FleetGroup657E42E4'], ElbInfoList: [] },
+    { clbCount: 0, albCount: 0, nlbCount: 2, TargetGroupInfoList: ['NLBListenerNLB0FleetGroup657E42E4', 'NLBListenerNLB1FleetGroup5298209F'], ElbInfoList: [] },
+    { clbCount: 0, albCount: 1, nlbCount: 0, TargetGroupInfoList: ['ALBListenerALB0FleetGroupF4CBAA91'], ElbInfoList: [] },
+    { clbCount: 0, albCount: 2, nlbCount: 0, TargetGroupInfoList: ['ALBListenerALB0FleetGroupF4CBAA91', 'ALBListenerALB1FleetGroupD8E824C3'], ElbInfoList: [] },
+    { clbCount: 0, albCount: 1, nlbCount: 1, TargetGroupInfoList: ['ALBListenerALB0FleetGroupF4CBAA91', 'NLBListenerNLB0FleetGroup657E42E4'], ElbInfoList: [] },
+    { clbCount: 0, albCount: 2, nlbCount: 2, TargetGroupInfoList: ['ALBListenerALB0FleetGroupF4CBAA91', 'ALBListenerALB1FleetGroupD8E824C3', 'NLBListenerNLB0FleetGroup657E42E4', 'NLBListenerNLB1FleetGroup5298209F'], ElbInfoList: [] },
+    { clbCount: 1, albCount: 0, nlbCount: 0, TargetGroupInfoList: [], ElbInfoList: ['CLB0677386E0'] },
+    { clbCount: 2, albCount: 0, nlbCount: 0, TargetGroupInfoList: [], ElbInfoList: ['CLB0677386E0', 'CLB18DCC2BD6'] },
+    { clbCount: 1, albCount: 0, nlbCount: 1, TargetGroupInfoList: ['NLBListenerNLB0FleetGroup657E42E4'], ElbInfoList: ['CLB0677386E0'] },
+    { clbCount: 2, albCount: 0, nlbCount: 2, TargetGroupInfoList: ['NLBListenerNLB0FleetGroup657E42E4', 'NLBListenerNLB1FleetGroup5298209F'], ElbInfoList: ['CLB0677386E0', 'CLB18DCC2BD6'] },
+    { clbCount: 1, albCount: 1, nlbCount: 0, TargetGroupInfoList: ['ALBListenerALB0FleetGroupF4CBAA91'], ElbInfoList: ['CLB0677386E0'] },
+    { clbCount: 2, albCount: 2, nlbCount: 0, TargetGroupInfoList: ['ALBListenerALB0FleetGroupF4CBAA91', 'ALBListenerALB1FleetGroupD8E824C3'], ElbInfoList: ['CLB0677386E0', 'CLB18DCC2BD6'] },
+    { clbCount: 1, albCount: 1, nlbCount: 1, TargetGroupInfoList: ['ALBListenerALB0FleetGroupF4CBAA91', 'NLBListenerNLB0FleetGroup657E42E4'], ElbInfoList: ['CLB0677386E0'] },
+    { clbCount: 2, albCount: 2, nlbCount: 2, TargetGroupInfoList: ['ALBListenerALB0FleetGroupF4CBAA91', 'ALBListenerALB1FleetGroupD8E824C3', 'NLBListenerNLB0FleetGroup657E42E4', 'NLBListenerNLB1FleetGroup5298209F'], ElbInfoList: ['CLB0677386E0', 'CLB18DCC2BD6'] },
+  ])('Should build ServerDeploymentGroup with $clbCount CLBs, $albCount ALBs and $nlbCount NLBs expecting TargetGroupInfoList $TargetGroupInfoList and ElbInfoList $ElbInfoList', ({ clbCount, albCount, nlbCount, TargetGroupInfoList, ElbInfoList }) => {


The test added below already gives test coverage for the new feature.
What's the benefit of checking for all potential combinations of ELBs?

Just wanted to make sure as many test cases were covered as possible in case I might of missed a corner case.

nicolastremblay1 · 2023-10-19T10:43:21Z

Moving the PR here

aws-cdk-automation · 2023-10-19T10:59:14Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: e3c8797
Result: SUCCEEDED
Build Logs (available for 30 days)

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

nicolastremblay1 added 3 commits October 9, 2023 20:37

Add multiple loadBalancer support to CodeDeploy deployment group

ed0b958

Add integration test to loadBalancers field

7449373

Add integration test to loadBalancers field

db6f726

github-actions bot added beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 labels Oct 10, 2023

aws-cdk-automation requested a review from a team October 10, 2023 04:29

aws-cdk-automation previously requested changes Oct 10, 2023

View reviewed changes

nicolastremblay1 changed the title ~~Add multiple load balancers to CodeDeploy deployment group~~ feat: add multiple load balancers to CodeDeploy deployment group Oct 10, 2023

Add integration test to loadBalancers field

c192ff9

Add integration test to loadBalancers field

259d5b4

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Oct 10, 2023

lpizzinidev suggested changes Oct 10, 2023

View reviewed changes

nicolastremblay1 and others added 2 commits October 10, 2023 09:31

Update packages/aws-cdk-lib/aws-codedeploy/lib/server/deployment-grou…

674a77a

…p.ts Co-authored-by: Luca Pizzini <[email protected]>

Update packages/aws-cdk-lib/aws-codedeploy/README.md

51459ab

Co-authored-by: Luca Pizzini <[email protected]>

nicolastremblay1 changed the title ~~feat: add multiple load balancers to CodeDeploy deployment group~~ feat(codedeploy): supporting the creation of deployment groups with multiple load balancers Oct 10, 2023

nicolastremblay1 and others added 6 commits October 10, 2023 09:35

Update packages/aws-cdk-lib/aws-codedeploy/README.md

134def5

Co-authored-by: Luca Pizzini <[email protected]>

Update packages/aws-cdk-lib/aws-codedeploy/README.md

e77761d

Co-authored-by: Luca Pizzini <[email protected]>

Update packages/aws-cdk-lib/aws-codedeploy/test/server/deployment-gro…

0abdb1f

…up.test.ts Co-authored-by: Luca Pizzini <[email protected]>

Update packages/aws-cdk-lib/aws-codedeploy/test/server/deployment-gro…

1f9397a

…up.test.ts Co-authored-by: Luca Pizzini <[email protected]>

Removing unused integ value in integ.deployment-group test

f687583

Try to build new tests without snapshots generated locally

ac2f908

lpizzinidev suggested changes Oct 11, 2023

View reviewed changes

nicolastremblay1 and others added 4 commits October 18, 2023 07:01

Merge branch 'main' into main

e7ac004

Add integ deployment-group-multiple-load-balancer snapshot back

beb6758

test(lbs): Adding tests to test out muliple lb combinations

283e663

fix(readme): try to remove duplicate elbv2 import and fix invalid type

acd8af1

lpizzinidev suggested changes Oct 19, 2023

View reviewed changes

Merge branch 'aws:main' into main

e3c8797

mergify bot mentioned this pull request Oct 19, 2023

feat(codedeploy): support multiple load balancers for deployment groups #27607

Merged

nicolastremblay1 closed this Oct 19, 2023

github-actions bot mentioned this pull request Nov 1, 2023

Monthly PR metrics report - October 2023 #27786

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(codedeploy): supporting the creation of deployment groups with multiple load balancers #27474

feat(codedeploy): supporting the creation of deployment groups with multiple load balancers #27474

nicolastremblay1 commented Oct 10, 2023 •

edited

Loading

aws-cdk-automation left a comment

lpizzinidev left a comment •

edited

Loading

lpizzinidev Oct 10, 2023

lpizzinidev left a comment •

edited

Loading

lpizzinidev Oct 11, 2023

lpizzinidev Oct 11, 2023

lpizzinidev Oct 11, 2023

lpizzinidev Oct 11, 2023

lpizzinidev Oct 11, 2023

lpizzinidev Oct 11, 2023

lpizzinidev left a comment

lpizzinidev Oct 19, 2023

nicolastremblay1 Oct 19, 2023

nicolastremblay1 commented Oct 19, 2023

aws-cdk-automation commented Oct 19, 2023

	* @deprecated - Deprecated in favor of loadBalancers.
	* @deprecated - Use `loadBalancers` instead.

	throw new Error('Cannot set empty loadBalancers array');
	throw new Error('loadBalancers must be a non-empty array');

	return loadBalancers.reduce((accumulator : {
	return loadBalancers?.reduce((accumulator : {

	test('can be created with multiple ALB Target Groups as the load balancers', () => {
	test('can be created with multiple Target Groups as the load balancers', () => {

	To provide multiple Elastic Load Balancers as target groups use the `loadBalancers` parameter:
	The `loadBalancer` property has been deprecated. To provide multiple Elastic Load Balancers
	as target groups use the `loadBalancers` parameter:

feat(codedeploy): supporting the creation of deployment groups with multiple load balancers #27474

feat(codedeploy): supporting the creation of deployment groups with multiple load balancers #27474

Conversation

nicolastremblay1 commented Oct 10, 2023 • edited Loading

Summary

aws-cdk-automation left a comment

Choose a reason for hiding this comment

lpizzinidev left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpizzinidev left a comment • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpizzinidev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolastremblay1 commented Oct 19, 2023

aws-cdk-automation commented Oct 19, 2023

AWS CodeBuild CI Report

nicolastremblay1 commented Oct 10, 2023 •

edited

Loading

lpizzinidev left a comment •

edited

Loading

lpizzinidev left a comment •

edited

Loading