fix(autoscaling): Allow adding AutoScalingGroup to multiple target groups #23044

azatoth · 2022-11-22T23:42:17Z

While this doesn't solve the underlying problem, which will require an API breakage in scaleOnRequestCount() (see
#5667 (comment)).

As scaleOnRequestCount() seems to be the only part that is affected by this, we'll only issue a validation error if we have multiple targetGroupArns and scaleOnRequestCount() has been called.

closes #5667

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-11-22T23:42:22Z

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.

azatoth · 2022-11-29T14:36:17Z

The pull request linter fails with the following errors:
❌ Fixes must contain a change to an integration test file and the resulting snapshot.
PRs must pass status checks before we can provide a meaningful review.

I don't think an update to integration is needed as we are not changing anything in what CFN is created.

aws-cdk-automation · 2022-12-14T00:10:10Z

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.

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

comcalvi

This looks really good! Could we add a unit test for the positive case too?

comcalvi · 2022-12-15T21:49:50Z

packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts

+  private validateTargetGroup(): string[] {
+    const errors = new Array<string>();
+    if (this.hasSetScaleOnRequest && this.targetGroupArns.length > 1) {
+      errors.push('Cannon use multiple target groups if `setScaleOnRequest()` is being used.');


setScaleOnRequest() should be scaleOnRequestCount(), no? scaleOnRequestCount() enables hasSetScaleOnRequest(), I don't see a setScaleOnRequest() method in the auto-scaling-group.ts

Oops, the "set" part in the variable meant to reflect that the method had been called; Seems I transposed that to method name when writing the error message. Changed in 1928074

packages/@aws-cdk/aws-autoscaling/test/auto-scaling-group.test.ts

Pull request has been modified.

azatoth · 2022-12-17T10:26:59Z

This looks really good! Could we add a unit test for the positive case too?

The test for the positive case was kinda in the test already; In any case, I did split the test into two separate tests in db1f0de to be more explicit.

…oups While this doesn't solve the underlying problem, which will require an API breakage in `scaleOnRequestCount()` (see aws#5667 (comment)). As `scaleOnRequestCount()` seems to be the only part that is affected by this, we'll only issue a validation error if we have multiple `targetGroupArns` and `scaleOnRequestCount()` has been called. closes aws#5667

Co-authored-by: Calvin Combs <66279577+comcalvi@users.noreply.github.com>

The naming and information didn't fully match eachother.

this to clearly indicate that one is positive and the other is negative.

mergify · 2022-12-20T23:19:40Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2022-12-21T00:35:40Z

AWS CodeBuild CI Report

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

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

mergify · 2022-12-21T00:36:00Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

…oups (aws#23044) While this doesn't solve the underlying problem, which will require an API breakage in `scaleOnRequestCount()` (see aws#5667 (comment)). As `scaleOnRequestCount()` seems to be the only part that is affected by this, we'll only issue a validation error if we have multiple `targetGroupArns` and `scaleOnRequestCount()` has been called. closes aws#5667 ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] 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*

github-actions bot added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1 labels Nov 22, 2022

aws-cdk-automation requested a review from a team November 22, 2022 23:42

github-actions bot added the beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK label Nov 22, 2022

aws-cdk-automation previously requested changes Nov 22, 2022

View reviewed changes

azatoth force-pushed the allow-autoscalinggroup-2nd-target-group branch from f4d532a to 89f051a Compare November 29, 2022 08:29

azatoth force-pushed the allow-autoscalinggroup-2nd-target-group branch from 89f051a to 7d16f05 Compare December 6, 2022 08:46

azatoth force-pushed the allow-autoscalinggroup-2nd-target-group branch from 7d16f05 to 1c22d00 Compare December 14, 2022 21:39

azatoth force-pushed the allow-autoscalinggroup-2nd-target-group branch from 1c22d00 to 5cb114e Compare December 15, 2022 08:49

comcalvi previously requested changes Dec 15, 2022

View reviewed changes

comcalvi self-assigned this Dec 15, 2022

azatoth requested a review from comcalvi December 17, 2022 09:30

azatoth force-pushed the allow-autoscalinggroup-2nd-target-group branch 2 times, most recently from 1928074 to 1ae1d8c Compare December 17, 2022 09:57

azatoth and others added 4 commits December 19, 2022 11:26

Apply suggestions from code review

bca2c12

Co-authored-by: Calvin Combs <66279577+comcalvi@users.noreply.github.com>

Properly name variable for checking if scale on request has been called

6fb845d

The naming and information didn't fully match eachother.

Split test for multiple target groups into two separate tests

9d96ab7

this to clearly indicate that one is positive and the other is negative.

azatoth force-pushed the allow-autoscalinggroup-2nd-target-group branch from db1f0de to 9d96ab7 Compare December 19, 2022 10:26

comcalvi approved these changes Dec 20, 2022

View reviewed changes

Merge branch 'main' into allow-autoscalinggroup-2nd-target-group

4834b6d

mergify bot merged commit 07acd18 into aws:main Dec 21, 2022

azatoth deleted the allow-autoscalinggroup-2nd-target-group branch December 21, 2022 00:54

azatoth restored the allow-autoscalinggroup-2nd-target-group branch December 21, 2022 00:54

azatoth deleted the allow-autoscalinggroup-2nd-target-group branch December 21, 2022 00:57

azatoth restored the allow-autoscalinggroup-2nd-target-group branch December 21, 2022 01:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(autoscaling): Allow adding AutoScalingGroup to multiple target groups #23044

fix(autoscaling): Allow adding AutoScalingGroup to multiple target groups #23044

azatoth commented Nov 22, 2022

gitpod-io bot commented Nov 22, 2022

aws-cdk-automation left a comment

azatoth commented Nov 29, 2022

aws-cdk-automation commented Dec 14, 2022

comcalvi left a comment

comcalvi Dec 15, 2022

azatoth Dec 17, 2022 •

edited

azatoth commented Dec 17, 2022

mergify bot commented Dec 20, 2022

aws-cdk-automation commented Dec 21, 2022

mergify bot commented Dec 21, 2022

fix(autoscaling): Allow adding AutoScalingGroup to multiple target groups #23044

fix(autoscaling): Allow adding AutoScalingGroup to multiple target groups #23044

Conversation

azatoth commented Nov 22, 2022

All Submissions:

Adding new Unconventional Dependencies:

New Features

gitpod-io bot commented Nov 22, 2022

aws-cdk-automation left a comment

Choose a reason for hiding this comment

azatoth commented Nov 29, 2022

aws-cdk-automation commented Dec 14, 2022

comcalvi left a comment

Choose a reason for hiding this comment

comcalvi Dec 15, 2022

Choose a reason for hiding this comment

azatoth Dec 17, 2022 • edited

Choose a reason for hiding this comment

azatoth commented Dec 17, 2022

mergify bot commented Dec 20, 2022

aws-cdk-automation commented Dec 21, 2022

AWS CodeBuild CI Report

mergify bot commented Dec 21, 2022

azatoth Dec 17, 2022 •

edited