feat(route53): latency based routing #28723

badmintoncryer · 2024-01-16T13:36:52Z

In this PR, I have made the RecordSet compatible with latency-based routing.
You can set it by providing a Region name to the region argument.

new route53.ARecord(this, 'ARecord', {
  zone: myZone,
  target: route53.RecordTarget.fromIpAddresses('1.2.3.4'),
  region: 'ap-northeast-1', // added
})

Closes #28722.

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

Looks good overall 👍
I left some notes for minor adjustments.

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

packages/aws-cdk-lib/aws-route53/lib/record-set.ts

badmintoncryer · 2024-01-21T12:43:42Z

@lpizzinidev Thank you for your review! I have addressed your comments.

lpizzinidev

Nice 👍

aaythapa

Thank you for your contribution! Looks great overall, only concern is should the region names be enum values? The docs clearly define which regions are allowed in this property so maybe we should do the same

packages/aws-cdk-lib/aws-route53/lib/record-set.ts

badmintoncryer · 2024-01-23T09:27:37Z

@aaythapa Thank you for your review. I'll add enum!

Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>

Co-authored-by: Aayush thapa <84202325+aaythapa@users.noreply.github.com>

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.

badmintoncryer · 2024-01-23T20:12:07Z

@aaythapa I've added LatencyBasedRoutingRegion. Please comfirm it.

aaythapa · 2024-01-23T23:47:08Z

@aaythapa I've added LatencyBasedRoutingRegion. Please confirm it.

Hi @badmintoncryer thank you for making the changes. I talked it over with the team and we decided we don't want to add enums for regions. Our reasoning is users already use strings when defining region in CDK, for example here and with references like stack.env.region. On top of that, new regions are constantly being added so we need to manually maintain the list of enums which add additional overhead.

Sorry about the extra work, I should've discussed this with the team before suggesting you add enums. Hopefully it isn't too much work to go back.

This reverts commit 22266ce.

This reverts commit f7e858e.

badmintoncryer · 2024-01-24T06:49:54Z

@aaythapa I've reverted modification! Please confirm it again.

aws-cdk-automation · 2024-01-24T20:01:44Z

AWS CodeBuild CI Report

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

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

mergify · 2024-01-24T20:02:02Z

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

In this PR, I have made the RecordSet compatible with latency-based routing. You can set it by providing a Region name to the region argument. ```ts new route53.ARecord(this, 'ARecord', { zone: myZone, target: route53.RecordTarget.fromIpAddresses('1.2.3.4'), region: 'ap-northeast-1', // added }) ``` Closes aws#28722. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

In this PR, I have made the RecordSet compatible with latency-based routing. You can set it by providing a Region name to the region argument. ```ts new route53.ARecord(this, 'ARecord', { zone: myZone, target: route53.RecordTarget.fromIpAddresses('1.2.3.4'), region: 'ap-northeast-1', // added }) ``` Closes #28722. ---- *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 January 16, 2024 13:37

github-actions bot added feature-request A feature should be added or improved. p2 admired-contributor [Pilot] contributed between 13-24 PRs to the CDK labels Jan 16, 2024

badmintoncryer mentioned this pull request Jan 16, 2024

support routing policy setting for ARecord class #4391

Open

badmintoncryer force-pushed the 28722-latencyBasedRouting branch 4 times, most recently from fad8429 to 9f583e6 Compare January 18, 2024 06:34

aws-cdk-automation previously requested changes Jan 18, 2024

View reviewed changes

github-actions bot added the effort/medium Medium work item – several days of effort label Jan 18, 2024

badmintoncryer marked this pull request as ready for review January 18, 2024 06:47

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

lpizzinidev suggested changes Jan 21, 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 Jan 21, 2024

lpizzinidev approved these changes Jan 21, 2024

View reviewed changes

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jan 21, 2024

aaythapa reviewed Jan 23, 2024

View reviewed changes

packages/aws-cdk-lib/aws-route53/lib/record-set.ts Outdated Show resolved Hide resolved

kaizencc changed the title ~~feat(route53): support latency based routing~~ feat(route53): latency based routing Jan 23, 2024

