Skip to content

feat(core): lazy mappings will only synthesize if keys are unresolved#15617

Merged
mergify[bot] merged 11 commits intomasterfrom
chaimber/lazy-mapping
Jul 22, 2021
Merged

feat(core): lazy mappings will only synthesize if keys are unresolved#15617
mergify[bot] merged 11 commits intomasterfrom
chaimber/lazy-mapping

Conversation

@BenChaimberg
Copy link
Copy Markdown
Contributor

This feature adds new static methods to the CfnMapping construct that
allow the creation of "lazy" mappings. A lazy mapping will only create
a Mappings section in the synthesized CFN template if some "find"
operation on the mapping was not able to return a value since one or
more of the lookup keys were unresolved.

Usage:

// Register the mapping as a lazy mapping.
CfnMapping.registerLazyMap('UNIQUEMAPPINGID', {
  TopLevelKey: {
    SecondLevelKey: 'value',
  },
});

// Later, find a value from the mapping.  Since the keys are both
// resolved, this returns a resolved value and does not create a
// CfnMapping.
CfnMapping.findInLazyMap(scope, 'UNIQUEMAPPINGID', 'TopLevelKey', 'SecondLevelKey');
// > 'value'

// If we try to find a value from the mapping using unresolved keys, a
// CfnMapping is created and a Fn::FindInMap is returned.
CfnMapping.findInLazyMap(scope, 'UNIQUEMAPPINGID', 'TopLevelKey', Aws.REGION);
// > { Fn::FindInMap: [ 'UNIQUEMAPPINGID', 'TopLevelKey', { Ref: 'AWS::Region' } ] }

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

This feature adds new static methods to the CfnMapping construct that
allow the creation of "lazy" mappings. A lazy mapping will only create
a Mappings section in the synthesized CFN template if some "find"
operation on the mapping was not able to return a value since one or
more of the lookup keys were unresolved.

Usage:
```ts
// Register the mapping as a lazy mapping.
CfnMapping.registerLazyMap('UNIQUEMAPPINGID', {
  TopLevelKey: {
    SecondLevelKey: 'value',
  },
});

// Later, find a value from the mapping.  Since the keys are both
// resolved, this returns a resolved value and does not create a
// CfnMapping.
CfnMapping.findInLazyMap(scope, 'UNIQUEMAPPINGID', 'TopLevelKey', 'SecondLevelKey');
// > 'value'

// If we try to find a value from the mapping using unresolved keys, a
// CfnMapping is created and a Fn::FindInMap is returned.
CfnMapping.findInLazyMap(scope, 'UNIQUEMAPPINGID', 'TopLevelKey', Aws.REGION);
// > { Fn::FindInMap: [ 'UNIQUEMAPPINGID', 'TopLevelKey', { Ref: 'AWS::Region' } ] }
```
@BenChaimberg BenChaimberg requested review from a team and rix0rrr July 16, 2021 23:48
@gitpod-io
Copy link
Copy Markdown

gitpod-io bot commented Jul 16, 2021

Copy link
Copy Markdown
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to make this the default behavior of a normal map? (with an option to disable). What's the use of a map that is unused?

@BenChaimberg
Copy link
Copy Markdown
Contributor Author

Would it make sense to make this the default behavior of a normal map? (with an option to disable). What's the use of a map that is unused?

In my mind, most of the users who use CfnMapping are doing so for some sort of legacy CFN integration (combining with existing CFN templates, post-processing, ...). To that end, I think making a "template-only" breaking change to this construct by not rendering by default is probably not a good thing. I can still add this as a option into CfnMapping but I'm wondering if that's enough of an abstraction away from the "primitive" nature of the L1 that it deserves to be a separate Mapping L2 construct. What do you think?

@BenChaimberg BenChaimberg requested a review from eladb July 19, 2021 16:19

private informLazyUse() {
if (!this.lazyInformed) {
Annotations.of(this).addInfo('Consider making this CfnMapping a lazy mapping by providing `lazy: true`: either no findInMap was called or every findInMap could be immediately resolved without using Fn::FindInMap');
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Annotations.of(this).addInfo('Consider making this CfnMapping a lazy mapping by providing `lazy: true`: either no findInMap was called or every findInMap could be immediately resolved without using Fn::FindInMap');
Annotations.of(this).addWarning('Consider making this CfnMapping a lazy mapping by providing `lazy: true`: either no findInMap was called or every findInMap could be immediately resolved without using Fn::FindInMap');

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning feels a bit intense here since they're not necessarily doing anything wrong and I don't think strict synthesis should fail in this case

@eladb eladb added the pr/do-not-merge This PR should not be merged at this time. label Jul 22, 2021
@BenChaimberg BenChaimberg removed the pr/do-not-merge This PR should not be merged at this time. label Jul 22, 2021
@aws-cdk-automation
Copy link
Copy Markdown
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: b53969f
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify mergify bot merged commit 32ed229 into master Jul 22, 2021
@mergify mergify bot deleted the chaimber/lazy-mapping branch July 22, 2021 20:32
@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Jul 22, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Aug 3, 2021
…aws#15617)

This feature adds new static methods to the CfnMapping construct that
allow the creation of "lazy" mappings. A lazy mapping will only create
a Mappings section in the synthesized CFN template if some "find"
operation on the mapping was not able to return a value since one or
more of the lookup keys were unresolved.

Usage:
```ts
// Register the mapping as a lazy mapping.
CfnMapping.registerLazyMap('UNIQUEMAPPINGID', {
  TopLevelKey: {
    SecondLevelKey: 'value',
  },
});

// Later, find a value from the mapping.  Since the keys are both
// resolved, this returns a resolved value and does not create a
// CfnMapping.
CfnMapping.findInLazyMap(scope, 'UNIQUEMAPPINGID', 'TopLevelKey', 'SecondLevelKey');
// > 'value'

// If we try to find a value from the mapping using unresolved keys, a
// CfnMapping is created and a Fn::FindInMap is returned.
CfnMapping.findInLazyMap(scope, 'UNIQUEMAPPINGID', 'TopLevelKey', Aws.REGION);
// > { Fn::FindInMap: [ 'UNIQUEMAPPINGID', 'TopLevelKey', { Ref: 'AWS::Region' } ] }
```


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
hollanddd pushed a commit to hollanddd/aws-cdk that referenced this pull request Aug 26, 2021
…aws#15617)

This feature adds new static methods to the CfnMapping construct that
allow the creation of "lazy" mappings. A lazy mapping will only create
a Mappings section in the synthesized CFN template if some "find"
operation on the mapping was not able to return a value since one or
more of the lookup keys were unresolved.

Usage:
```ts
// Register the mapping as a lazy mapping.
CfnMapping.registerLazyMap('UNIQUEMAPPINGID', {
  TopLevelKey: {
    SecondLevelKey: 'value',
  },
});

// Later, find a value from the mapping.  Since the keys are both
// resolved, this returns a resolved value and does not create a
// CfnMapping.
CfnMapping.findInLazyMap(scope, 'UNIQUEMAPPINGID', 'TopLevelKey', 'SecondLevelKey');
// > 'value'

// If we try to find a value from the mapping using unresolved keys, a
// CfnMapping is created and a Fn::FindInMap is returned.
CfnMapping.findInLazyMap(scope, 'UNIQUEMAPPINGID', 'TopLevelKey', Aws.REGION);
// > { Fn::FindInMap: [ 'UNIQUEMAPPINGID', 'TopLevelKey', { Ref: 'AWS::Region' } ] }
```


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contribution/core This is a PR that came from AWS.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants