-
Notifications
You must be signed in to change notification settings - Fork 403
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.
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
feat(event_source): Add support for S3 batch operations #3572
feat(event_source): Add support for S3 batch operations #3572
Conversation
…t tests. This seamlessly support both schema 1.0 and 2.0 A few notes: - S3BatchOperationXXX or S3BatchOperationsXXX ? - s3 key is not url-encoded in real life despite what the documentation implies. Need to test with some keys that contain spaces, etc... - S3BatchOperationResult has some factory methods to simplifies building - S3BatchOperationEvent may need to as it makes initialization needlessly complicated
Thanks a lot for your first contribution! Please check out our contributing guidelines and don't hesitate to ask whatever you need. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## develop #3572 +/- ##
===========================================
+ Coverage 95.51% 95.54% +0.02%
===========================================
Files 211 213 +2
Lines 9860 9961 +101
Branches 1802 850 -952
===========================================
+ Hits 9418 9517 +99
- Misses 329 330 +1
- Partials 113 114 +1 ☔ View full report in Codecov by Sentry. |
Hey @sbailliez! You were quick to send this PR and did an excellent job. I would like to highlight that the level of detail in the PR description is impressive, you put an effort into it!!. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @sbailliez! I did an initial review and we have some small things to change!
Please let me know what you think about this.
aws_lambda_powertools/utilities/data_classes/s3_batch_operation_event.py
Outdated
Show resolved
Hide resolved
aws_lambda_powertools/utilities/data_classes/s3_batch_operation_event.py
Outdated
Show resolved
Hide resolved
aws_lambda_powertools/utilities/data_classes/s3_batch_operation_event.py
Outdated
Show resolved
Hide resolved
Hey @sbailliez! Please let me know if you need any help here! |
I'm working on this PR to release this support in our next release - 19/01/2024 |
@rubenfonseca pls review this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Just left some style comments inline.
aws_lambda_powertools/utilities/data_classes/s3_batch_operation_event.py
Outdated
Show resolved
Hide resolved
aws_lambda_powertools/utilities/data_classes/s3_batch_operation_event.py
Outdated
Show resolved
Hide resolved
aws_lambda_powertools/utilities/data_classes/s3_batch_operation_event.py
Outdated
Show resolved
Hide resolved
aws_lambda_powertools/utilities/data_classes/s3_batch_operation_event.py
Outdated
Show resolved
Hide resolved
aws_lambda_powertools/utilities/data_classes/s3_batch_operation_event.py
Outdated
Show resolved
Hide resolved
aws_lambda_powertools/utilities/data_classes/s3_batch_operation_event.py
Outdated
Show resolved
Hide resolved
aws_lambda_powertools/utilities/data_classes/s3_batch_operation_event.py
Outdated
Show resolved
Hide resolved
aws_lambda_powertools/utilities/data_classes/s3_batch_operation_event.py
Outdated
Show resolved
Hide resolved
aws_lambda_powertools/utilities/data_classes/s3_batch_operation_event.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Waiting until all conversations are resolved before the next review
Done |
Quality Gate passedThe SonarCloud Quality Gate passed, but some issues were introduced. 2 New issues |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
APPROVED!
Awesome work, congrats on your first merged pull request and thank you for helping improve everyone's experience! |
@leandrodamascena Thanks for the review and the fixes. Sorry for the silence but was OOO and away from computer. |
Hello. Maybe this is a silly question - but why is there a check that there can be only 1 result? |
Hey @Dilski! Because this is a relation 1x1 with invoke and response. According to the official documentation S3 invokes the Lambda for each object, and not for a batch of objects.
Do you see a different behavior or something we're missing that needs to be fixed? I'd love to hear it. PS: Please next time open a discussion or issue and indicate the specific PR/comment you want to discuss. Commenting on closed issues/PRs is sometimes challenging to respond to because we don't have much visibility. |
Issue number: #3563
Summary
Changes
This feature adds data class support for S3 Batch Operations. It supports events that are using either invocation schema 1.0 or 2.0 and provides as well as the response structure that needs to be returned by the lambda.
Notable details:
Convenient way to get the first task via
event.task
in addition toevent.tasks[0]
Convenient way to get the s3 key via
event.task.s3_bucket
which works for both 1.0 and 2.0 and avoids version specific code usings3BucketArn
(1.0) ors3Bucket
(2.0).Factory methods
as_succeeded
,as_temporary_failure
andas_permanent_failure
to build result. For exampleS3BatchOperationResult.as_temporary_failure(task, 'failure message')
is simpler thanS3BatchOperationResult(task.task_id, 'TemporaryFailure', 'failure message')
The s3 key decoding is using
unquote_plus
while AWS documentation example is usingunquote
. Testing on S3 Batch Operations supports the usage ofunquote_plus
. (see comments in code and unit test)A couple of things where I'm asking for a second opinion:
Should the name use
S3BatchOperationXXX
orS3BatchOperationsXXX
- The AWS service is namedBatch Operations
, it felt more natural to use singular but I'm going back and forth.Add a factory method to
S3BatchOperationsResponse.from_event(event)
similar to the factory methods for the results. There is a strong dependency with the original event, so it feels better to use this from a syntax perspective compare to the current verbose way.User experience
The data class documentation has been updated with a converted sample provided by AWS in the S3 Batch Operations documentation. A simplified example like the one below gives a good idea of the changes:
before:
After:
Update 16/01
New DX with some small changes.
Checklist
If your change doesn't seem to apply, please leave them unchecked.
Is this a breaking change?
RFC issue number:
Checklist:
Acknowledgment
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.