fix(route53-targets): deprecated method for dns name is used in userpool domain target (under feature flag) #31403

go-to-k · 2024-09-11T11:09:35Z

Reason for this change

The PR created a new method to get CloudFront DNS name in Cognito user pool domain. A custom resource is created in the old method, but is not in the new method.

However, the UserPoolDomainTarget in the route53-targets module continues to use the old method. So we should change to use the new method.

Description of changes

The bind method in the UserPoolDomainTarget implements IAliasRecordTarget interface, so a new method instead of the bind cannot be created.

Therefore, I take it using a feature flag.

Description of how you validated changes

Both unit and integ tests.

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

…ool domain target (under feature flag)

…rget-cognito

aws-cdk-automation · 2024-09-24T00:12:14Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

…rget-cognito

aws-cdk-automation · 2024-10-21T00:11:57Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

…rget-cognito

aws-cdk-automation · 2024-10-28T00:12:32Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

…rget-cognito

aws-cdk-automation · 2024-11-19T00:11:53Z

This PR has been in the BUILD FAILING 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.

…ithout custom resource (#31402) ### Issue # (if applicable) Closes #31342. ### Reason for this change `UserPoolDomain` creates a custom resource to get CloudFront endpoint. However, CFn exposes the attribute natively now (see the issue). No custom resource is better if it is not needed. ### Description of changes I propose a new method `cloudFrontEndpoint` without a custom resource. Three ways were originally considered. This method was chosen as it was the most reasonable of all. #### 1. Create a new method This is the method submitted in this PR. #### 2. Rewrite an existing method directly This causes destructive changes. Custom resources are not directly used in the user's application. However, this change will result in resource deletion in the user's CDK stack. This causes confusion for users and should be avoided. Also, the existing integ tests that use the method will fail because the changes are considered as destructive changes. ``` Tests: 1 failed, 1 total Failed: /aws-cdk/packages/@aws-cdk-testing/framework-integ/test/aws-cognito/test/integ.user-pool-domain-cfdist.js !!! This test contains destructive changes !!! Stack: integ-user-pool-domain-cfdist - Resource: UserPoolDomainCloudFrontDomainNameE213E594 - Impact: WILL_DESTROY Stack: integ-user-pool-domain-cfdist - Resource: UserPoolDomainCloudFrontDomainNameCustomResourcePolicy7DE54188 - Impact: WILL_DESTROY Stack: integ-user-pool-domain-cfdist - Resource: AWS679f53fac002430cb0da5b7982bd2287ServiceRoleC1EA0FF2 - Impact: WILL_DESTROY Stack: integ-user-pool-domain-cfdist - Resource: AWS679f53fac002430cb0da5b7982bd22872D164C4C - Impact: WILL_DESTROY !!! If these destructive changes are necessary, please indicate this on the PR !!! Error: Some changes were destructive! at main (/aws-cdk/packages/@aws-cdk/integ-runner/lib/cli.js:183:15) error Command failed with exit code 1. info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command. ``` #### 3. Rewrite an existing method with a feature flag An alternative to way 2, but a feature flag was avoided in this case as it leads to complexity. The [design guidelines](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md#dealing-with-breaking-behavior-changes) also recommend a new method. ### Additional information I avoided the feature flag in this PR, but there are situations where there are constructs that use an existing method, but cannot provide a new method for the constructs because it is used by a method implemented in the interface. https://github.com/go-to-k/aws-cdk/blob/fcbdc769e681f1f915cdc8cd7aa3a565d807884d/packages/aws-cdk-lib/aws-route53-targets/lib/userpool-domain.ts#L14 I will make changes to those cases using a feature flag in a separate PR. #31403 ### Description of how you validated changes Both unit and integ tests. ### 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*

…rget-cognito

codecov · 2024-11-27T02:53:44Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.67%. Comparing base (ca01a25) to head (809eabe).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #31403      +/-   ##
==========================================
+ Coverage   78.66%   78.67%   +0.01%     
==========================================
  Files         107      107              
  Lines        7237     7237              
  Branches     1329     1329              
==========================================
+ Hits         5693     5694       +1     
+ Misses       1358     1357       -1     
  Partials      186      186

Flag	Coverage Δ
suite.unit	`78.67% <ø> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
packages/aws-cdk	`78.67% <ø> (+0.01%)`	⬆️

aws-cdk-automation · 2024-12-01T00:14:31Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2024-12-02T00:12:33Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

…rget-cognito

aws-cdk-automation · 2024-12-09T00:12:19Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

…rget-cognito

nmussy · 2024-12-10T08:03:15Z

I didn't have a long enough look to give a meaningful review, but I'm not sure "new method" is descriptive enough. Not to mention that we might add "newer" method flags in the future

go-to-k · 2024-12-10T11:03:40Z

@nmussy

You're absolutely right (I hadn't really thought about it properly...). I have corrected it as such.

nmussy

LGTM. It might be better to rename the feature flag to say what it does vs what it doesn't do anymore, ie. attribute referencing vs CR lookup, but I'll let maintainers weigh in on this

nmussy · 2024-12-10T13:51:07Z

...-testing/framework-integ/test/aws-route53-targets/test/integ.userpool-domain-alias-target.ts

+new IntegTest(app, 'userpool-domain-alias-target-integ', {
+  testCases: [stack],
+});


I was going to ask to add a test to make sure the CR wasn't created, but you're already checking for it in the unit test, so it might be overkill

https://github.com/aws/aws-cdk/pull/31403/files#diff-d39a13fcbdb9a308f6017d9800b5f4244d21103d76b708c68d84226d0172faeaR78-R79

mergify · 2024-12-11T21:54:50Z

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 · 2024-12-11T22:27:08Z

AWS CodeBuild CI Report

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

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

mergify · 2024-12-11T22:27:37Z

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).

github-actions · 2024-12-11T22:28:04Z

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

fix(route53-targets): deprecated method for dns name is used in userp…

7938e8f

…ool domain target (under feature flag)

aws-cdk-automation requested a review from a team September 11, 2024 11:09

github-actions bot added p2 distinguished-contributor [Pilot] contributed 50+ PRs to the CDK labels Sep 11, 2024

go-to-k mentioned this pull request Sep 11, 2024

feat(cognito): new cloudFrontEndpoint method for user pool domain without custom resource #31402

Merged

1 task

go-to-k added 2 commits September 11, 2024 20:20

Merge branch 'main' into r53-target-cognito

0e65646

Merge branch 'main' of https://github.com/go-to-k/aws-cdk into r53-ta…

7a269d4

…rget-cognito

go-to-k added 5 commits September 24, 2024 14:10

Merge branch 'main' of https://github.com/go-to-k/aws-cdk into r53-ta…

d4c3d75

…rget-cognito

Merge branch 'main' of https://github.com/go-to-k/aws-cdk into r53-ta…

701bf9c

…rget-cognito

Merge branch 'main' of https://github.com/go-to-k/aws-cdk into r53-ta…

1b921ca

…rget-cognito

Merge branch 'main' of https://github.com/go-to-k/aws-cdk into r53-ta…

247404e

…rget-cognito

Merge branch 'main' of https://github.com/go-to-k/aws-cdk into r53-ta…

1e94917

…rget-cognito

go-to-k added 2 commits October 21, 2024 14:19

Merge branch 'main' of https://github.com/go-to-k/aws-cdk into r53-ta…

60462ab

…rget-cognito

space

34743e8

Merge branch 'main' of https://github.com/go-to-k/aws-cdk into r53-ta…

7797a55

…rget-cognito

Merge branch 'main' into r53-target-cognito

b9e22c8

Merge branch 'main' of https://github.com/go-to-k/aws-cdk into r53-ta…

ad018bc

…rget-cognito

go-to-k marked this pull request as ready for review November 27, 2024 02:41

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

Merge branch 'main' of https://github.com/go-to-k/aws-cdk into r53-ta…

4ed4cea

…rget-cognito

Merge branch 'main' of https://github.com/go-to-k/aws-cdk into r53-ta…

25cd57d

…rget-cognito

change name to userPoolDomainNameMethodWithoutCustomResource

3d846ad

nmussy approved these changes Dec 10, 2024

View reviewed changes

aws-cdk-automation added pr/needs-maintainer-review This PR needs a review from a Core Team Member and removed pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels Dec 10, 2024

GavinZZ approved these changes Dec 11, 2024

View reviewed changes

Merge branch 'main' into r53-target-cognito

809eabe

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 11, 2024

mergify bot merged commit 5e73dd0 into aws:main Dec 11, 2024
20 checks passed

github-actions bot locked as resolved and limited conversation to collaborators Dec 11, 2024

go-to-k deleted the r53-target-cognito branch December 12, 2024 11:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(route53-targets): deprecated method for dns name is used in userpool domain target (under feature flag) #31403

fix(route53-targets): deprecated method for dns name is used in userpool domain target (under feature flag) #31403

go-to-k commented Sep 11, 2024 •

edited

Loading

aws-cdk-automation commented Sep 24, 2024

aws-cdk-automation commented Oct 21, 2024

aws-cdk-automation commented Oct 28, 2024

aws-cdk-automation commented Nov 19, 2024

codecov bot commented Nov 27, 2024 •

edited

Loading

aws-cdk-automation commented Dec 1, 2024

aws-cdk-automation commented Dec 2, 2024

aws-cdk-automation commented Dec 9, 2024

nmussy commented Dec 10, 2024

go-to-k commented Dec 10, 2024

nmussy left a comment

nmussy Dec 10, 2024

mergify bot commented Dec 11, 2024

aws-cdk-automation commented Dec 11, 2024

mergify bot commented Dec 11, 2024

github-actions bot commented Dec 11, 2024

fix(route53-targets): deprecated method for dns name is used in userpool domain target (under feature flag) #31403

fix(route53-targets): deprecated method for dns name is used in userpool domain target (under feature flag) #31403

Conversation

go-to-k commented Sep 11, 2024 • edited Loading

Reason for this change

Description of changes

Description of how you validated changes

Checklist

aws-cdk-automation commented Sep 24, 2024

aws-cdk-automation commented Oct 21, 2024

aws-cdk-automation commented Oct 28, 2024

aws-cdk-automation commented Nov 19, 2024

codecov bot commented Nov 27, 2024 • edited Loading

Codecov Report

aws-cdk-automation commented Dec 1, 2024

aws-cdk-automation commented Dec 2, 2024

aws-cdk-automation commented Dec 9, 2024

nmussy commented Dec 10, 2024

go-to-k commented Dec 10, 2024

nmussy left a comment

Choose a reason for hiding this comment

nmussy Dec 10, 2024

Choose a reason for hiding this comment

mergify bot commented Dec 11, 2024

aws-cdk-automation commented Dec 11, 2024

AWS CodeBuild CI Report

mergify bot commented Dec 11, 2024

github-actions bot commented Dec 11, 2024

go-to-k commented Sep 11, 2024 •

edited

Loading

codecov bot commented Nov 27, 2024 •

edited

Loading