fix(elasticloadbalancerV2): logicalId supports switch from addTargetGroups (under feature flag) #29513

ahammond · 2024-03-16T00:03:31Z

Issue ELBv2 logicalId inconsistency of ApplicationListenerRule logicalIds

Mitigates #29496

Reason for this change

People using ALBs who need to migrate from the addTargetGroups() convenience method to the lower level addAction() method should not be blocked due to inconsistent logicalId's. Further, the logicalIds should be consistent going forward.

Description of changes

There are two feature flags, one which sets a migration compat mode and another which fixed the behaviour to be consistent.

Description of how you validated changes

Unit testing.

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

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.

ahammond · 2024-03-25T16:17:17Z

Exemption Request: Per https://github.com/ahammond/aws-cdk/blob/62be9c7a06e3191ea8ada225eb69c626d22033e4/INTEGRATION_TESTS.md#L32 none of the conditions which require an integration test pertain.

paulhcsun

Thank you for your contribution @ahammond! The changes look good to me overall but I've just let some comments regarding theENABLE_ALBV2_EXTERNALAPPLICATIONLISTENER_ADDTARGETGROUP_CONSISTENT_LOGICALID feature flag as well as some wording changes.

We apologize for the delay in reviewing your PR. We'll make sure this gets prioritized and hopefully merged as part of the 2.136.0 release this week. We appreciate your patience.

paulhcsun · 2024-04-01T23:13:39Z

packages/aws-cdk-lib/cx-api/README.md

@@ -309,3 +309,29 @@ _cdk.json_
  }
 }
 ```
+


Can you also add the feature flag to the FEATURE_FLAGS.md file.

packages/aws-cdk-lib/cx-api/README.md

packages/aws-cdk-lib/cx-api/lib/features.ts

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

Co-authored-by: paulhcsun <47882901+paulhcsun@users.noreply.github.com>

Pull request has been modified.

Co-authored-by: paulhcsun <47882901+paulhcsun@users.noreply.github.com>

paulhcsun

Just adding a new line to keep the spacing consistent here.

packages/aws-cdk-lib/cx-api/lib/features.ts

paulhcsun

Just adding a new line to keep the spacing consistent here.

Add linebreak

paulhcsun

Nice work @ahammond, thanks again for the contribution. Looking forward to this fix being part of the next release!

paulhcsun · 2024-04-03T17:10:22Z

@Mergifyio update

mergify · 2024-04-03T17:10:46Z

update

✅ Branch has been successfully updated

aws-cdk-automation · 2024-04-03T17:38:39Z

AWS CodeBuild CI Report

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

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

mergify · 2024-04-03T17:39: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).

scanlonp · 2024-04-03T23:44:39Z

Hey @ahammond, saw that the recommended value for this feature flag is false. This makes me question if we need a feature flag or not. I see feature flags as a way to change default behavior while staying backwards compatible. If you only need this behavior some of the time, I would think setting a property is the way to go.

Is adding an optional prop to AddApplicationActionProps that controls how the suffix is generated possible?

ahammond · 2024-04-03T23:53:09Z

@scanlonp I don't have a strong feeling either way here.

scanlonp · 2024-04-04T00:08:53Z

Got it. I saw @TheRealAmazonKendra commented on your related PR. I think there has been a concerted effort to limit our feature flags, and properties looks like the best path here, so let's go with properties.

scanlonp · 2024-04-04T00:15:59Z

Going to revert this specific PR, but I will be attentive to your two PRs so that we can get them in quickly for you.

…ddTargetGroups (under feature flag)" (#29716) Reverts #29513 We will want to use a property to achieve the desired behavior instead of a feature flag since we are not changing the default behavior.

ahammond · 2024-04-13T00:20:32Z

@scanlonp to clarify, I don't care how it is solved, I care that it is solved. We are now 4 weeks since this fix was initially pushed. Regarding the props based PR I pushed on Fri Apr 5th, I addressed the comments on Monday Apr 8th in the morning. FYI, as a workaround we have bundled the entire aws-cdk-lib package into one of our internal libs and are using patch-package to apply a hack-around. This is incredibly fragile and I would like to get us back to stock ASAP.

…icationListener.addAction() (#29746) ### Issue # (if applicable) Closes #29496 ### Reason for this change See #29513 (props based solution instead of feature-flag) ### Description of changes Adds a `removeSuffix` property to addAction method to address problems due to logicalId inconsistency. ### Description of how you validated changes UTs. Per IT document, integration tests are not necessary. ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

aws-cdk-automation requested a review from a team March 16, 2024 00:03

github-actions bot added bug This issue is a bug. p2 valued-contributor [Pilot] contributed between 6-12 PRs to the CDK labels Mar 16, 2024

aws-cdk-automation previously requested changes Mar 16, 2024

View reviewed changes

ahammond force-pushed the logicalId-consistency branch from d533340 to 1bddd9f Compare March 16, 2024 00:09

ahammond changed the title ~~fix(ElasticLoadBalancerV2): consistent logicalIds (under feature flag)~~ fix(aws-elasticloadbalancerV2): consistent logicalIds (under feature flag) Mar 16, 2024

ahammond force-pushed the logicalId-consistency branch from 1bddd9f to 1387f78 Compare March 16, 2024 00:33

ahammond changed the title ~~fix(aws-elasticloadbalancerV2): consistent logicalIds (under feature flag)~~ fix(elasticloadbalancerV2): consistent logicalIds (under feature flag) Mar 16, 2024

github-actions bot mentioned this pull request Mar 18, 2024

Weekly PR metrics report #29527

Closed

aws-cdk-automation added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Mar 25, 2024

ahammond force-pushed the logicalId-consistency branch 2 times, most recently from fe2d9cc to 143a485 Compare March 26, 2024 20:17

fix(ElasticLoadBalancerV2): consistent logicalIds (under feature flag)

90c518e

ahammond force-pushed the logicalId-consistency branch from 143a485 to 90c518e Compare March 26, 2024 20:18

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

fix comment to match feature flag names

086d2f4

github-actions bot mentioned this pull request Apr 1, 2024

Monthly PR metrics report #29668

Closed

paulhcsun previously requested changes Apr 2, 2024

View reviewed changes

paulhcsun added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Apr 2, 2024

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Apr 2, 2024

Update packages/aws-cdk-lib/cx-api/README.md

4ad1095

Co-authored-by: paulhcsun <47882901+paulhcsun@users.noreply.github.com>

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

ahammond and others added 4 commits April 2, 2024 08:51

Update packages/aws-cdk-lib/cx-api/lib/features.ts

562b4a6

Co-authored-by: paulhcsun <47882901+paulhcsun@users.noreply.github.com>

Update packages/aws-cdk-lib/cx-api/lib/features.ts

06bdfe5

Co-authored-by: paulhcsun <47882901+paulhcsun@users.noreply.github.com>

Update packages/aws-cdk-lib/cx-api/lib/features.ts

c83bac6

Co-authored-by: paulhcsun <47882901+paulhcsun@users.noreply.github.com>

Update packages/aws-cdk-lib/cx-api/lib/features.ts

7a8ee82

Co-authored-by: paulhcsun <47882901+paulhcsun@users.noreply.github.com>

ahammond and others added 5 commits April 2, 2024 08:52

Update packages/aws-cdk-lib/cx-api/lib/features.ts

c6d1de3

Co-authored-by: paulhcsun <47882901+paulhcsun@users.noreply.github.com>

address comments

440cb4e

fix tests

d6417c1

Merge branch 'main' into logicalId-consistency

c8b1504

correctly revert the change for addTargetGroups

c737e4b

ahammond changed the title ~~fix(elasticloadbalancerV2): consistent logicalIds (under feature flag)~~ fix(elasticloadbalancerV2): logicalId supports switch from addTargetGroups (under feature flag) Apr 2, 2024

paulhcsun reviewed Apr 3, 2024

View reviewed changes

packages/aws-cdk-lib/cx-api/lib/features.ts Outdated Show resolved Hide resolved

paulhcsun reviewed Apr 3, 2024

View reviewed changes

Update packages/aws-cdk-lib/cx-api/lib/features.ts

b743db6

Add linebreak

paulhcsun approved these changes Apr 3, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Apr 3, 2024

Merge branch 'main' into logicalId-consistency

28fcdbe

mergify bot merged commit 8e3848c into aws:main Apr 3, 2024
9 checks passed

ahammond deleted the logicalId-consistency branch April 3, 2024 18:18

ahammond mentioned this pull request Apr 3, 2024

fix(elasticloadbalancerV2): consistent logicalId for addTargetGroups (under feature flag) #29712

Closed

1 task

scanlonp mentioned this pull request Apr 4, 2024

revert: "fix(elasticloadbalancerV2): logicalId supports switch from addTargetGroups (under feature flag)" #29716

Merged

ahammond mentioned this pull request Apr 5, 2024

feat(elasticloadbalancingv2): add removeSuffix param for ExternalApplicationListener.addAction() #29746

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(elasticloadbalancerV2): logicalId supports switch from addTargetGroups (under feature flag) #29513

fix(elasticloadbalancerV2): logicalId supports switch from addTargetGroups (under feature flag) #29513

ahammond commented Mar 16, 2024 •

edited

Loading

aws-cdk-automation left a comment

ahammond commented Mar 25, 2024 •

edited

Loading

paulhcsun left a comment

paulhcsun Apr 1, 2024

paulhcsun left a comment

paulhcsun left a comment

paulhcsun left a comment

paulhcsun commented Apr 3, 2024

mergify bot commented Apr 3, 2024

aws-cdk-automation commented Apr 3, 2024

mergify bot commented Apr 3, 2024

scanlonp commented Apr 3, 2024

ahammond commented Apr 3, 2024

scanlonp commented Apr 4, 2024

scanlonp commented Apr 4, 2024

ahammond commented Apr 13, 2024

@@ @@ -309,3 +309,29 @@ _cdk.json_ @@
                 }
               }
               ```

