-
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(jmespath): new built-in envelopes to unwrap S3 events #2169
feat(jmespath): new built-in envelopes to unwrap S3 events #2169
Conversation
Co-authored-by: Ruben Fonseca <fonseka@gmail.com> Signed-off-by: Leandro Damascena <leandro.damascena@gmail.com>
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.
Looks great!!! THANK YOU for that @leandrodamascena ;)
One necessary change, and two suggestions to make this better.
docs/utilities/jmespath_functions.md
Outdated
| **`SNS`** | `Records[0].Sns.Message | powertools_json(@)` | | ||
| **`SQS`** | `Records[*].powertools_json(body)` | | ||
|
||
???+ tip |
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.
Niiiice!! You can make it slightly better by:
- Adding a title so ESL readers know upfront if they should care or not e.g., "Using SNS?"
- Educate the reader that if they do need SNS event metadata they don't need to enable it
Write, review, cut the unnecessary words without losing its original intent, and ship it <3
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.
- Added the title
- I thought of the following mental model to improve this text, and see if it makes sense.
- Benefits - Reduce payload size
- How to do - enabling raw message
- why? - architectures that require sending event notifications from S3 to SNS but not require SNS metadata
Consider rewriting the PR title so anyone reading later know what this provides; play around with words |
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@
## develop #2169 +/- ##
========================================
Coverage 97.46% 97.46%
========================================
Files 147 147
Lines 6872 6872
Branches 505 505
========================================
Hits 6698 6698
Misses 137 137
Partials 37 37 ☔ View full report in Codecov by Sentry. |
Thank you, @heitorlessa! Can you check again pls? |
Signed-off-by: Heitor Lessa <lessa@amazon.nl>
Issue number: #2168
Summary
Changes
In this pull request, 5 new envelopes were created to make the developer's experience simpler.
User experience
BEFORE
AFTER
Payloads to test
S3->SNS->SQS
S3->SNS(raw delivery message enabled)->SQS
S3->SNS->Kinesis Firehose
S3->SNS(raw delivery message enabled)->Kinesis Firehose
S3->EventBridge->SQS
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.