-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
CFn: implement language extensions transform #12813
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
Conversation
Test Results (amd64) - Integration, Bootstrap 5 files 5 suites 2h 18m 2s ⏱️ Results for commit 4dc46ee. ♻️ This comment has been updated with latest results. |
69e9fb6 to
5991ea7
Compare
So we can have well scoped templates going forward
5991ea7 to
58b5e60
Compare
|
|
||
|
|
||
| @dataclass | ||
| class ResolveRefsRecursivelyContext: |
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.
Slight improvement to the resolve_refs_recursively API however I don't plan on continuing this refactoring
| ) | ||
| if resource_provider is None and not config.CFN_IGNORE_UNSUPPORTED_RESOURCE_TYPES: | ||
| raise NoResourceProvider | ||
| if resource_provider is None: |
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 is a fix for a previous PR where I incorrectly logged the not available message regardless of whether we could load the resource provider or not.
| state.stacks[stack.stack_id] = stack | ||
| return CreateStackOutput(StackId=stack.stack_id) | ||
|
|
||
| # HACK: recreate the stack (including all of its confusing processes in the __init__ method |
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 comment is to add clarity more than anything else. The movement of the construction of the stack is not relevant here
| copy_template = clone(stack.template_original) | ||
| copy_template.pop("ChangeSetName", None) | ||
| copy_template.pop("StackName", None) | ||
| for key in [ |
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.
There are other keys that are not present in the response, so clean up here
|
|
||
| if template_path is not None: | ||
| template = load_template_file(template_path) | ||
| if template is None: |
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 has annoyed me SO much!
|
|
||
| def _visit(obj, path, **_): | ||
| # Fn::ForEach | ||
| # TODO: can this be used in non-resource positions? |
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.
Yes, yes it can... 🤦♂️
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.
Maybe we should delete the TODO
pinzon
left a comment
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.
Thank you for implementing this. The changes look good. Hopefully this will be enough for the use case.
Motivation
We have had many requests for the Language Extensions Transform. These provide additional useful functionality to CloudFormation.
In particular, the
Fn::ForEachfunction is useful for generating many templated resources.Changes
Unfortunately the implementation is quite dense, relying on
recurse_objectwhich creates some complex situations. Though I found it quite straightforward to write the implementation, I feel it might be confusing to read.resolve_refs_recursivelyto use aResolveRefsRecursivelyContextdataclasswithresolve(thing)methodget_templateandget_template_resourcesexpand_fn_foreachso we can quicker iterate cases that don't require deployment of CFn templates