badmintoncryer added 8 commits January 24, 2024 04:16

feat: add setIdentifier

5801950

test: integ test

59a923c

test: update integ test

e12ad3b

fix: addressing comments

625a429

feat: auto generate setIdentifier

c1bf60a

test: update integ testy

5c91f38

feat: setIdentifier cannot specify in simple routing

d77163c

feat: add region props

3bd4700

badmintoncryer and others added 10 commits January 24, 2024 04:16

comment

dd20819

docs: README

01cdfb1

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

e6856af

Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>

Update packages/aws-cdk-lib/aws-route53/lib/record-set.ts

74b8236

Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>

Update packages/aws-cdk-lib/aws-route53/lib/record-set.ts

bb799a0

Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>

Update packages/aws-cdk-lib/aws-route53/lib/record-set.ts

47bfde4

Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>

Update packages/aws-cdk-lib/aws-route53/lib/record-set.ts

6ba0873

Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>

fix: unit test

d73d793

record-set.ts を更新

f4ae7d1

Co-authored-by: Aayush thapa <84202325+aaythapa@users.noreply.github.com>

refactor: add LatencyBasedRoutingRegion

f7e858e

badmintoncryer force-pushed the 28722-latencyBasedRouting branch from 6a7b044 to f7e858e Compare January 23, 2024 19:16

aws-cdk-automation previously requested changes Jan 23, 2024

View reviewed changes

badmintoncryer added 2 commits January 24, 2024 04:41

test: integ test

22266ce

Merge branch 'main' into 28722-latencyBasedRouting

110dbc3

Merge branch 'main' into 28722-latencyBasedRouting

4e55d79

badmintoncryer added 2 commits January 24, 2024 15:48

Revert "test: integ test"

82803e4

This reverts commit 22266ce.

Revert "refactor: add LatencyBasedRoutingRegion"

ed8f582

This reverts commit f7e858e.

Merge branch 'main' into 28722-latencyBasedRouting

afdc41e

aaythapa approved these changes Jan 24, 2024

View reviewed changes

mergify bot merged commit 169fd91 into aws:main Jan 24, 2024
10 checks passed

badmintoncryer deleted the 28722-latencyBasedRouting branch January 24, 2024 21:12

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

Monthly PR metrics report - Jan 2024 #28952

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(route53): latency based routing #28723

feat(route53): latency based routing #28723

badmintoncryer commented Jan 16, 2024 •

edited

aws-cdk-automation left a comment

lpizzinidev left a comment

badmintoncryer commented Jan 21, 2024 •

edited

lpizzinidev left a comment

aaythapa left a comment •

edited

badmintoncryer commented Jan 23, 2024 •

edited

aws-cdk-automation left a comment

badmintoncryer commented Jan 23, 2024

aaythapa commented Jan 23, 2024

badmintoncryer commented Jan 24, 2024

aws-cdk-automation commented Jan 24, 2024

mergify bot commented Jan 24, 2024

feat(route53): latency based routing #28723

feat(route53): latency based routing #28723

Conversation

badmintoncryer commented Jan 16, 2024 • edited

aws-cdk-automation left a comment

Choose a reason for hiding this comment

lpizzinidev left a comment

Choose a reason for hiding this comment

badmintoncryer commented Jan 21, 2024 • edited

lpizzinidev left a comment

Choose a reason for hiding this comment

aaythapa left a comment • edited

Choose a reason for hiding this comment

badmintoncryer commented Jan 23, 2024 • edited

aws-cdk-automation left a comment

Choose a reason for hiding this comment

badmintoncryer commented Jan 23, 2024

aaythapa commented Jan 23, 2024

badmintoncryer commented Jan 24, 2024

aws-cdk-automation commented Jan 24, 2024

AWS CodeBuild CI Report

mergify bot commented Jan 24, 2024

badmintoncryer commented Jan 16, 2024 •

edited

badmintoncryer commented Jan 21, 2024 •

edited

aaythapa left a comment •

edited

badmintoncryer commented Jan 23, 2024 •

edited