search

aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.51k stars 3.85k forks source link

aws-location-alpha: dataSource property should be required in PlaceIndex class #31288

Closed mazyu36 closed 3 days ago

mazyu36 commented 1 week ago

Describe the bug

Currently, dataSource property in PlaceIndex class is optional.

But in CFn document, DataSource property is required.

I think it would be preferable to make this a required field in accordance with CloudFormation.

In #30682, the dataSource property was also implemented, and based on the review comment, it was changed from optional to required.

https://github.com/aws/aws-cdk/pull/30682/files#r1734958764

Regression Issue

Last Known Working CDK Version

No response

Expected Behavior

The dataSource property is required.

Current Behavior

The dataSource property is optional.

Reproduction Steps

When use PlaceIndex class without dataSource property.

Possible Solution

Change dataSource property to required.

Additional Information/Context

No response

CDK CLI Version

2.151.0

Framework Version

No response

Node.js Version

All

OS

MacOS

Language

TypeScript

Language Version

No response

Other information

I have created an issue to discuss whether the modification should be implemented.

Considering the consistency with the RouteCalculator class and the CFn documentation, I think it would be better to make the dataSource required. I recognize that this would be a breaking change, but aws-location is an alpha module.

However, I think this change is not mandatory.

Please let me know your thoughts.

ashishdhingra commented 1 week ago

CloudFormation schema aws-location-placeindex.json specifies DataSource as required field:

...
  "required" : [ "DataSource", "IndexName" ],
...
kaizencc commented 1 week ago

Copying my comment on the PR here:

A property being required on CFN does not necessarily mean we have to require it in CDK, if we can find a reasonable default for it. That's what we've done here -- we are not sending undefined into CFN and getting an error back, we are providing what we think is a sane default.

It sounds like you don't think that DataSource.ESRI is a reasonable default -- before making that change, I think that's a better discussion to have in an issue.

github-actions[bot] commented 3 days ago

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.