Skip to content
This repository was archived by the owner on Mar 23, 2026. It is now read-only.

CFn: fix number parameter types#13231

Merged
simonrw merged 6 commits intomainfrom
cfn/number-parameter-types
Oct 8, 2025
Merged

CFn: fix number parameter types#13231
simonrw merged 6 commits intomainfrom
cfn/number-parameter-types

Conversation

@simonrw
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown

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
Copy Markdown

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
Copy Markdown

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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

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

1 more reviewer

@pinzon pinzon pinzon approved these changes

Reviewers whose approvals may not affect merge requirements

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.

2 participants

@simonrw @pinzon