Refactor: Events v2: Move existing provider code to v1 folder #10730
Conversation
maxhoheiser
added
aws:events
maxhoheiser
force-pushed
the
refactor/events_v2-move-existing-to-v1-folder
branch
2 times, most recently
from
c5b0427
to
c5448c9
Compare
maxhoheiser
requested review from
dominikschubert,
MEPalma and
dfangl
as code owners
| from localstack.services.stores import AccountRegionBundle, BaseStore, LocalAttribute | ||
|
|
||
|
|
||
| class EventsStore(BaseStore): |
There was a problem hiding this comment.
💡 Please check whether persistence still works before merging after moving the model to a new location.
There was a problem hiding this comment.
This should be done by the persistency test in the companion branch in pro: https://github.com/localstack/localstack-ext/pull/2907
Since all the tests are green including the persistency tests, it should still work after moving.
maxhoheiser
force-pushed
the
refactor/events_v2-move-existing-to-v1-folder
branch
5 times, most recently
from
b958125
to
0a4374e
Compare
maxhoheiser
force-pushed
the
refactor/events_v2-move-existing-to-v1-folder
branch
2 times, most recently
from
6d5b7f4
to
0ed692e
Compare
localstack/services/events/rule.py
Outdated
There was a problem hiding this comment.
As discussed the v1 folder should contain only code related to the legacy provider and nothing from the new provider should import from the v1 provider.
The v1 code is allowed to import from the v2 implementation.
This strict separation in one direction allows for potential easy deletion of the v1 provider in the future.
maxhoheiser
force-pushed
the
refactor/events_v2-move-existing-to-v1-folder
branch
from
0ed692e
to
0497c5a
Compare
There was a problem hiding this comment.
LGTM 👍 This will help a lot phasing out the old provider in the future and keep them separated 🚀
It might be worth checking out what's wrong with the test result publishing given that the artifact download failed repeatedly. Latest failure here: https://github.com/localstack/localstack/actions/runs/8999955477/job/24725298339?pr=10730
| from localstack.services.events.packages import event_ruler_package | ||
| from localstack.services.events.utils import InvalidEventPatternException | ||
| from localstack.services.events.v1.packages import event_ruler_package | ||
| from localstack.services.events.v1.utils import InvalidEventPatternException |
There was a problem hiding this comment.
I suggest avoiding any imports into the legacy v1 package from the main codebase.
Motivation
To improve the readability of events service folder by separating v1 and v2 providers not via file suffixes but dedicated folders.
There are also dependencies in ext that rely on v1 and v2 provider files, these are updated in a companion PR in ext: https://github.com/localstack/localstack-ext/pull/2907
Changes
Move existing (v1) provider and related files to v1 folder.
Remove suffix for v2 provider and related files.