Skip to content

add support for Fn::Tranform in CFnV2 #12966

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 19 commits into
base: main
Choose a base branch
from
Draft

add support for Fn::Tranform in CFnV2 #12966

wants to merge 19 commits into from
+394 −163

Conversation

pinzon
Copy link
Member

@pinzon pinzon commented Aug 6, 2025

Motivation

Changes

@pinzon pinzon added the semver: patch Non-breaking changes which can be included in patch releases label Aug 6, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 6, 2025 •
edited
Loading

Test Results - Preflight, Unit

22 063 tests  ±0   20 329 ✅ ±0   6m 14s ⏱️ -31s
     1 suites ±0    1 734 💤 ±0 
     1 files   ±0        0 ❌ ±0 

Results for commit bd805fd. ± Comparison against base commit 4258f76.

♻️ This comment has been updated with latest results.

@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 6, 2025 •
edited
Loading

Test Results (amd64) - Acceptance

7 tests  ±0   5 ✅ ±0   3m 5s ⏱️ -17s
1 suites ±0   2 💤 ±0 
1 files   ±0   0 ❌ ±0 

Results for commit bd805fd. ± Comparison against base commit 4258f76.

♻️ This comment has been updated with latest results.

@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 6, 2025 •
edited
Loading

Test Results (amd64) - Integration, Bootstrap

  5 files    5 suites   35m 23s ⏱️
579 tests 355 ✅ 209 💤 15 ❌
585 runs  355 ✅ 215 💤 15 ❌

For more details on these failures, see this check.

Results for commit bd805fd.

♻️ This comment has been updated with latest results.

@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 6, 2025 •
edited
Loading

LocalStack Community integration with Pro

  2 files  ±    0    2 suites  ±0   24m 36s ⏱️ - 1h 18m 17s
555 tests  - 4 073  330 ✅  - 3 858  210 💤  - 230  15 ❌ +15 
557 runs   - 4 073  330 ✅  - 3 858  212 💤  - 230  15 ❌ +15 

For more details on these failures, see this check.

Results for commit bd805fd. ± Comparison against base commit 4258f76.

This pull request removes 4073 tests. 
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_lambda_dynamodb
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_opensearch_crud
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_search_books
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_setup
tests.aws.scenario.kinesis_firehose.test_kinesis_firehose.TestKinesisFirehoseScenario ‑ test_kinesis_firehose_s3
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_destination_sns
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_infra
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_prefill_dynamodb_table
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input0-SUCCEEDED]
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input1-SUCCEEDED]
…

♻️ This comment has been updated with latest results.

@pinzon pinzon force-pushed the cp/cfn/v2/transform branch from a85bc97 to aae7310 Compare August 7, 2025 19:40
simonrw
simonrw reviewed Aug 8, 2025
View reviewed changes
localstack-core/localstack/services/cloudformation/engine/v2/change_set_model.py

if intrinsic_function == FnTransform:
if scope.count(FnTransform) > 1:
raise RuntimeError("Nested Fn::Transforms are bad")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"are bad"! I like that error message!

But seriously... let's change that before we merge

localstack-core/localstack/services/cloudformation/engine/v2/change_set_model.py
if scope.count(FnTransform) > 1:
raise RuntimeError("Nested Fn::Transforms are bad")

path = "$" + ".".join(scope.split("/")[:-1])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do quite like the idea of making this implementation a helper on the Scope class, e.g.

class Scope(str):
    # ...
    def to_jsonpath(self) -> str:
        return "$" + ".".join(self.split("/"))

perhaps even adding a .parent property if we know we need to traverse up the tree

class Scope(str):
    # ...
    @property
    def parent(self) -> Self:
        return Scope(self._SEPARATOR.join(self.split(self._SEPARATOR)[:-1]))

or something

@pinzon pinzon force-pushed the cp/cfn/v2/transform branch from ebe637a to 0eee9ce Compare August 8, 2025 20:54
@pinzon pinzon force-pushed the cp/cfn/v2/transform branch from 6d41b4a to 11cc3b8 Compare August 11, 2025 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simonrw simonrw simonrw left review comments

@dominikschubert dominikschubert Awaiting requested review from dominikschubert dominikschubert will be requested when the pull request is marked ready for review dominikschubert is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pinzon @simonrw