feat(location): support RouteCalculator by mazyu36 · Pull Request #30682 · aws/aws-cdk

mazyu36 · 2024-06-26T16:21:00Z

Issue # (if applicable)

Closes #30681 .

Reason for this change

In aws-location-alpha, route calculator has not been supported yet.

Description of changes

Add RouteCalculator class.

Description of how you validated changes

Add unit tests 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

mazyu36 · 2024-06-27T14:11:59Z

packages/@aws-cdk/aws-location-alpha/lib/route-calculator.ts

+   *
+   * @default DataSource.ESRI
+   */
+  readonly dataSource?: DataSource;


Currently, the DataSource for PlaceIndex and RouteCalculator are identical.
Therefore, I decided to reference the DataSource of PlaceIndex.
However, if there's a better approach, please share your opinion.

Ref:

https://docs.aws.amazon.com/location/latest/developerguide/what-is-data-provider.html

https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-location-placeindex.html#cfn-location-placeindex-datasource

https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-location-routecalculator.html#cfn-location-routecalculator-datasource

I also considered defining a completely separate Enum like RouteDataSource, but decided against it as I thought it would be redundant.
I also thought about moving PlaceIndex's DataSource to a separate file, but I think it would result in breaking changes...

Agree. Let's see if the core team has a different opinion.

I see in another PR of yours that you added a util file. I think it would be nice to move PlaceIndex's DataSource to somewhere more generic. Based on the CFN documentation, https://docs.aws.amazon.com/location/latest/developerguide/what-is-data-provider.html, it seems that all these three resources should re-use the DataSource prop.

but I think it would result in breaking changes
I don't see why this would be a breaking change. Can you explain that to me please?

Sorry, I misunderstood the breaking change. Although I needed to fix unit test within the package, it doesn't actually affect the usage of the package.

I've moved the DataSource to the util module.

lpizzinidev

Thanks 👍 Left some comments for minor adjustments

packages/@aws-cdk/aws-location-alpha/lib/route-calculator.ts

lpizzinidev · 2024-07-28T14:39:25Z

packages/@aws-cdk/aws-location-alpha/lib/route-calculator.ts

+   *
+   * @default DataSource.ESRI
+   */
+  readonly dataSource?: DataSource;


Agree. Let's see if the core team has a different opinion.

packages/@aws-cdk/aws-location-alpha/lib/route-calculator.ts

packages/@aws-cdk/aws-location-alpha/README.md

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

mazyu36 · 2024-07-29T15:07:29Z

@lpizzinidev
Thank you for approval !

…x class (#30974) ### Issue # (if applicable) N/A ### Reason for this change Add validation for PlaceIndex description, similar to #30648, #30682, and #30711 . ### Description of changes Add validation and unit tests for description. ### Description of how you validated changes Add unit 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*

GavinZZ

Generally LGTM, some minor feedbacks.

GavinZZ · 2024-08-28T16:08:40Z

packages/@aws-cdk/aws-location-alpha/lib/route-calculator.ts

+   *
+   * @default DataSource.ESRI
+   */
+  readonly dataSource?: DataSource;


I see in another PR of yours that you added a util file. I think it would be nice to move PlaceIndex's DataSource to somewhere more generic. Based on the CFN documentation, https://docs.aws.amazon.com/location/latest/developerguide/what-is-data-provider.html, it seems that all these three resources should re-use the DataSource prop.

but I think it would result in breaking changes
I don't see why this would be a breaking change. Can you explain that to me please?

GavinZZ · 2024-08-28T16:09:05Z

packages/@aws-cdk/aws-location-alpha/lib/route-calculator.ts

+  readonly description?: string;
+}
+
+abstract class RouteCalculatorBase extends Resource implements IRouteCalculator {


Same comment to another PR, don't think Base construct class is needed.

Thanks. I've removed base class.

GavinZZ · 2024-08-28T16:10:48Z

packages/@aws-cdk/aws-location-alpha/lib/route-calculator.ts

+
+    const routeCalculator = new CfnRouteCalculator(this, 'Resource', {
+      calculatorName: this.physicalName,
+      dataSource: props.dataSource ?? DataSource.ESRI,


I think CFN doc didn't specify a default value. I'm not familiar with this service/resource, can you briefly explain the reasoning of choosing ESRI as the default behaviour?

I aligned it with the already implemented PlaceIndex class.
https://github.com/aws/aws-cdk/blob/main/packages/%40aws-cdk/aws-location-alpha/lib/place-index.ts#L42

However, since it's listed as a required item in the CFn documentation, I've adjusted it to match that.

I think the DataSource for PlaceIndex should also be required, but I haven't made that change this time.

Pull request has been modified.

mazyu36 · 2024-08-29T09:31:31Z

@GavinZZ
Thank you for the review. I've addressed all your comments.

GavinZZ

LGTM, thank you!

mergify · 2024-08-29T17:17:39Z

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

mergify · 2024-08-29T17:21:47Z

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

…ulator

Pull request has been modified.

mazyu36 · 2024-08-29T22:57:50Z

@GavinZZ
Sorry, conflicts occurred due to the merge of #30711, so I resolved them.
Could you please review and approve again?

GavinZZ

LGTM, appreciate all the effort into making this resource available!

mergify · 2024-08-30T00:07: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-08-30T00:39:17Z

AWS CodeBuild CI Report

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

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

mergify · 2024-08-30T00:39:41Z

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-08-30T00:39:56Z

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.

feat: add route-calculator

0212a23

github-actions bot added the star-contributor [Pilot] contributed between 25-49 PRs to the CDK label Jun 26, 2024

aws-cdk-automation requested a review from a team June 26, 2024 16:21

github-actions bot added feature-request A feature should be added or improved. p2 labels Jun 26, 2024

mazyu36 added 2 commits June 27, 2024 01:40

Merge branch 'main' into location-route-calculator

68be912

fix: update README

21e2488

mazyu36 marked this pull request as ready for review June 27, 2024 14:05

mazyu36 commented Jun 27, 2024

View reviewed changes

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

mazyu36 added 2 commits June 27, 2024 23:14

Merge branch 'main' into location-route-calculator

3183553

Merge branch 'main' into location-route-calculator

4260c94

This was referenced Jul 1, 2024

Monthly PR metrics report #30715

Closed

Weekly PR metrics report #30720

Closed

Merge branch 'main' into location-route-calculator

056ffb9

lpizzinidev suggested changes Jul 28, 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 Jul 28, 2024

mazyu36 and others added 6 commits July 28, 2024 23:54

Update packages/@aws-cdk/aws-location-alpha/lib/route-calculator.ts

0c6608d

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

Update packages/@aws-cdk/aws-location-alpha/lib/route-calculator.ts

1e01265

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

Update packages/@aws-cdk/aws-location-alpha/README.md

3e99d7c

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

Update packages/@aws-cdk/aws-location-alpha/README.md

f23e19b

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

add description validation

3841404

update validation

7f12eec

mazyu36 force-pushed the location-route-calculator branch from cd26ab7 to 7f12eec Compare July 29, 2024 14:55

lpizzinidev approved these changes Jul 29, 2024

View reviewed changes

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

mazyu36 mentioned this pull request Jul 29, 2024

chore(location): add validation for description property in PlaceIndex class #30974

Merged

1 task

update docs

15c105a

Merge branch 'main' into location-route-calculator

e10a710

GavinZZ previously requested changes Aug 28, 2024

View reviewed changes

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

remove base class

6c8dff4

mazyu36 mentioned this pull request Aug 29, 2024

feat(location): support Map #30648

Merged

1 task

add datasource to util

5761669

mazyu36 force-pushed the location-route-calculator branch from ed060ab to 5761669 Compare August 29, 2024 07:32

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

mazyu36 requested a review from GavinZZ August 29, 2024 09:31

GavinZZ previously approved these changes Aug 29, 2024

View reviewed changes

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

Merge remote-tracking branch 'upstream/main' into location-route-calc…

ebf3bd2

…ulator

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

mazyu36 requested a review from GavinZZ August 29, 2024 22:58

GavinZZ approved these changes Aug 30, 2024

View reviewed changes

Merge branch 'main' into location-route-calculator

10380f0

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

mergify bot merged commit 574d383 into aws:main Aug 30, 2024

github-actions bot locked as resolved and limited conversation to collaborators Aug 30, 2024

mazyu36 deleted the location-route-calculator branch August 30, 2024 00:41

Conversation

mazyu36 commented Jun 26, 2024

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

Uh oh!

mazyu36 Jun 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpizzinidev Jul 28, 2024

Choose a reason for hiding this comment

Uh oh!

GavinZZ Aug 28, 2024

Choose a reason for hiding this comment

Uh oh!

mazyu36 Aug 29, 2024

Choose a reason for hiding this comment

Uh oh!

lpizzinidev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lpizzinidev Jul 28, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mazyu36 commented Jul 29, 2024

Uh oh!

GavinZZ left a comment

Choose a reason for hiding this comment

Uh oh!

GavinZZ Aug 28, 2024

Choose a reason for hiding this comment

Uh oh!

GavinZZ Aug 28, 2024

Choose a reason for hiding this comment

Uh oh!

mazyu36 Aug 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GavinZZ Aug 28, 2024

Choose a reason for hiding this comment

Uh oh!

mazyu36 Aug 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mazyu36 commented Aug 29, 2024

Uh oh!

GavinZZ left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Aug 29, 2024

Uh oh!

mergify bot commented Aug 29, 2024

Uh oh!

mazyu36 commented Aug 29, 2024

Uh oh!

GavinZZ left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Aug 30, 2024

Uh oh!

aws-cdk-automation commented Aug 30, 2024

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Aug 30, 2024

Uh oh!

github-actions bot commented Aug 30, 2024

Uh oh!

Reviewers

Assignees

Labels

mazyu36 Jun 27, 2024 •

edited

Loading

mazyu36 Aug 29, 2024 •

edited

Loading

mazyu36 Aug 29, 2024 •

edited

Loading