8000 add service_name attribute to AssetDirectory by thrau · Pull Request #9125 · localstack/localstack · GitHub
[go: up one dir, main page]
Skip to content
8000

add service_name attribute to AssetDirectory #9125

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
merged 1 commit into from
Sep 12, 2023
Merged

add service_name attribute to AssetDirectory #9125

merged 1 commit into from
Sep 12, 2023

Conversation

thrau
Copy link
Member
@thrau thrau commented Sep 12, 2023

Motivation

In the StateVisitor implementations that handle cloud pod state, there is currently no way to know which service the AssetDirectory being visited belongs to, unlike AccountRegionBundle and BackendDict which both have a service_name attribute.
Knowing the service name for an AssetDirectory is required for technical reasons for s3 v3 persistence.

Changes

  • Make StateContainer a protocol and make service_name a mandatory attribute
  • Refactor all instances of AssetDirectory to correctly use the service name

@thrau thrau added the semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases label Sep 12, 2023
@coveralls
Copy link

Coverage Status

coverage: 79.747% (-0.2%) from 79.939% when pulling d2bf2d8 on asset-directory-service-name into 982da5d on master.

@github-actions
Copy link

LocalStack Community integration with Pro

       2 files         2 suites   1h 15m 23s ⏱️
2 194 tests 1 705 ✔️ 489 💤 0 ❌
2 195 runs  1 705 ✔️ 490 💤 0 ❌

Results for commit d2bf2d8.

bentsku
bentsku approved these changes Sep 12, 2023
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! Awesome, thank you so much for adding this. I'm sure it will allow us to have more flexibility regarding persistence, especially cloud pods when not having persistence enabled!

@thrau thrau merged commit b05a613 into master Sep 12, 2023
@thrau thrau deleted the asset-directory-service-name branch September 12, 2023 20:55
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

@dominikschubert dominikschubert Awaiting requested review from dominikschubert

@MEPalma MEPalma Awaiting requested review from MEPalma

@macnev2013 macnev2013 Awaiting requested review from macnev2013

@alexrashed alexrashed Awaiting requested review from alexrashed

@silv-io silv-io Awaiting requested review from silv-io

@viren-nadkarni viren-nadkarni Awaiting requested review from viren-nadkarni

@giograno giograno Awaiting requested review from giograno

Assignees
No one assigned
Labels
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
@thrau @coveralls @bentsku
0