Skip to content

implemented phone ops, added tests #13449

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
baermat merged 4 commits into from
Dec 5, 2025
Merged

implemented phone ops, added tests #13449

baermat merged 4 commits into from
Dec 5, 2025

Conversation

@baermat
Copy link
Member

@baermat baermat commented Dec 1, 2025 •
edited
Loading

Motivation

This PR adds phone number operations in regards to opting out to SNS. The main challenge was to test this against AWS, since many restrictions apply and a phone number can only be opted back in once every 30(!) days

Closes PNX-87

Changes

  • adds opt_in_phone_number, list_phone_numbers_opted_out, check_if_phone_number_is_opted_out
  • add AWS validated tests

Tests

Related

@baermat baermat added semver: minor Non-breaking changes which can be included in minor releases, but not 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 Dec 1, 2025
@github-actions
Copy link

github-actions bot commented Dec 1, 2025 •
edited
Loading

Test Results - Preflight, Unit

22 889 tests  ±0   21 075 ✅ ±0   6m 22s ⏱️ -5s
     1 suites ±0    1 814 💤 ±0 
     1 files   ±0        0 ❌ ±0 

Results for commit bdd3c30. ± Comparison against base commit c603057.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Dec 1, 2025 •
edited
Loading

Test Results - Alternative Providers

202 tests   - 1 261   158 ✅  - 726   2m 10s ⏱️ - 36m 6s
  1 suites  -     4    44 💤  - 535 
  1 files    -     4     0 ❌ ±  0 

Results for commit bdd3c30. ± Comparison against base commit c603057.

This pull request removes 1265 and adds 4 tests. Note that renamed tests count towards both. 
tests.aws.services.cloudformation.api.test_changesets ‑ test_autoexpand_capability_requirement
tests.aws.services.cloudformation.api.test_changesets ‑ test_create_and_then_remove_non_supported_resource_change_set
tests.aws.services.cloudformation.api.test_changesets ‑ test_create_and_then_remove_supported_resource_change_set
tests.aws.services.cloudformation.api.test_changesets ‑ test_create_and_then_update_refreshes_template_metadata
tests.aws.services.cloudformation.api.test_changesets ‑ test_create_change_set_create_existing
tests.aws.services.cloudformation.api.test_changesets ‑ test_create_change_set_invalid_params
tests.aws.services.cloudformation.api.test_changesets ‑ test_create_change_set_missing_stackname
tests.aws.services.cloudformation.api.test_changesets ‑ test_create_change_set_no_changes
tests.aws.services.cloudformation.api.test_changesets ‑ test_create_change_set_update_nonexisting
tests.aws.services.cloudformation.api.test_changesets ‑ test_create_change_set_update_without_parameters
…

tests.aws.services.sns.test_sns.TestSNSSMS ‑ test_is_phone_number_opted_out
tests.aws.services.sns.test_sns.TestSNSSMS ‑ test_list_phone_numbers_opted_out
tests.aws.services.sns.test_sns.TestSNSSMS ‑ test_opt_in_non_existing_phone_number
tests.aws.services.sns.test_sns.TestSNSSMS ‑ test_opt_in_phone_number

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Dec 1, 2025 •
edited
Loading

Test Results (amd64) - Acceptance

7 tests  ±0   5 ✅ ±0   3m 32s ⏱️ +13s
1 suites ±0   2 💤 ±0 
1 files   ±0   0 ❌ ±0 

Results for commit bdd3c30. ± Comparison against base commit c603057.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Dec 1, 2025 •
edited
Loading

Test Results (amd64) - Integration, Bootstrap

    5 files  ±    0      5 suites  ±0   1h 28m 22s ⏱️ - 1h 12m 23s
3 115 tests  - 2 362  2 946 ✅  - 1 979  169 💤  - 383  0 ❌ ±0 
3 121 runs   - 2 362  2 946 ✅  - 1 979  175 💤  - 383  0 ❌ ±0 

Results for commit bdd3c30. ± Comparison against base commit c603057.

This pull request removes 2366 and adds 4 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.sns.test_sns.TestSNSSMS ‑ test_is_phone_number_opted_out
tests.aws.services.sns.test_sns.TestSNSSMS ‑ test_list_phone_numbers_opted_out
tests.aws.services.sns.test_sns.TestSNSSMS ‑ test_opt_in_non_existing_phone_number
tests.aws.services.sns.test_sns.TestSNSSMS ‑ test_opt_in_phone_number
This pull request removes 386 skipped tests and adds 3 skipped tests. 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.cloudcontrol.test_cloudcontrol_api.TestCloudControlResourceApi ‑ test_api_exceptions
tests.aws.services.cloudcontrol.test_cloudcontrol_api.TestCloudControlResourceApi ‑ test_create_exceptions
tests.aws.services.cloudcontrol.test_cloudcontrol_api.TestCloudControlResourceApi ‑ test_create_invalid_desiredstate
tests.aws.services.cloudcontrol.test_cloudcontrol_api.TestCloudControlResourceApi ‑ test_double_create_with_client_token
tests.aws.services.cloudcontrol.test_cloudcontrol_api.TestCloudControlResourceApi ‑ test_lifecycle
…

