GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Sns/v2 topic migration by baermat · Pull Request #13205 · localstack/localstack

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

Conversation

@baermat

@baermat baermat commented Sep 29, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Motivation

First step of internalization of the SNS provider by migrating topics. This PR addresses both topics and topic attributes since they are intertwined

Changes

  • add create_topic, list_topics, delete_topics, get_topic_attributes, set_topic_attributes
  • activates all applicable tests in TestSnsTopicCrud
  • adds new class TestSnsTopicCrudV2 for tests that were covered in moto
    • The tests were only used as reference to "what is tested", the tests themselves were written from scratch

closes PNX-83, closes PNX-79

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
baermat added 5 commits September 29, 2025 11:30
@baermat
working test
2947736
@baermat
working test
63eab28
@baermat
working test
78c2f42
@baermat
add new test class based on moto, add tests to be verified
f282a46
@baermat
add moto tests
e364681
@baermat baermat added semver: patch Non-breaking changes which can be included in patch releases docs: skip Pull request does not require documentation changes semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases and removed semver: patch Non-breaking changes which can be included in patch releases labels Sep 29, 2025
@baermat
add todo
fe036cb
@baermat
baermat marked this pull request as ready for review September 29, 2025 12:14
@baermat
baermat requested a review from bentsku as a code owner September 29, 2025 12:14
@github-actions

github-actions Bot commented Sep 29, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

Test Results - Preflight, Unit

22 292 tests  +11   20 549 ✅ +11   15m 18s ⏱️ -34s
     1 suites ± 0    1 743 💤 ± 0 
     1 files   ± 0        0 ❌ ± 0 

Results for commit f713a3c. ± Comparison against base commit e60ff8e.

♻️ This comment has been updated with latest results.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions

github-actions Bot commented Sep 29, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

Test Results - Alternative Providers

132 tests     9 ✅  23s ⏱️
  1 suites  123 💤
  1 files      0 ❌

Results for commit f713a3c.

♻️ This comment has been updated with latest results.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions

github-actions Bot commented Sep 29, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

Test Results (amd64) - Acceptance

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

Results for commit f713a3c. ± Comparison against base commit e60ff8e.

♻️ This comment has been updated with latest results.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions

github-actions Bot commented Sep 29, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

LocalStack Community integration with Pro

    2 files  ±    0      2 suites  ±0   59m 50s ⏱️ - 59m 22s
2 824 tests  - 1 968  2 711 ✅  - 1 745  113 💤  - 223  0 ❌ ±0 
2 826 runs   - 1 968  2 711 ✅  - 1 745  115 💤  - 223  0 ❌ ±0 

Results for commit f713a3c. ± Comparison against base commit e60ff8e.

This pull request removes 1975 and adds 7 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.apigateway.test_apigateway_api.TestApigatewayTestInvoke ‑ test_failed_invoke_test_method
tests.aws.services.sns.test_sns.TestSNSTopicCrudV2 ‑ test_create_topic_in_multiple_regions
tests.aws.services.sns.test_sns.TestSNSTopicCrudV2 ‑ test_create_topic_name_constraints
tests.aws.services.sns.test_sns.TestSNSTopicCrudV2 ‑ test_create_topic_should_be_idempotent
tests.aws.services.sns.test_sns.TestSNSTopicCrudV2 ‑ test_delete_non_existent_topic
tests.aws.services.sns.test_sns.TestSNSTopicCrudV2 ‑ test_list_topic_paging
tests.aws.services.sns.test_sns.TestSNSTopicCrudV2 ‑ test_topic_get_attributes_with_fifo_false
This pull request removes 225 skipped tests and adds 2 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.TestSNSTopicCrudV2 ‑ test_create_topic_name_constraints
tests.aws.services.sns.test_sns.TestSNSTopicCrudV2 ‑ test_topic_get_attributes_with_fifo_false

♻️ This comment has been updated with latest results.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions

github-actions Bot commented Sep 29, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

Test Results (amd64) - Integration, Bootstrap

    5 files      5 suites   1h 28m 28s ⏱️
2 848 tests 2 738 ✅ 110 💤 0 ❌
2 854 runs  2 738 ✅ 116 💤 0 ❌

