migrate S3 notifications to ASF #6903
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
21 tasks
steffyP
force-pushed
the
s3-notifications
branch
from
7318440
to
dadd87d
Compare
steffyP
force-pushed
the
s3-notifications
branch
from
dadd87d
to
35e5435
Compare
steffyP
force-pushed
the
s3-notifications
branch
from
35e5435
to
f2f03dd
Compare
steffyP
force-pushed
the
s3-notifications
branch
from
f2f03dd
to
f04572f
Compare
steffyP
force-pushed
the
s3-notifications
branch
from
f04572f
to
627a713
Compare
steffyP
commented
Sep 23, 2022
localstack/services/s3/provider.py
Outdated
Comment on lines
105
to
109
| class InvalidArgumentError(CommonServiceException): | ||
| def __init__(self, message: str, name: str, value: str): | ||
| super().__init__("InvalidArgument", message, 400, False) | ||
| # TODO how to set values? | ||
|
|
thrau
requested changes
Sep 24, 2022
There was a problem hiding this comment.
great progress on the provider!
i have two major points that i think we should address, either in this PR or as part of an immediate refactoring:
- processing notifications asynchronously
- i think the event notification code would be easier to understand and more maintainable if we used OOP concepts and introduced some basic abstractions. in particular to process event notifications asynchronously it will help to encapsulate the event context into an object, and homogenize the send_event api. i made some suggestions inline.
i think 1. is quite essential since that seems to be how AWS behaves. at least in the lambda documentation it says that Amazon S3 invokes your function asynchronously.
in terms of code organization i think we should move all the code related to notifications into a separate module notifications.py to keep the provider.py a little slimmer.
@bentsku let's discuss who will do the refactoring.
bentsku
force-pushed
the
s3-notifications
branch
from
627a713
to
720e078
Compare
bentsku
force-pushed
the
s3-notifications
branch
from
ed6c37d
to
4b60a7f
Compare
thrau
force-pushed
the
s3-notifications
branch
from
4b60a7f
to
17daf4c
Compare
- store notification config in s3-store - notifications for eventbridge, sqs, sns, lambda - add snapshot to tests - implement filter validation - add verification for notification destination
thrau
force-pushed
the
s3-notifications
branch
from
17daf4c
to
ad964dd
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR contains S3 bucket notifications for:
Implemented:
Following events are covered (also by existing tests):
Other implementation:
Known Issues + Future work:
InvalidArgumentError: marked a few TODOs here, because in another PR we already have a batch for that missing Exceptiontest_create_object_by_presigned_request_via_dynamodbis failing)