Skip to content

Sns/v2 switch default provider#13699

Merged
baermat merged 10 commits intomainfrom
sns/v2-switch-default-provider
Feb 5, 2026
Merged

Sns/v2 switch default provider#13699
baermat merged 10 commits intomainfrom
sns/v2-switch-default-provider

Conversation

@baermat
Copy link
Member

@baermat baermat commented Feb 4, 2026 •
edited
Loading

Motivation

This PR finally puts the new SNS v2 provider as default! 🎉
After a discussion with @bentsku , I opted to not go with the squash and rebase approach, but with a regular squash and merge. The history seems to be at least somewhat useful in this case, because the old version was already partly internalized.
This PR supersedes #13686
closes PNX-520

Changes

  • move v2 files to default location
  • remove the old provider
  • remove all skips for tests for parity gaps in the old provider
  • enable previously skipped tests for the new provider
  • remove the extra sns v2 step from the pipeline

Tests

Related

The current MA/MR pipeline failures are unrelated, so are the s3control failures.

@baermat baermat mentioned this pull request Feb 4, 2026
Closed
@github-actions
Copy link

github-actions bot commented Feb 4, 2026 •
edited
Loading

Test Results - Preflight, Unit

23 114 tests  ±0   21 255 ✅ ±0   6m 29s ⏱️ +23s
     1 suites ±0    1 859 💤 ±0 
     1 files   ±0        0 ❌ ±0 

Results for commit c2656fe. ± Comparison against base commit 7c60109.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Feb 4, 2026 •
edited
Loading

Test Results (MA/MR) - Preflight, Unit

23 114 tests   21 255 ✅  6m 4s ⏱️
     1 suites   1 859 💤
     1 files         0 ❌

Results for commit c2656fe.

♻️ This comment has been updated with latest results.

@baermat baermat force-pushed the sns/v2-switch-default-provider branch from 707e440 to dd276ff Compare February 4, 2026 12:03
@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 docs: needed Pull request requires documentation updates and removed docs: skip Pull request does not require documentation changes labels Feb 4, 2026
@baermat baermat force-pushed the sns/v2-switch-default-provider branch from dd276ff to 0f5f5e1 Compare February 4, 2026 12:51
@github-actions
Copy link

github-actions bot commented Feb 4, 2026 •
edited
Loading

Test Results (amd64, MA/MR) - Acceptance

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

Results for commit c2656fe.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Feb 4, 2026 •
edited
Loading

Test Results (amd64) - Acceptance

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

Results for commit c2656fe. ± Comparison against base commit 7c60109.

♻️ This comment has been updated with latest results.

@baermat baermat self-assigned this Feb 4, 2026
@github-actions
Copy link

github-actions bot commented Feb 4, 2026 •
edited
Loading

Test Results (amd64, MA/MR) - Integration, Bootstrap

    5 files      5 suites   2h 38m 42s ⏱️
5 611 tests 5 089 ✅ 521 💤 1 ❌
5 617 runs  5 089 ✅ 527 💤 1 ❌

For more details on these failures, see this check.

Results for commit c2656fe.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Feb 4, 2026 •
edited
Loading

Test Results (amd64) - Integration, Bootstrap

    5 files      5 suites   2h 40m 0s ⏱️
5 611 tests 5 090 ✅ 521 💤 0 ❌
5 617 runs  5 090 ✅ 527 💤 0 ❌

Results for commit c2656fe.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Feb 4, 2026 •
edited
Loading

LocalStack Community integration with Pro

    2 files  ±0      2 suites  ±0   2h 0m 6s ⏱️ + 1m 19s
5 190 tests +1  4 837 ✅ +39  351 💤  - 40  2 ❌ +2 
5 192 runs  +1  4 837 ✅ +39  353 💤  - 40  2 ❌ +2 

For more details on these failures, see this check.

Results for commit c2656fe. ± Comparison against base commit 7c60109.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Feb 4, 2026 •
edited
Loading

Test Results - Alternative Providers

585 tests   322 ✅  17m 43s ⏱️
  1 suites  263 💤
  1 files      0 ❌

Results for commit c2656fe.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Feb 4, 2026 •
edited
Loading

Test Results (MA/MR) - Alternative Providers

585 tests   322 ✅  18m 12s ⏱️
  1 suites  263 💤
  1 files      0 ❌

Results for commit c2656fe.

♻️ This comment has been updated with latest results.

@baermat baermat marked this pull request as ready for review February 4, 2026 16:22
bentsku
bentsku approved these changes Feb 4, 2026
View reviewed changes
Copy link
Contributor

@bentsku bentsku left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM! Nice cleanup! Awesome to see all of this work being promoted to be the default! 🚀

Seems like everything has been done to clean up the internal implementation, tests look good, and the CI job is removed!

Only one nit about importing a private function in the CFN code, not blocking, but would be nice to be addressed.

Awesome! 👌

Note: MA/MR failure seems unrelated, and is a CFN change

localstack-core/localstack/services/sns/resource_providers/aws_sns_topicpolicy.py Outdated
ResourceRequest,
)
from localstack.services.sns.models import create_default_sns_topic_policy
from localstack.services.sns.provider import _create_default_topic_policy
Copy link
Contributor

@bentsku bentsku Feb 4, 2026

Choose a reason for hiding this comment

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

nit: this is probably a sign that _create_default_topic_policy should be in the utils, and that it should be a private function

localstack-core/localstack/services/sns/provider.py
Copy link
Contributor

@bentsku bentsku Feb 4, 2026

Choose a reason for hiding this comment

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

as we discussed, the history is a bit broken because some of the function definitions have changed order compared to before and git cannot make sense of it. But overall, we're still keeping most of the old history, and this is better than fully erasing it by doing a deletion + move! Nicely done 👌

@baermat baermat merged commit 9f9e8ea into main Feb 5, 2026
62 of 66 checks passed
@baermat baermat deleted the sns/v2-switch-default-provider branch February 5, 2026 10:06
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

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

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

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

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

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

@silv-io silv-io Awaiting requested review from silv-io silv-io is a code owner

@k-a-il k-a-il Awaiting requested review from k-a-il k-a-il is a code owner

Assignees

@baermat baermat

Labels

docs: needed Pull request requires documentation updates 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.

2 participants

@baermat @bentsku