Conversation
21 tasks
7318440 to
dadd87d
Compare
dadd87d to
35e5435
Compare
35e5435 to
f2f03dd
Compare
f2f03dd to
f04572f
Compare
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? | ||
|
|
Member
Author
thrau
requested changes
Sep 24, 2022
Member
thrau
left a comment
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.
627a713 to
720e078
Compare
ed6c37d to
4b60a7f
Compare
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
17daf4c to
ad964dd
Compare
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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)