add support for Fn::Tranform in CFnV2 #12966
Conversation
a85bc97 to
aae7310
Compare
localstack-core/localstack/services/cloudformation/engine/v2/change_set_model.py
Outdated
Show resolved
Hide resolved
localstack-core/localstack/services/cloudformation/engine/v2/change_set_model.py
Show resolved
Hide resolved
8703ae2 to
b22a894
Compare
daa7743 to
c962e2f
Compare
| except FailedTransformationException as e: | ||
| change_set.status = ChangeSetStatus.FAILED | ||
| change_set.status_reason = e.message | ||
| change_set.stack.set_stack_status( | ||
| status=StackStatus.ROLLBACK_IN_PROGRESS, reason=e.message | ||
| ) | ||
| change_set.stack.set_stack_status(status=StackStatus.CREATE_FAILED) | ||
| return |
There was a problem hiding this comment.
@simonrw This is the section I'm not liking that much
There was a problem hiding this comment.
I think you are right, it's not general enough (e.g. it hard codes CREATE_FAILED) however I think as we get more testing around transforms in general this will work itself out.
e078fad to
3409251
Compare
Adding these docstrings helped me understand what the test did
There was a problem hiding this comment.
Amazing stuff, this was a really hard problem and it's solution is actually pretty clean. I am still not 100% on board with using the scope to manipulate the template structure however it is still quite a neat solution, and it prevents us from needing to add checks for Fn::Transform in parent nodes as well 👍
| before_siblings: list[Any], | ||
| after_siblings: list[Any], |
There was a problem hiding this comment.
question: I'm slightly confused why the arguments can be a ChangeSetEntity but the siblings have to be separate values. When I try to change it I end up re-visiting the transform and hitting the "Invalid: Fn::Transforms cannot be nested inside another Fn::Transform" case. I understand why (the Fn::Transform is included in the siblings) and I don't think it's worth it but perhaps we can try to switch to this type (i.e. make these a single siblings: ChangeSetEntity property) in a follow up
| "Invalid: Fn::Transforms cannot be nested inside another Fn::Transform" | ||
| ) | ||
|
|
||
| path = "$" + ".".join(scope.split("/")[:-1]) |
There was a problem hiding this comment.
suggestion: could this be replaced with
| path = "$" + ".".join(scope.split("/")[:-1]) | |
| path = scope.parent.jsonpath |
| except FailedTransformationException as e: | ||
| change_set.status = ChangeSetStatus.FAILED | ||
| change_set.status_reason = e.message | ||
| change_set.stack.set_stack_status( | ||
| status=StackStatus.ROLLBACK_IN_PROGRESS, reason=e.message | ||
| ) | ||
| change_set.stack.set_stack_status(status=StackStatus.CREATE_FAILED) | ||
| return |
There was a problem hiding this comment.
I think you are right, it's not general enough (e.g. it hard codes CREATE_FAILED) however I think as we get more testing around transforms in general this will work itself out.
|
|
||
| from localstack.aws.api.lambda_ import Runtime | ||
| from localstack.testing.pytest import markers | ||
| from localstack.utils.strings import short_uid | ||
|
|
||
|
|
||
| @pytest.mark.skip(reason="Not implemented with either provider") | ||
| @skip_if_v1_provider("change sets") |
| @@ -830,7 +825,7 @@ def test_attribute_uses_macro(self, deploy_cfn_template, create_lambda_function, | |||
| assert "test-" in resulting_value | |||
|
|
|||
| @markers.aws.validated | |||
| @pytest.mark.skip(reason="Fn::Transform does not support array of transformations") | |||
| @skip_if_v1_provider("V1 is unable to resolve fn::transform with lists") | |||
Motivation
This PR implements the capability to resolve FnTransform in the new CFnV2 engine in multiple locations of a template.
Changes
Testing
skip_if_v2_providerin related testsNotes
This PR only implements the resolution of the transformations in specific locations. More locations are going to be needed in the future