Results for commit f713a3c.

♻️ This comment has been updated with latest results.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bentsku
bentsku reviewed Sep 29, 2025
View reviewed changes

@bentsku bentsku left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Nice work, this is looking great! I believe this to be a sound base to continue building on 🚀

The only issue is using plain Python class for the store attributes, once this is resolved, I believe we're good to go.
The rest is minor nits and simplification of the code.

Nice new coverage with new snapshot tests, this is really great to see! 👌

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread localstack-core/localstack/services/sns/v2/models.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread localstack-core/localstack/services/sns/v2/models.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread localstack-core/localstack/services/sns/v2/provider.py
Comment on lines +53 to +54
# TODO:
raise InvalidParameterException("Fix this Exception message and type")

@bentsku bentsku Sep 29, 2025

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

note for later: this is about idempotency when creating, right? This was pretty tricky for subscriptions, might be worth looking into subscribe to see if we can take some things from there 👍

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
baermat reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread localstack-core/localstack/services/sns/v2/provider.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread localstack-core/localstack/services/sns/v2/provider.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread localstack-core/localstack/services/sns/v2/provider.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread localstack-core/localstack/services/sns/v2/provider.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread localstack-core/localstack/services/sns/v2/provider.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread localstack-core/localstack/services/sns/v2/provider.py
topics = {"Topics": [{"TopicArn": t.arn} for t in store.topics.values()]}
topics = PaginatedList(topics["Topics"])
page, nxt = topics.get_page(
lambda topic: topic["TopicArn"], next_token=next_token, page_size=100

@bentsku bentsku Sep 29, 2025

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

nit: using the topic ARN as the pagination token might be a bit of an issue (some client might not handle the arn format for that), but this can left as is for now, just a thought

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
baermat reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread tests/aws/services/sns/test_sns.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@alexrashed alexrashed modified the milestones: Playground, 4.10 Sep 30, 2025
@baermat
work in review suggestions
8c5cdf7
@baermat
baermat requested a review from bentsku October 6, 2025 09:37
@localstack-bot

localstack-bot commented Oct 6, 2025

Copy link
Copy Markdown
Contributor

Currently, only patch changes are allowed on main. Your PR labels (semver: minor, docs: skip) indicate that it cannot be merged into the main at this time.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@baermat baermat mentioned this pull request Oct 6, 2025
Sns/v2 apply subscribe changes #13226
Merged
1 task done
@baermat baermat added the notes: skip Pull request does not have to be mentioned in the release notes label Oct 6, 2025
bentsku
bentsku reviewed Oct 6, 2025
View reviewed changes

@bentsku bentsku left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Nice, this is great!

Thanks for addressing the comments, I believe there are only a few left and then we're good to merge 👍

It would be good to move the CreateTopic logic in the operation so that it's clearer where it is applied, and remove some redundant attributes of the topic.

The rest are questions, nits and thoughts about cross-region topic access that we will need to consider.

Once the CreateTopic logic has been cleaned up, we're good to merge 👍

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread localstack-core/localstack/services/sns/v2/models.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread localstack-core/localstack/services/sns/v2/models.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread tests/aws/services/sns/test_sns.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread localstack-core/localstack/services/sns/v2/provider.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread localstack-core/localstack/services/sns/v2/provider.py Outdated
"""
:param arn: the Topic ARN
:param context: the RequestContext of the request
:return: the Moto model Topic

@bentsku bentsku Oct 6, 2025

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

nit: comment: not the Moto model anymore

2nd comment: we will want to add the multi region flag sooner than later I believe, because right now we changed the behavior to raise an exception if the region of the request is different from the ARN, but this not should not be the default behavior for many operations. Just need to be kept in mind for the follow-up PRs

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
baermat reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread localstack-core/localstack/services/sns/v2/models.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@baermat
work in review suggestions
f713a3c
@baermat
baermat requested a review from bentsku October 7, 2025 08:02
bentsku
bentsku approved these changes Oct 7, 2025
View reviewed changes

@bentsku bentsku left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM! thanks for addressing the comments! this is good to go, my comments are mostly about keeping tracks of some things to fix in follow-ups 👍 this looks great, thanks a lot 🙏

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread localstack-core/localstack/services/sns/v2/provider.py
{
"ContentBasedDeduplication": "false",
"FifoTopic": "false",
"SignatureVersion": "2",

@bentsku bentsku Oct 7, 2025

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

nit: I think this is an oversight from the test, looking at all our tests, they all set the SignatureVersion to 2 when creating a fifo topic.

But looking at AWS, it doesn't seem like it sets this attribute by default:

$ aws us-east-1 sns create-topic --name test-topic.fifo --attributes FifoTopic=true
{
    "TopicArn": "arn:aws:sns:us-east-1:<account-id>:test-topic.fifo"
}
$ aws sns get-topic-attributes --topic-arn arn:aws:sns:us-east-1:<account-id>:test-topic.fifo
{
    "Attributes": {
        "Policy": "{\"Version\":\"2008-10-17\",\"Id\":\"__default_policy_ID\",\"Statement\":[{\"Sid\":\"__default_statement_ID\",\"Effect\":\"Allow\",\"Principal\":{\"AWS\":\"*\"},\"Action\":[\"SNS:GetTopicAttributes\",\"SNS:SetTopicAttributes\",\"SNS:AddPermission\",\"SNS:RemovePermission\",\"SNS:DeleteTopic\",\"SNS:Subscribe\",\"SNS:ListSubscriptionsByTopic\",\"SNS:Publish\"],\"Resource\":\"arn:aws:sns:us-east-1:<account-id>:test-topic.fifo\",\"Condition\":{\"StringEquals\":{\"AWS:SourceOwner\":\"<account-id>\"}}}]}",
        "Owner": "<account-id>",
        "SubscriptionsPending": "0",
        "TopicArn": "arn:aws:sns:us-east-1:<account-id>:test-topic.fifo",
        "EffectiveDeliveryPolicy": "{\"http\":{\"defaultHealthyRetryPolicy\":{\"minDelayTarget\":20,\"maxDelayTarget\":20,\"numRetries\":3,\"numMaxDelayRetries\":0,\"numNoDelayRetries\":0,\"numMinDelayRetries\":0,\"backoffFunction\":\"linear\"},\"disableSubscriptionOverrides\":false,\"defaultRequestPolicy\":{\"headerContentType\":\"text/plain; charset=UTF-8\"}}}",
        "SubscriptionsConfirmed": "0",
        "FifoTopic": "true",
        "DisplayName": "",
        "ContentBasedDeduplication": "false",
        "SubscriptionsDeleted": "0"
    }
}

But this is a nit, not blocking, something to consider to remove in a follow-up PR 👍 let's get this one merged! Sorry I missed it earlier

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
baermat reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread tests/aws/services/sns/test_sns.py
Comment on lines +382 to +386
# Invalid chars
for c in [":", ";", "!", "@", "|", "^", "%", " "]:
with pytest.raises(ClientError) as e:
sns_create_topic(Name=f"bad{c}name")
snapshot.match(f"name-invalid-char-{c}", e.value.response)

@bentsku bentsku Oct 7, 2025

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

question: it looks like the snapshot issue we discussed this morning is not part of the PR, will it be part of a follow-up? Not worries, not blocking either, I was wondering 😄

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@baermat baermat Oct 7, 2025

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

works against v2 🤷

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread tests/aws/services/sns/test_sns.py
Comment on lines +389 to +393
@markers.snapshot.skip_snapshot_verify(
paths=[
"$..Attributes.EffectiveDeliveryPolicy",
]
)

@bentsku bentsku Oct 7, 2025

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

something I missed earlier too: it would be great to implement this default EffectiveDeliveryPolicy in a follow-up PR if we could keep track of it, as it is used in Publish to determine which content type to use for HTTP subscriptions. Not to be implemented in this PR either, this is just to keep track of it and remove in a follow-up for v2 👍

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
baermat reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@baermat
baermat merged commit f239baf into main Oct 7, 2025
@baermat
baermat deleted the sns/v2-topic-migration branch October 7, 2025 14:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
@bentsku bentsku bentsku approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

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

4.10

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@baermat @localstack-bot @bentsku @alexrashed
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.