fix(elasticloadbalancerV2): logicalId supports switch from addTargetGroups (under feature flag) #29513

fix(elasticloadbalancerV2): logicalId supports switch from addTargetGroups (under feature flag) #29513

Conversation

ahammond commented Mar 16, 2024 • edited Loading

Issue ELBv2 logicalId inconsistency of ApplicationListenerRule logicalIds

Reason for this change

Description of changes

Description of how you validated changes

Checklist

aws-cdk-automation left a comment

Choose a reason for hiding this comment

ahammond commented Mar 25, 2024 • edited Loading

paulhcsun left a comment

Choose a reason for hiding this comment

paulhcsun Apr 1, 2024

Choose a reason for hiding this comment

paulhcsun left a comment

Choose a reason for hiding this comment

paulhcsun left a comment

Choose a reason for hiding this comment

paulhcsun left a comment

Choose a reason for hiding this comment

paulhcsun commented Apr 3, 2024

mergify bot commented Apr 3, 2024

✅ Branch has been successfully updated

aws-cdk-automation commented Apr 3, 2024

AWS CodeBuild CI Report

mergify bot commented Apr 3, 2024

scanlonp commented Apr 3, 2024

ahammond commented Apr 3, 2024

scanlonp commented Apr 4, 2024

scanlonp commented Apr 4, 2024

ahammond commented Apr 13, 2024

ahammond commented Mar 16, 2024 •

edited

Loading

ahammond commented Mar 25, 2024 •

edited

Loading