CFNv2: parameter resolution and presentation by simonrw · Pull Request #12796 · localstack/localstack · GitHub | Latest TMZ Celebrity News & Gossip | Watch TMZ Live
Skip to content

CFNv2: parameter resolution and presentation #12796

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
Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

CFNv2: parameter resolution and presentation #12796

wants to merge 7 commits into from

Conversation

simonrw
Copy link
Contributor

@simonrw simonrw commented Jun 24, 2025 •
edited
Loading

Motivation

We currently don't support the resolution of SSM parameters in the template Parameters when performing template describing, only executing. This does not match AWS behaviour, which represents the resolved values as well as the given parameters.

Changes

  • Move SSM parameter resolution to the preprocessing step as it's performed during create_change_set, so we should do it before describing and executing
  • Add significant test for resolving SSM parameters
  • Remove skip from cdk bootstrap since it's no longer failing
  • Add structured type to parameters when describing so we can represent the original value (e.g. SSM parameter name) and resolved value (the SSM parameter value)

@simonrw simonrw added the semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases label Jun 24, 2025
@simonrw simonrw added the semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases label Jun 24, 2025
@simonrw simonrw marked this pull request as draft June 24, 2025 15:26
@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 24, 2025 •
edited
Loading

Test Results - Preflight, Unit

0 tests   - 21 795   0 ✅  - 20 138   0s ⏱️ - 6m 28s
0 suites  -      1   0 💤  -  1 657 
0 files    -      1   0 ❌ ±     0 

Results for commit 2ac855d. ± Comparison against base commit e558996.

♻️ This comment has been updated with latest results.

@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 24, 2025 •
edited
Loading

Test Results (amd64) - Acceptance

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

Results for commit 6c4cd17. ± Comparison against base commit 62f162f.

♻️ This comment has been updated with latest results.

@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 24, 2025 •
edited
Loading

LocalStack Community integration with Pro

  2 files  ±    0    2 suites  ±0   23m 0s ⏱️ - 1h 19m 46s
888 tests  - 4 025  323 ✅  - 3 814  565 💤  - 211  0 ❌ ±0 
890 runs   - 4 025  323 ✅  - 3 814  567 💤  - 211  0 ❌ ±0 

Results for commit 2ac855d. ± Comparison against base commit e558996.

This pull request removes 4026 and adds 1 tests. Note that renamed tests count towards both. 
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]
…

tests.aws.services.cloudformation.v2.test_change_sets ‑ test_dynamic_ssm_parameter_lookup
This pull request removes 212 skipped tests and adds 1 skipped test. Note that renamed tests count towards both. 
tests.aws.scenario.kinesis_firehose.test_kinesis_firehose.TestKinesisFirehoseScenario ‑ test_kinesis_firehose_s3
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input4-FAILED]
tests.aws.scenario.mythical_mysfits.test_mythical_misfits.TestMythicalMisfitsScenario ‑ test_deployed_infra_state
tests.aws.scenario.mythical_mysfits.test_mythical_misfits.TestMythicalMisfitsScenario ‑ test_populate_data
tests.aws.scenario.mythical_mysfits.test_mythical_misfits.TestMythicalMisfitsScenario ‑ test_user_clicks_are_stored
tests.aws.services.apigateway.test_apigateway_api.TestApiGatewayApiRestApi ‑ test_get_api_case_insensitive
tests.aws.services.apigateway.test_apigateway_api.TestApigatewayIntegration ‑ test_integration_response_invalid_responsetemplates
tests.aws.services.apigateway.test_apigateway_api.TestApigatewayIntegration ‑ test_put_integration_request_parameter_bool_type
tests.aws.services.apigateway.test_apigateway_basic.TestAPIGateway ‑ test_api_gateway_authorizer_crud
tests.aws.services.apigateway.test_apigateway_basic.TestAPIGateway ‑ test_api_gateway_http_integration_with_path_request_parameter
…

tests.aws.services.cloudformation.v2.test_change_sets ‑ test_dynamic_ssm_parameter_lookup

♻️ This comment has been updated with latest results.

@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 24, 2025 •
edited
Loading

Test Results (amd64) - Integration, Bootstrap

  5 files    5 suites   34m 34s ⏱️
911 tests 348 ✅ 563 💤 0 ❌
917 runs  348 ✅ 569 💤 0 ❌

Results for commit 6c4cd17.

♻️ This comment has been updated with latest results.

@simonrw simonrw marked this pull request as ready for review June 24, 2025 22:54
@simonrw simonrw requested a review from MEPalma June 24, 2025 22:55
MEPalma
MEPalma reviewed Jun 26, 2025
View reviewed changes
localstack-core/localstack/services/cloudformation/engine/v2/change_set_model.py
@@ -1382,7 +1394,7 @@ def _type_name_of(value: Maybe[Any]) -> str:

@staticmethod
def _is_terminal(value: Any) -> bool:
return type(value) in {int, float, bool, str, None, NothingType}
return type(value) in {int, float, bool, str, None, NothingType, ResolvedParameter}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you help me understand why we need this in the modeler?

MEPalma
MEPalma reviewed Jun 26, 2025
View reviewed changes
localstack-core/localstack/services/cloudformation/engine/v2/change_set_model_preproc.py
@@ -304,6 +308,10 @@ def _resolve_reference(self, logical_id: str) -> PreprocEntityDelta:
node_parameter = self._get_node_parameter_if_exists(parameter_name=logical_id)
if isinstance(node_parameter, NodeParameter):
parameter_delta = self.visit(node_parameter)
if isinstance(parameter_delta.before, ResolvedParameter):
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to first wrap the relevant values in a ResolvedParameter class and only evaluate them here?

MEPalma
MEPalma reviewed Jun 26, 2025
View reviewed changes
localstack-core/localstack/services/cloudformation/engine/v2/change_set_model_preproc.py
region_name=self._change_set.region_name,
stack_parameter_value=after,
)
after = ResolvedParameter(value=after, resolved_value=resolved_value)
Copy link
Contributor

Choose a reason for hiding this comment

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

why are we not resolving the resolved parameter here (this visitor function), but later?

@simonrw simonrw added this to the Playground milestone Jun 30, 2025
@simonrw simonrw force-pushed the cfn/v2/describe-parameters branch from 6c4cd17 to 2ac855d Compare July 3, 2025 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MEPalma MEPalma MEPalma left review comments

@dominikschubert dominikschubert Awaiting requested review from dominikschubert dominikschubert is a code owner

@pinzon pinzon Awaiting requested review from pinzon pinzon is a code owner

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

Assignees
No one assigned
Labels
semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases
Projects
None yet
Milestone
Playground
Development

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

TMZ Celebrity News – Breaking Stories, Videos & Gossip

Looking for the latest TMZ celebrity news? You've come to the right place. From shocking Hollywood scandals to exclusive videos, TMZ delivers it all in real time.

Whether it’s a red carpet slip-up, a viral paparazzi moment, or a legal drama involving your favorite stars, TMZ news is always first to break the story. Stay in the loop with daily updates, insider tips, and jaw-dropping photos.

🎥 Watch TMZ Live

TMZ Live brings you daily celebrity news and interviews straight from the TMZ newsroom. Don’t miss a beat—watch now and see what’s trending in Hollywood.