Skip to content

CFn: handle resolving parameter names constructed in intrinsics #13192

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
Merged
pinzon merged 8 commits into from
Sep 25, 2025
Merged

CFn: handle resolving parameter names constructed in intrinsics #13192

pinzon merged 8 commits into from
Sep 25, 2025

Conversation

@simonrw
Copy link
Contributor

@simonrw simonrw commented Sep 24, 2025 •
edited
Loading

Motivation

From #13169 we have an example of dynamic resolving where the name of the SSM parameter is built up from an Fn::Sub call, e.g.

Value: !Sub "abc:{{resolve:ssm:${ParameterName}}}"

This means the !Sub intrinsic should be invoked before the SSM parameter is resolved.

Changes

We now

  • perform better matching of whether a parameter is present
  • handle failure to resolve the parameter and assume that we will come back to it later

Also:

  • add a unit test for the regex matching
  • update an exisitng test to include this use case

Closes #13169

@simonrw simonrw added aws:cloudformation AWS CloudFormation semver: patch Non-breaking changes which can be included in patch releases docs: skip Pull request does not require documentation changes notes: skip Pull request does not have to be mentioned in the release notes labels Sep 24, 2025
@github-actions
Copy link

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

Test Results - Preflight, Unit

22 277 tests  +7   20 536 ✅ +7   15m 46s ⏱️ +17s
     1 suites ±0    1 741 💤 ±0 
     1 files   ±0        0 ❌ ±0 

Results for commit 72f71f2. ± Comparison against base commit 15fc427.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Sep 25, 2025 •
edited
Loading

LocalStack Community integration with Pro

    2 files  ±0      2 suites  ±0   2h 0m 37s ⏱️ -24s
4 781 tests ±0  4 445 ✅ ±0  336 💤 ±0  0 ❌ ±0 
4 783 runs  ±0  4 445 ✅ ±0  338 💤 ±0  0 ❌ ±0 

Results for commit 72f71f2. ± Comparison against base commit 15fc427.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Sep 25, 2025 •
edited
Loading

Test Results (amd64) - Acceptance

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

Results for commit 72f71f2. ± Comparison against base commit 15fc427.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Sep 25, 2025 •
edited
Loading

Test Results - Alternative Providers

572 tests   - 775   329 ✅  - 372   25m 51s ⏱️ - 13m 29s
  1 suites  -   4   243 💤  - 403 
  1 files    -   4     0 ❌ ±  0 

Results for commit 72f71f2. ± Comparison against base commit 15fc427.

This pull request removes 775 tests. 
tests.aws.services.cloudwatch.test_cloudwatch.TestCloudWatchMultiProtocol ‑ test_basic_operations_multiple_protocols[json]
tests.aws.services.cloudwatch.test_cloudwatch.TestCloudWatchMultiProtocol ‑ test_basic_operations_multiple_protocols[query]
tests.aws.services.cloudwatch.test_cloudwatch.TestCloudWatchMultiProtocol ‑ test_basic_operations_multiple_protocols[smithy-rpc-v2-cbor]
tests.aws.services.cloudwatch.test_cloudwatch.TestCloudWatchMultiProtocol ‑ test_exception_serializing_with_no_shape_in_spec[json]
tests.aws.services.cloudwatch.test_cloudwatch.TestCloudWatchMultiProtocol ‑ test_exception_serializing_with_no_shape_in_spec[query]
tests.aws.services.cloudwatch.test_cloudwatch.TestCloudWatchMultiProtocol ‑ test_exception_serializing_with_no_shape_in_spec[smithy-rpc-v2-cbor]
tests.aws.services.cloudwatch.test_cloudwatch.TestCloudWatchMultiProtocol ‑ test_multi_protocol_client_fixture[json]
tests.aws.services.cloudwatch.test_cloudwatch.TestCloudWatchMultiProtocol ‑ test_multi_protocol_client_fixture[query]
tests.aws.services.cloudwatch.test_cloudwatch.TestCloudWatchMultiProtocol ‑ test_multi_protocol_client_fixture[smithy-rpc-v2-cbor]
tests.aws.services.cloudwatch.test_cloudwatch.TestCloudwatch ‑ test_alarm_lambda_target
…
This pull request skips 1 test. 
tests.aws.services.cloudformation.test_template_engine.TestSsmParameters ‑ test_resolve_ssm

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Sep 25, 2025 •
edited
Loading

Test Results (amd64) - Integration, Bootstrap

    5 files  ±0      5 suites  ±0   2h 38m 38s ⏱️ - 1m 2s
5 155 tests ±0  4 659 ✅ ±0  496 💤 ±0  0 ❌ ±0 
5 161 runs  ±0  4 659 ✅ ±0  502 💤 ±0  0 ❌ ±0 

Results for commit 72f71f2. ± Comparison against base commit 15fc427.

♻️ This comment has been updated with latest results.

@simonrw simonrw added the review: merge when ready Signals to the reviewer that a PR can be merged if accepted label Sep 25, 2025
@simonrw simonrw marked this pull request as ready for review September 25, 2025 11:42
pinzon
pinzon approved these changes Sep 25, 2025
View reviewed changes
Copy link
Member

@pinzon pinzon left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for fixing this.

tests/aws/services/cloudformation/test_template_engine.py
Name=parameter_key, Overwrite=True, Type="String", Value=parameter_value_v2
)

versioned_parameter_reference = f"{parameter_key}:{v1['Version']}"
Copy link
Member

@pinzon pinzon Sep 25, 2025

Choose a reason for hiding this comment

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

Nit: You could have added the snapshot util here too.

@pinzon pinzon merged commit 339ad5c into main Sep 25, 2025
42 checks passed
@pinzon pinzon deleted the cfn/dynamic-parameter-resolving branch September 25, 2025 17:15
pinzon added a commit that referenced this pull request Sep 25, 2025
@pinzon
Revert "CFn: handle resolving parameter names constructed in intrinsi…
af82a02 
…cs (#13192)"

This reverts commit 339ad5c.
@alexrashed alexrashed added this to the 4.9 milestone Sep 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pinzon pinzon pinzon approved these changes

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

Assignees

No one assigned

Labels

aws:cloudformation AWS CloudFormation docs: skip Pull request does not require documentation changes notes: skip Pull request does not have to be mentioned in the release notes review: merge when ready Signals to the reviewer that a PR can be merged if accepted semver: patch Non-breaking changes which can be included in patch releases

Projects

None yet

Milestone

4.9

Development

Successfully merging this pull request may close these issues.

bug: CloudFormation dynamic reference regression 4.7 to 4.8

4 participants

@simonrw @pinzon @alexrashed