feat(core): Fn.findInMap supports default value by scanlonp · Pull Request #26543 · aws/aws-cdk

scanlonp · 2023-07-27T21:17:51Z

Cloudformation recently added support for a default value in the FindInMap intrinsic function. This adds that support.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

Update fork

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.

mrgrain

This one should have an integration test!

Also if you read the announcement carefully, you'll notice it mentions the Language Extension Transform. What do you make of that in regards to this PR?

packages/aws-cdk-lib/core/lib/cfn-mapping.ts

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.

scanlonp

I had a couple of lingering questions about comment guidelines and where code should live. I feel confident with the PR as is, but wanted to get feedback on those points in particular.

scanlonp · 2023-08-03T20:28:15Z

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

 regionTable.findInMap(Aws.REGION, 'regionName');
 ```

+An optional default value can also be passed to `findInMap`. If either key is not found in the map and the mapping is lazy, `findInMap` will return the default value. If the mapping is not lazy or either key is an unresolved token, the call to `findInMap` will return a token that resolves to `{ "Fn::FindInMap": [ "MapName", "TopLevelKey", "SecondLevelKey", { "DefaultValue": "DefaultValue" } ] }`. Note that the `AWS::LanguageExtentions` transform is added to enable the default value functionality.


Is the last line necessary?

packages/aws-cdk-lib/core/lib/cfn-fn.ts

scanlonp · 2023-08-03T20:37:33Z

packages/aws-cdk-lib/core/lib/cfn-fn.ts

+   * Prefer to use `CfnMapping.findInMap`.
+   * Warning: do not use with lazy maps.


Is this how we like to convey warnings / best practices?

I think you should explain succinctly why you should not use with lazy maps

packages/aws-cdk-lib/core/lib/cfn-fn.ts

rix0rrr

Nice! I've got some detail notes, but conditionally approved. Do with my comments as you will and feel free to remove the pr/do-not-merge label yourself when you are feel you have addressed them to the degree that you want to 😉.

One final thing: in the title, you're describing the feature using the Python casing Fn.find_in_map. In TypeScript the function is called Fn.findInMap, and we usually use that casing to describe changes.

packages/aws-cdk-lib/core/lib/cfn-fn.ts

rix0rrr · 2023-08-04T07:37:56Z

packages/aws-cdk-lib/core/lib/cfn-mapping.ts


  /**
-   * @returns A reference to a value in the map based on the two keys.
+   * @returns A reference to a value in the map based on the two keys. If mapping is lazy, the value from the map or default value is returned instead of the reference.


I'm pretty sure "lazy" means: only render the actual mapping to the template if there is at least one value that actually needs to be looked up from it at deploy time.

Conversely: if no value is ever read from the map, or all values can be resolved at synth time, then there is no need to render the map.

The line of documentation you added here doesn't help me understand that :).

Felt that it wasn't accurate to say it returns a reference is sometimes it just passes a value. Also could help debug if the short circuit was not documented. Can add a line about the mapping behavior too

rix0rrr · 2023-08-04T07:38:12Z

packages/aws-cdk-lib/core/lib/cfn-mapping.ts

+   * @returns A reference to a value in the map based on the two keys. If mapping is lazy, the value from the map or default value is returned instead of the reference.
   */
-  public findInMap(key1: string, key2: string): string {
+  public findInMap(key1: string, key2: string, defaultValue?: string): string {


Ohmigod. Can this method be simplified if we reorganize it?

Would be glad to, but not seeing a clear way. Think we ship it and someone enterprising can tidy it up later.

kaizencc

Have a few minor nits of my own lmao. Feel free to remove do-not-merge when you address the one comment

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

kaizencc · 2023-08-07T21:01:19Z

packages/aws-cdk-lib/core/lib/cfn-fn.ts

+   * Prefer to use `CfnMapping.findInMap`.
+   * Warning: do not use with lazy maps.


I think you should explain succinctly why you should not use with lazy maps

packages/aws-cdk-lib/core/lib/cfn-fn.ts

Outdated

mergify · 2023-08-08T11:16:58Z

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 · 2023-08-08T19:20:53Z

AWS CodeBuild CI Report

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

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

mergify · 2023-08-08T19:21:15Z

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

scanlonp added 6 commits September 21, 2022 10:33

Merge pull request #2 from aws/main

ae65409

Update fork

Merge branch 'aws:main' into main

2ebdad8

init soln, needs to consider lazy + be widespread

376c505

API update, fullyResolved not set right, need test

d03b8f8

logic works, added tests

7c427cd

docs updates

7e61955

github-actions bot added the p2 label Jul 27, 2023

aws-cdk-automation requested a review from a team July 27, 2023 21:18

scanlonp changed the title ~~feat(core): Fn.find_in~~ feat(core): Fn.find_in_map supports Default Value Jul 27, 2023

mergify bot added the contribution/core This is a PR that came from AWS. label Jul 27, 2023

aws-cdk-automation previously requested changes Jul 27, 2023

View reviewed changes

github-actions bot added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1 and removed p2 labels Jul 27, 2023

scanlonp added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Jul 27, 2023

scanlonp marked this pull request as draft July 27, 2023 21:28

scanlonp added 2 commits July 27, 2023 15:10

remove empty object in Fn::findInMap token

642a69e

README update

e550c7f

scanlonp marked this pull request as ready for review July 27, 2023 22:13

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

mrgrain previously requested changes Jul 27, 2023

View reviewed changes

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

add LanguageExtentions transform

75c6765

mrgrain reviewed Jul 29, 2023

View reviewed changes

packages/aws-cdk-lib/core/lib/cfn-mapping.ts Outdated Show resolved Hide resolved

github-actions bot mentioned this pull request Aug 1, 2023

Monthly PRs metrics report - July 2023 #26583

Closed

added integ test, still seeing odd behavior

3b671b5

scanlonp marked this pull request as draft August 1, 2023 23:15

update integ test

9ea3be0

add transform, working integ test

1a0b7f0

scanlonp removed the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Aug 3, 2023

aws-cdk-automation previously requested changes Aug 3, 2023

View reviewed changes

scanlonp added 2 commits August 3, 2023 12:18

Readme update

8f3366b

add integ snapshot

08f4b74

scanlonp marked this pull request as ready for review August 3, 2023 20:24

scanlonp commented Aug 3, 2023

View reviewed changes

rix0rrr added the pr/do-not-merge This PR should not be merged at this time. label Aug 4, 2023

rix0rrr approved these changes Aug 4, 2023

View reviewed changes

rix0rrr changed the title ~~feat(core): Fn.find_in_map supports Default Value~~ feat(core): Fn.findInMap supports default value Aug 4, 2023

scanlonp added 2 commits August 6, 2023 19:57

remove unnecessary function

159171a

docs update

bb555f3

kaizencc approved these changes Aug 7, 2023

View reviewed changes

picking last docs nits

2a48975

scanlonp removed the pr/do-not-merge This PR should not be merged at this time. label Aug 7, 2023

Merge branch 'main' into fn-find-in-map

b3fc6ae

mergify bot merged commit 8526feb into aws:main Aug 8, 2023

drobbins-ancile mentioned this pull request Aug 22, 2023

aws-cdk: Fn.find_in_map default_value not working in version 2.92.0 #26843

Closed

		* Prefer to use `CfnMapping.findInMap`.
		* Warning: do not use with lazy maps.

Conversation

scanlonp commented Jul 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Uh oh!

mrgrain 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!

scanlonp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rix0rrr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaizencc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mergify bot commented Aug 8, 2023

Uh oh!

aws-cdk-automation commented Aug 8, 2023

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Aug 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

scanlonp commented Jul 27, 2023 •

edited

Loading