Refactor CFn conditions & mappings #8546
Conversation
9c49445 to
4ed3175
Compare
4ed3175 to
79aeb73
Compare
| raise Exception( | ||
| f"Cannot resolve fn::FindInMap: {value[keys_list[0]]} {list(resources.keys())}" | ||
| f"Cannot find Mapping with ID {mapping_id} for Fn::FindInMap: {value[keys_list[0]]} {list(resources.keys())}" # TODO: verify |
There was a problem hiding this comment.
| f"Cannot find Mapping with ID {mapping_id} for Fn::FindInMap: {value[keys_list[0]]} {list(resources.keys())}" # TODO: verify | |
| f"Cannot find Mapping with ID {mapping_id} for Fn::FindInMap: {value[keys_list[0]]} {list(mappings.keys())}" # TODO: verify |
| if ref is None: | ||
| msg = 'Unable to resolve Ref for resource "%s" (yet)' % value["Ref"] | ||
| LOG.debug("%s - %s", msg, resources.get(value["Ref"]) or set(resources.keys())) | ||
| raise DependencyNotYetSatisfied(resource_ids=value["Ref"], message=msg) | ||
| ref = resolve_refs_recursively(stack_name, resources, ref) | ||
| ref = resolve_refs_recursively(stack_name, resources, mappings, ref) | ||
| return ref | ||
|
|
||
| if stripped_fn_lower == "getatt": |
There was a problem hiding this comment.
nit (unrelated to this PR): You probably have this on your refactoring list anyway, but I feel like we could introduce proper class abstractions for the different intrinsic functions at some point, instead of the if branches below. :) 👍
There was a problem hiding this comment.
Yup, that will be a whole intrinsic function rework. Unfortunately we're not quite there yet were we can replace the intrinsic function resolving. The idea we had here was to build a sort of "hydrated" stack that contains the whole dependency graph and a somwhat "rich" API to navigate it & resolve refs, etc.
There was a problem hiding this comment.
The current approach is also a bit of a problem because we allow things that we shouldn't. A lot of sections / Intrinsic functions only allow a subset of other intrinsic functions to be contained in their fields, which we currently can't easily model with this construct.
|
|
||
| return result.get(key) | ||
| return selected_map.get(first_level_attribute).get(second_level_attribute) |
There was a problem hiding this comment.
nit: Not sure if we want to hedge against lookup errors here, if the first level attribute is not contained in the map?
There was a problem hiding this comment.
As one of the tests shows, we should actually fail much harder than we currently do 😅 CloudFormation will immediately block the execute_changeset / create stack call while we still silently fail
| @@ -450,33 +448,44 @@ def _resolve_refs_recursively(stack_name, resources, value: dict | list | str | | |||
| item_to_sub[1].update(attr_refs) | |||
|
|
|||
| for key, val in item_to_sub[1].items(): | |||
| val = resolve_refs_recursively(stack_name, resources, val) | |||
| val = resolve_refs_recursively(stack_name, resources, mappings, val) | |||
There was a problem hiding this comment.
General observation / nit: With the various refactorings going on right now, it seems a bit brittle to rely mostly on positional args. Technically it should be highlighted by the IDE if incorrect param types are passed to a function, but wondering if we could sprinkle in a few keyword args, to reduce the surface area of potentially incorrect parameter passing. 🤷
There was a problem hiding this comment.
Yes! That's something I want to start doing (forcing kwargs usage instead of position arguments).
Technically it should be highlighted by the IDE if incorrect param types are passed to a function
In practice that hasn't worked out well unfortunately, which is why I'm also of the opinion that kwargs are usually to be preferred where possible.
It's extremely hard to refactor with positional args only especially since most of the GenericBaseModel implementations were using the signatures very inconsistently
| # TODO: ?? | ||
| mapping_id = resolve_ref(stack_name, resources, mappings, mapping_id["Ref"], "Ref") |
There was a problem hiding this comment.
a bit of context for the ?? here: I'm just not a fan of using resolve_ref here (because not all refs here are valid) in the long term but for now its fine.
Changes
Stack.resourcesfor a clearer data flow / future restructuringAWS::NoValuehandling and add corresponding tests