-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(cfn-include): allow dynamic mappings to be used in Fn::FindInMap #13428
fix(cfn-include): allow dynamic mappings to be used in Fn::FindInMap #13428
Conversation
The template parsing logic in cloudformation-include always searched for the Mapping in the templatebased on the first argument passed to Fn::FindInMap. However, that doesn't work if that first argument is a dynamic expression, like `{ Ref: Param }`. Check for that case explicitly, and don't search for the Mapping if the first argument to Fn::FindInMap is a dynamic expression.
mappingName = value[0]; | ||
} else { | ||
const mapping = this.finder.findMapping(value[0]); | ||
if (!mapping) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error seems at odds with allowing unresolved values. Is this error preventing something from breaking down the road? If so, will it break if this value is unresolved?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, this error is to prevent situations where the argument to Fn::FindInMap
is a static string (so something like { 'Fn::FinInMap': ['MappingName', 'Lvl1', 'Lvl2'] }
), but a Mapping with that name could not be found in the template (so, continuing the above example, MappingName
would not be in the template).
In case the value is dynamic, like in the test for this PR, we can't prevent this mistake from happening. We also can't handle things like re-naming the Mapping from CDK code correctly with dynamic mappings. But I think that's fine - we do the validation when we can, and when we can't, we just trust the template author they modeled things correctly.
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). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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). |
…ws#13428) The template parsing logic in cloudformation-include always searched for the Mapping in the template based on the first argument passed to Fn::FindInMap. However, that doesn't work if that first argument is a dynamic expression, like `{ Ref: Param }`. Check for that case explicitly, and don't search for the Mapping if the first argument to Fn::FindInMap is a dynamic expression. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
The template parsing logic in cloudformation-include always searched for the Mapping
in the template based on the first argument passed to Fn::FindInMap.
However, that doesn't work if that first argument is a dynamic expression,
like
{ Ref: Param }
.Check for that case explicitly, and don't search for the Mapping
if the first argument to Fn::FindInMap is a dynamic expression.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license