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

mazyu36 · 2024-06-30T08:01:08Z

Issue # (if applicable)

Closes #30710.

Reason for this change

To support L2 level geofence collection.

Description of changes

Add Geofence Collection 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

lpizzinidev

Thanks 👍 Left some comments for minor adjustments

packages/@aws-cdk/aws-location-alpha/lib/geofence-collection.ts

packages/@aws-cdk/aws-location-alpha/lib/util.ts

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

packages/@aws-cdk/aws-location-alpha/lib/geofence-collection.ts

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

…into location-geofence

mazyu36 · 2024-07-28T15:24:44Z

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

packages/@aws-cdk/aws-location-alpha/lib/geofence-collection.ts

lpizzinidev · 2024-07-29T12:35:52Z

packages/@aws-cdk/aws-location-alpha/lib/geofence-collection.ts

+
+  constructor(scope: Construct, id: string, props: GeofenceCollectionProps = {}) {
+
+    if (props.description && !Token.isUnresolved(props.geofenceCollectionName) && props.description.length > 1000) {


Suggested change

if (props.description && !Token.isUnresolved(props.geofenceCollectionName) && props.description.length > 1000) {

if (props.description !== undefined && !Token.isUnresolved(props.description) && props.description.length < 0 && props.description.length > 1000) {

Your implementation is not validating description: "".
Can you please add test coverage for the specific case as well?

Thanks.

I implemented the validation as follows. Since length is 0 or greater, I thought the condition length < 0 might be unnecessary. If my understanding is incorrect, please let me know.

aws-cdk/packages/@aws-cdk/aws-location-alpha/lib/geofence-collection.ts

Line 142 in bb82a5e

if (props.description !== undefined && !Token.isUnresolved(props.description) && props.description.length > 1000) {

I also added the case of description: "" to the unit tests. Furthermore, I confirmed that deployment is possible as follows.

You're right, I messed up 😄 Thanks for double-checking!

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

mazyu36 · 2024-07-29T13:49:05Z

@lpizzinidev
Thank you for the review.
I've fixed.

lpizzinidev

Thanks 👍

packages/@aws-cdk/aws-location-alpha/lib/geofence-collection.ts

Co-authored-by: Luca Pizzini <lpizzini7@gmail.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.

…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 · 2024-08-27T21:22:14Z

packages/@aws-cdk/aws-location-alpha/lib/geofence-collection.ts

+  readonly kmsKey?: kms.IKey;
+}
+
+abstract class GeofenceCollectionBase extends Resource implements IGeofenceCollection {


What's the reasoning behind creating a base class when it's only inherited by one class? Do you foresee another class extending this abstract class?

This is because the already implemented PlaceIndex class has a base class. I don't know if more classes will be added in the future, but I'm doing this to maintain consistency within the package.
I'd appreciate your opinion on this if you have any.

https://github.com/aws/aws-cdk/blob/main/packages/%40aws-cdk/aws-location-alpha/lib/place-index.ts#L95

I personally don't see the purpose of having a separate base class. I would recommend to just use one class instead. Otherwise the PR LGTM.

@GavinZZ
Thanks. I've removed the based class.
Would you please review again?

packages/@aws-cdk/aws-location-alpha/lib/util.ts

packages/@aws-cdk/aws-location-alpha/rosetta/default.ts-fixture

Pull request has been modified.

mazyu36 · 2024-08-28T00:36:39Z

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

GavinZZ

LGTM, thank you making the changes

mergify · 2024-08-29T17:10:27Z

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-29T17:42:19Z

AWS CodeBuild CI Report

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

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

mergify · 2024-08-29T17:42:38Z

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-29T17:42:54Z

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.

aws-cdk-automation requested a review from a team June 30, 2024 08:01

github-actions bot added star-contributor [Pilot] contributed between 25-49 PRs to the CDK feature-request A feature should be added or improved. p2 labels Jun 30, 2024

mazyu36 force-pushed the location-geofence branch from b9886ce to 4ea0ec1 Compare June 30, 2024 08:36

feat: add geofence collection

4b80527

mazyu36 force-pushed the location-geofence branch from 4ea0ec1 to 4b80527 Compare June 30, 2024 09:12

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 30, 2024

This was referenced Jul 1, 2024

Monthly PR metrics report #30715

Closed

Weekly PR metrics report #30720

Closed

Merge branch 'main' into location-geofence

5e5d121

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 3 commits July 28, 2024 23:41

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

4b22485

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

incorporate review comments

9324d26

Merge branch 'location-geofence' of https://github.com/mazyu36/aws-cdk …

baf15dd

…into location-geofence

mazyu36 requested a review from lpizzinidev July 28, 2024 15:24

lpizzinidev suggested changes Jul 29, 2024

View reviewed changes

mazyu36 and others added 3 commits July 29, 2024 22:01

Update packages/@aws-cdk/aws-location-alpha/lib/geofence-collection.ts

1a715e2

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

Update packages/@aws-cdk/aws-location-alpha/lib/geofence-collection.ts

df19851

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

incorporate review comments

bb82a5e

mazyu36 requested a review from lpizzinidev July 29, 2024 13:49

lpizzinidev approved these changes Jul 29, 2024

View reviewed changes

packages/@aws-cdk/aws-location-alpha/lib/geofence-collection.ts Outdated Show resolved Hide resolved

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

mazyu36 force-pushed the location-geofence branch from 4d72eaf to ad28fb4 Compare July 29, 2024 15:05

update validation

1eb8ff9

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

mazyu36 force-pushed the location-geofence branch from ad28fb4 to 1eb8ff9 Compare July 29, 2024 15:06

Merge branch 'main' into location-geofence

ff62d88

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

aws-cdk-automation previously requested changes Jul 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 Jul 29, 2024

mazyu36 changed the title ~~feat(location): Support GeofenceCollection~~ feat(location): support GeofenceCollection Jul 29, 2024

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

a063feb

GavinZZ previously requested changes Aug 27, 2024

View reviewed changes

GavinZZ self-assigned this Aug 27, 2024

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

mazyu36 added 2 commits August 28, 2024 09:02

incorporate review comments

9068e77

Merge branch 'main' into location-geofence

a9eb231

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

mazyu36 requested a review from GavinZZ August 28, 2024 00:37

remove base class

a624296

mazyu36 mentioned this pull request Aug 29, 2024

feat(location): support Map #30648

Merged

1 task

GavinZZ approved these changes Aug 29, 2024

View reviewed changes

Merge branch 'main' into location-geofence

0270867

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

mergify bot merged commit 04d73ac into aws:main Aug 29, 2024

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

mazyu36 deleted the location-geofence branch August 29, 2024 22:29


		constructor(scope: Construct, id: string, props: GeofenceCollectionProps = {}) {

		if (props.description && !Token.isUnresolved(props.geofenceCollectionName) && props.description.length > 1000) {

	if (props.description && !Token.isUnresolved(props.geofenceCollectionName) && props.description.length > 1000) {
	if (props.description !== undefined && !Token.isUnresolved(props.description) && props.description.length < 0 && props.description.length > 1000) {

Conversation

mazyu36 commented Jun 30, 2024

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

Uh oh!

lpizzinidev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mazyu36 commented Jul 28, 2024

Uh oh!

Uh oh!

lpizzinidev Jul 29, 2024

Choose a reason for hiding this comment

Uh oh!

mazyu36 Jul 29, 2024

Choose a reason for hiding this comment

Uh oh!

lpizzinidev Jul 29, 2024

Choose a reason for hiding this comment

Uh oh!

mazyu36 commented Jul 29, 2024

Uh oh!

lpizzinidev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Uh oh!

GavinZZ Aug 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!

mazyu36 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

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mazyu36 commented Aug 28, 2024

Uh oh!

GavinZZ left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Aug 29, 2024

Uh oh!

aws-cdk-automation commented Aug 29, 2024

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Aug 29, 2024

Uh oh!

github-actions bot commented Aug 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

GavinZZ Aug 27, 2024 •

edited

Loading