Skip to content
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

Upgrade type structure for Fn::FindInMappings #76

Merged
merged 1 commit into from
Oct 23, 2022
Merged

Conversation

iph
Copy link
Collaborator

@iph iph commented Oct 23, 2022

When passing down types into an Fn::Mapping, I originally expected to continue passing down the types into deeply nested structures.

This actually doesn't make sense. As an example, let's take a look at ReadCapacityUnits in DDB:

"ReadCapacityUnits" : 3000

Simple. It requires an integer.

When reading this fun mutated version:

"ReadCapacityUnits": {"Fn::FindInMap": ["DDBValue", "Table"] }

or something or other. ReadCapacityUnits is indeed still requiring an integer, but FindInMap is not, it is requiring two input strings in an array. The original code would pass the complexity type from the parent (in this case RSU), but that is incorrect and so would not work for resolving anything other than strings in maps. In this newest iteration, Mappings will now only resolve strings. This may cause chaos down the road, but according to the spec, all mapping keys are strings, so I'll allow it for now.

When passing down types into an Fn::Mapping, I originally expected
to continue passing down the types into deeply nested structures.

This actually doesn't make sense. As an example, let's take a look
at ReadCapacityUnits in DDB:

```
"ReadCapacityUnits" : 3000
```

Simple. It requires an integer.

When reading this fun mutated version:

```
"ReadCapacityUnits": {"Fn::FindInMap": ["DDBValue", "Table"] }
```

or something or other. ReadCapacityUnits is indeed still requiring an
integer, but FindInMap is not, it is requiring two input strings in
an array. The original code would pass the complexity type from the
parent (in this case RSU), but that is incorrect and so would not work
for resolving anything other than strings in maps. In this newest iteration,
Mappings will now only resolve strings. This may cause chaos down the road,
but according to the spec, all mapping keys are strings, so I'll allow it
for now.
@iph iph merged commit 905c959 into main Oct 23, 2022
@iph iph deleted the fix-mapping-indexes branch October 23, 2022 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant