Skip to content

CFn: fix number parameter types #13231

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
merged 6 commits into from
Oct 8, 2025
Merged

CFn: fix number parameter types #13231

merged 6 commits into from
Oct 8, 2025

Conversation

@simonrw
Copy link
Contributor

@simonrw simonrw commented Oct 6, 2025

Motivation

A customer raised an issue with deploying their stack which contains a Number Parameter type. Specifically they were using it in a condition:

Parameters:
  Foo:
    Type: Number
Conditions:
  ShouldFrobble:
    Fn::Equals:
      - !Ref Foo
      - 1
Resources:
  MyConditionalResource:
    Condition: ShouldFrobble
    # ...

When supplied with the input [{"ParameterKey": "Value", "ParameterValue": "1"}] (all parameter values passed in are strings) the 1 was treated as a string, then compared to the integer 1 (i.e. "1" == 1), which is falsy and as such the condition evaluates to False.

Changes

TBD

@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 Oct 6, 2025
@github-actions
Copy link

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

Test Results - Preflight, Unit

22 298 tests  +6   20 555 ✅ +6   15m 59s ⏱️ +25s
     1 suites ±0    1 743 💤 ±0 
     1 files   ±0        0 ❌ ±0 

Results for commit 812284b. ± Comparison against base commit 928f470.

This pull request removes 1 and adds 7 tests. Note that renamed tests count towards both. 
tests.unit.test_common.TestCommon ‑ test_is_number

tests.unit.test_common.TestCommon ‑ test_is_number[-12.1-True]
tests.unit.test_common.TestCommon ‑ test_is_number[2000000000000000.0-True]
tests.unit.test_common.TestCommon ‑ test_is_number[5-True]
tests.unit.test_common.TestCommon ‑ test_is_number[False-False]
tests.unit.test_common.TestCommon ‑ test_is_number[None-False]
tests.unit.test_common.TestCommon ‑ test_is_number[True-False]
tests.unit.test_common.TestCommon ‑ test_is_number[test-False]

♻️ This comment has been updated with latest results.

@github-actions
Copy link

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

Test Results (amd64) - Acceptance

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

Results for commit 812284b. ± Comparison against base commit 928f470.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

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

Test Results - Alternative Providers

578 tests   - 772   330 ✅  - 371   25m 44s ⏱️ - 14m 22s
  1 suites  -   4   248 💤  - 401 
  1 files    -   4     0 ❌ ±  0 

Results for commit 812284b. ± Comparison against base commit 928f470.

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

tests.aws.services.cloudformation.resources.test_logs ‑ test_handle_existing_log_group
tests.aws.services.cloudformation.test_change_set_parameters.TestChangeSetParameters ‑ test_invalid_parameter_type
tests.aws.services.cloudformation.test_change_set_parameters.TestChangeSetParameters ‑ test_parameter_type_change
This pull request removes 404 skipped tests and adds 3 skipped tests. Note that renamed tests count towards both. 
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[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[smithy-rpc-v2-cbor]
tests.aws.services.cloudwatch.test_cloudwatch.TestCloudwatch ‑ test_anomaly_detector_lifecycle[json]
tests.aws.services.cloudwatch.test_cloudwatch.TestCloudwatch ‑ test_anomaly_detector_lifecycle[query]
tests.aws.services.cloudwatch.test_cloudwatch.TestCloudwatch ‑ test_anomaly_detector_lifecycle[smithy-rpc-v2-cbor]
…

tests.aws.services.cloudformation.resources.test_logs ‑ test_handle_existing_log_group
tests.aws.services.cloudformation.test_change_set_parameters.TestChangeSetParameters ‑ test_invalid_parameter_type
tests.aws.services.cloudformation.test_change_set_parameters.TestChangeSetParameters ‑ test_parameter_type_change

♻️ This comment has been updated with latest results.

@github-actions
Copy link

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

Test Results (amd64) - Integration, Bootstrap

    5 files  ± 0      5 suites  ±0   2h 40m 17s ⏱️ + 1m 44s
5 179 tests +10  4 681 ✅ +8  498 💤 +2  0 ❌ ±0 
5 185 runs  +10  4 681 ✅ +8  504 💤 +2  0 ❌ ±0 

Results for commit 812284b. ± Comparison against base commit 928f470.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

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

LocalStack Community integration with Pro

    2 files  ± 0      2 suites  ±0   2h 1m 36s ⏱️ + 2m 33s
4 805 tests +10  4 467 ✅ +8  338 💤 +2  0 ❌ ±0 
4 807 runs  +10  4 467 ✅ +8  340 💤 +2  0 ❌ ±0 

Results for commit 812284b. ± Comparison against base commit 928f470.

♻️ This comment has been updated with latest results.

@simonrw simonrw marked this pull request as ready for review October 7, 2025 22:58
pinzon
pinzon approved these changes Oct 8, 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 👍

localstack-core/localstack/utils/numbers.py
Comment on lines +18 to +19
if s is False or s is True:
return False
Copy link
Member

@pinzon pinzon Oct 8, 2025

Choose a reason for hiding this comment

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

Praise: Nice consideration.

tests/aws/services/cloudformation/resources/test_logs.py
parameters={"LogGroupName": log_group_name},
)

snapshot.match("failed-stack-describe", exc_info.value.describe_result)
Copy link
Member

@pinzon pinzon Oct 8, 2025

Choose a reason for hiding this comment

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

Nit: It could be useful to have the status reason if there was any.

Copy link
Contributor Author

@simonrw simonrw Oct 8, 2025

Choose a reason for hiding this comment

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

Sorry I'm not sure I understand. There is no StatusReason in the snapshot so do you mean we should set it even though it's not in parity?

Copy link
Member

@pinzon pinzon Oct 8, 2025

Choose a reason for hiding this comment

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

No. I'm talking about the StatusReason that should be in the CREATE_FAILED event of the Log Group.

@simonrw simonrw merged commit ccbd18a into main Oct 8, 2025
40 checks passed
@simonrw simonrw deleted the cfn/number-parameter-types branch October 8, 2025 05:52
baermat pushed a commit that referenced this pull request Oct 13, 2025
@simonrw @baermat
CFn: fix number parameter types (#13231)
9555926
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 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.

3 participants

@simonrw @pinzon