Add initial support for CFn Fn:Transform intrinsic functions#7711
Add initial support for CFn Fn:Transform intrinsic functions#7711
Conversation
LocalStack integration with Pro 1 files - 2 1 suites - 2 1h 1m 31s ⏱️ - 30m 50s For more details on these failures, see this check. Results for commit a7e3a12. ± Comparison against base commit 87feed2. ♻️ This comment has been updated with latest results. |
|
Test error in |
85548ab to
ec3cd6f
Compare
dominikschubert
left a comment
There was a problem hiding this comment.
Mostly small nits that don't block the merge, otherwise LGTM. I like the intrinsic function abstraction. It kinda resembles what I've seen so far in related tools and how I'd like us to approach the rest as well 👍
Request for change affects the recorded snapshot since it won't pass against AWS when re-running.
| from localstack.utils.aws import aws_stack | ||
| from localstack.utils.strings import str_to_bool | ||
|
|
||
| TAG_KEY_LOGICAL_ID = "aws:cloudformation:logical-id" |
There was a problem hiding this comment.
Is there something that explicitly breaks on this missing? Because I'm not sure about only implementing it here when it should actually be included nearly everywhere 🤔
There was a problem hiding this comment.
That's a great catch. This change actually hints at a general limitation we have in our approach (as you're well aware) - the way how we detect the resource state and the fact that we're allowing templates to be deployed on top of existing resources.
With the current implementation of (some of) our resource classes, in combination with the change above, the fetch_state method can sometimes return false positives - i.e., report that the resource already exists when in fact it doesn't exist yet.
This issue in particular occurs if we have multiple instances of resource types whose deployment state cannot easily be determined based on the resource properties, e.g., consider the example below (we have a similar example in our tests):
routeTable1:
Type: AWS::EC2::RouteTable
Properties:
VpcId: vpc-12345
routeTable2:
Type: AWS::EC2::RouteTable
Properties:
VpcId: vpc-12345
I've simplified the implementation - now the state lookup detects whether PhysicalResourceId has already been set (using the result_handler hook) and only then returns the resource details. This is quite a bit cleaner, and also should give us better parity with AWS (no additional tags). 👌
This also plays into the general discussion of resource state handling and the deployment loop (as we already discussed) - over time, we can refactor the approach and loosen the assumption that existing resources are "imported" into the stack idempotently if they already exist. This will give us even better parity with AWS over time. 👍
tests/integration/cloudformation/api/test_transformers.snapshot.json
Outdated
Show resolved
Hide resolved
Co-authored-by: Dominik Schubert <dominik.schubert91@gmail.com>
027d245 to
b361bcf
Compare
b361bcf to
a7e3a12
Compare
|
Tests against pro failed with |
dominikschubert
left a comment
There was a problem hiding this comment.
LGTM now 🚀
Thanks for addressing all the raised nits/concerns 👍
| transformers: Dict[str, Type] = {"AWS::Include": AwsIncludeTransformer} | ||
|
|
||
|
|
||
| def apply_transform_intrinsic_functions(template: dict, stack=None) -> dict: |
There was a problem hiding this comment.
While I understand why we currently need it, I'm not super happy about the stack class being in here again 😬
Add initial support for CFn
Fn:Transformintrinsic functions. This requirement came up while playing around with a more complex AWS sample app using a SAM / CloudFormation template: https://workshop.serverlesscoffee.comFor reference - part of the SAM template in the sample is a construct like this:
(the
Locationparameter is replaced with ans3://...URL by SAM upon deployment)