Skip to content

chore: define explicit state containers for providers #13423

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
bentsku merged 16 commits into from
Dec 1, 2025
Merged

chore: define explicit state containers for providers #13423

bentsku merged 16 commits into from
Dec 1, 2025

Conversation

@bentsku
Copy link
Contributor

@bentsku bentsku commented Nov 26, 2025 •
edited
Loading

Motivation

This PR reworks how we define the dependencies between the providers and their state containers. Previously, we've relied on the ReflectionStateLocator to automatically discover where the state for a service would be saved.

This had several drawbacks: lots of magic and indirection, the need to follow some conventions that were sometimes tricky (the service name, the plural form of the _stores prefix, etc). It has been extremely useful to migrate the whole codebase to the new store system a few years back, but we would like today to be more explicit about it.
It also had the drawback to automatically attach the Moto state backends to a service if Moto implemented a service that LocalStack would implement, even if LocalStack had no dependency on that particular Moto service.

This will also help us to visualize the dependencies between the providers and where they store their state.

Changes

Migrate the following services to be explicit about their state containers:

Service Store Backend
acm - X
apigateway X X
cloudwatch legacy - X
cloudwatch default X -
config - X
dynamodb (v1 and v2) X -
dynamodbstreams v1 X -
dynamodbstreams v2 - -
ec2 - X
es - -
events v1 X X
events v2 X -
firehose X -
iam - X
kms X -
logs X X
resourcegroups - X
resourcegroupstaggingapi - -

Tests

@bentsku bentsku added this to the 4.12 milestone Nov 26, 2025
@bentsku bentsku self-assigned this Nov 26, 2025
@bentsku bentsku added 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 Nov 26, 2025
@github-actions
Copy link

github-actions bot commented Nov 26, 2025 •
edited
Loading

Test Results - Preflight, Unit

22 669 tests  ±0   20 901 ✅ ±0   6m 13s ⏱️ -13s
     1 suites ±0    1 768 💤 ±0 
     1 files   ±0        0 ❌ ±0 

Results for commit 80fb19b. ± Comparison against base commit 42ed9a1.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Nov 26, 2025 •
edited
Loading

Test Results (amd64) - Acceptance

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

Results for commit 80fb19b. ± Comparison against base commit 42ed9a1.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Nov 26, 2025 •
edited
Loading

Test Results - Alternative Providers

678 tests   399 ✅  12m 47s ⏱️
  3 suites  279 💤
  3 files      0 ❌

Results for commit 80fb19b.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Nov 26, 2025 •
edited
Loading

Test Results (amd64) - Integration, Bootstrap

    5 files      5 suites   1h 41m 10s ⏱️
4 217 tests 3 981 ✅ 236 💤 0 ❌
4 223 runs  3 981 ✅ 242 💤 0 ❌

Results for commit 80fb19b.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Nov 26, 2025 •
edited
Loading

LocalStack Community integration with Pro

    2 files  ±  0      2 suites  ±0   1h 12m 57s ⏱️ - 52m 6s
4 193 tests  - 909  3 954 ✅  - 757  239 💤  - 152  0 ❌ ±0 
4 195 runs   - 909  3 954 ✅  - 757  241 💤  - 152  0 ❌ ±0 

Results for commit 80fb19b. ± Comparison against base commit 42ed9a1.

This pull request removes 909 tests. 
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]
…

♻️ This comment has been updated with latest results.

@bentsku bentsku mentioned this pull request Nov 27, 2025
Merged
@bentsku bentsku marked this pull request as ready for review December 1, 2025 11:06
aidehn
aidehn reviewed Dec 1, 2025
View reviewed changes
localstack-core/localstack/services/resourcegroupstaggingapi/provider.py

class ResourcegroupstaggingapiProvider(ResourcegroupstaggingapiApi, ABC):
pass
def accept_state_visitor(self, visitor: StateVisitor):
Copy link
Contributor

@aidehn aidehn Dec 1, 2025

Choose a reason for hiding this comment

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

Agree with this comment, I believe I will need to update this for the pro version of this provider in the future. Just had a few questions about how to implement this as I'm not 100% in the loop

  • How would this work for services which have a only-pro implementation, is it just the case of defining this on the Pro provider?
  • If a service has both a pro and community provider should we opt to define accept_state_visitor on the community provider or the pro provider? (Does it even matter?)
  • In the future should everyone be making sure to implement the accept_state_visitor on any new services they implement?

Copy link
Contributor Author

@bentsku bentsku Dec 1, 2025

Choose a reason for hiding this comment

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

Yes, I've done it in a Pro PR that is still in the draft, I'll send it over to you!

For the questions:

  • yes, defining it only in the Pro implementation is enough
  • for both Pro and Community, as the Pro is a subclass of the Community, defining it only in the Community should be enough, except if the dependency is different (let's say we only depend on Moto in Community but add a Store in the Pro version, or we use the Assets). Technically, as this relates to persistence, defining it in Pro only should be enough, but at least we make it clear here 👍
  • very good question! yes, we should! we can have mechanism in place to ensure this going forward, like the persistence tests, if we remove the reflection locator 👍

Copy link
Contributor

@aidehn aidehn Dec 1, 2025

Choose a reason for hiding this comment

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

perfect, thank you for clarifying!

giograno
giograno approved these changes Dec 1, 2025
View reviewed changes
Copy link
Member

@giograno giograno left a comment

Choose a reason for hiding this comment

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

Wow, thanks for churning over all these services 💯 It looks all good to me!

@bentsku bentsku merged commit ff6e5e0 into main Dec 1, 2025
48 checks passed
@bentsku bentsku deleted the explicit-visitor branch December 1, 2025 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aidehn aidehn aidehn left review comments

@giograno giograno giograno approved these changes

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

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

@sannya-singal sannya-singal Awaiting requested review from sannya-singal sannya-singal is a code owner

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

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

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

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

@viren-nadkarni viren-nadkarni Awaiting requested review from viren-nadkarni viren-nadkarni is a code owner

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

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

Assignees

@giograno giograno

@bentsku bentsku

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: patch Non-breaking changes which can be included in patch releases

Projects

None yet

Milestone

4.12

Development

Successfully merging this pull request may close these issues.

4 participants

@bentsku @giograno @aidehn