tests.aws.services.sns.test_sns.TestSNSSMS ‑ test_is_phone_number_opted_out
tests.aws.services.sns.test_sns.TestSNSSMS ‑ test_list_phone_numbers_opted_out
tests.aws.services.sns.test_sns.TestSNSSMS ‑ test_opt_in_phone_number

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Dec 1, 2025 •
edited
Loading

LocalStack Community integration with Pro

    2 files  ±    0      2 suites  ±0   1h 0m 56s ⏱️ - 1h 3m 18s
3 091 tests  - 2 012  2 919 ✅  - 1 792  172 💤  - 220  0 ❌ ±0 
3 093 runs   - 2 012  2 919 ✅  - 1 792  174 💤  - 220  0 ❌ ±0 

Results for commit bdd3c30. ± Comparison against base commit c603057.

This pull request removes 2016 and adds 4 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.sns.test_sns.TestSNSSMS ‑ test_is_phone_number_opted_out
tests.aws.services.sns.test_sns.TestSNSSMS ‑ test_list_phone_numbers_opted_out
tests.aws.services.sns.test_sns.TestSNSSMS ‑ test_opt_in_non_existing_phone_number
tests.aws.services.sns.test_sns.TestSNSSMS ‑ test_opt_in_phone_number
This pull request removes 223 skipped tests and adds 3 skipped tests. 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.cloudcontrol.test_cloudcontrol_api.TestCloudControlResourceApi ‑ test_api_exceptions
tests.aws.services.cloudcontrol.test_cloudcontrol_api.TestCloudControlResourceApi ‑ test_create_exceptions
tests.aws.services.cloudcontrol.test_cloudcontrol_api.TestCloudControlResourceApi ‑ test_create_invalid_desiredstate
tests.aws.services.cloudcontrol.test_cloudcontrol_api.TestCloudControlResourceApi ‑ test_double_create_with_client_token
tests.aws.services.cloudcontrol.test_cloudcontrol_api.TestCloudControlResourceApi ‑ test_lifecycle
…

tests.aws.services.sns.test_sns.TestSNSSMS ‑ test_is_phone_number_opted_out
tests.aws.services.sns.test_sns.TestSNSSMS ‑ test_list_phone_numbers_opted_out
tests.aws.services.sns.test_sns.TestSNSSMS ‑ test_opt_in_phone_number

♻️ This comment has been updated with latest results.

@baermat baermat marked this pull request as ready for review December 3, 2025 09:25
@baermat baermat requested a review from bentsku as a code owner December 3, 2025 09:25
bentsku
bentsku approved these changes Dec 3, 2025
View reviewed changes
Copy link
Contributor

@bentsku bentsku left a comment

Choose a reason for hiding this comment

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

LGTM, nice implementing those methods! I just have a question regarding the expected usage of those operations, and if we're planning any follow up to allow users to actually use those methods?

In its current form, it seems pretty much hardcoded to an empty list. It doesn't have to be implemented now, I suppose there were some hardcoded values in Moto before?

In any case, one small nit would be to set that value name to lowercase if it's not a constant 👍

localstack-core/localstack/services/sns/v2/models.py

TAGS: TaggingService = CrossRegionAttribute(default=TaggingService)

PHONE_NUMBERS_OPTED_OUT: list[PhoneNumber] = CrossRegionAttribute(default=list)
Copy link
Contributor

@bentsku bentsku Dec 3, 2025

Choose a reason for hiding this comment

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

question: is this in uppercase because it is supposed to be a constant? if that's the case, does it make sense to be in the store?

if it's not a constant, then maybe it could be a lowercase variable

localstack-core/localstack/services/sns/v2/provider.py
#
# Phone number operations
#

Copy link
Contributor

@bentsku bentsku Dec 3, 2025

Choose a reason for hiding this comment

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

question about PHONE_NUMBERS_OPTED_OUT, how are users expected to add values to that? In its current form, using an empty list would have the same result, is that right?

Are we expecting to add a new internal route in the future to allow users to add values to this field? if yes, maybe putting it back to lowercase would make sense

tests/aws/services/sns/test_sns.py

@markers.aws.manual_setup_required
@pytest.mark.skipif(is_sns_v1_provider(), reason="Not correctly implemented in v1")
def test_is_phone_number_opted_out(
Copy link
Contributor

@bentsku bentsku Dec 3, 2025

Choose a reason for hiding this comment

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

nice work validating those tests 👌

@baermat baermat merged commit 5253193 into main Dec 5, 2025
45 checks passed
@baermat baermat deleted the sns/v2-phone-number-operations branch December 5, 2025 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bentsku bentsku bentsku approved these changes

Assignees

No one assigned

Labels

docs: skip Pull request does not require documentation changes notes: skip Pull request does not have to be mentioned in the release notes semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@baermat @bentsku