Skip to content
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(nested-event-sources): Generic unwrapping of event source data #4069

Open
wants to merge 43 commits into
base: v2
Choose a base branch
from

Conversation

seshubaws
Copy link
Contributor

@seshubaws seshubaws commented Apr 3, 2024

Issue number:
#2678

Summary

Changes

Please provide a summary of what's being changed

User experience

Please share what the user experience looks like before and after this change

Checklist

If your change doesn't seem to apply, please leave them unchecked.

Is this a breaking change?

RFC issue number:
#2678

Checklist:

  • Migration process documented
  • Implement warnings (if it can live side by side)

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.

@pull-request-size pull-request-size bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Apr 3, 2024
@seshubaws seshubaws self-assigned this Apr 4, 2024
@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 4, 2024
@github-actions github-actions bot added the feature New feature or functionality label Apr 4, 2024
@pull-request-size pull-request-size bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 9, 2024
@seshubaws
Copy link
Contributor Author

seshubaws commented Apr 26, 2024

@rafaelgsr @leandrodamascena Please take a look at this PR and let me know if I need to change anything. I will work on fixing the linting soon. I had a couple general questions as well:

  1. Should we take every instance of a class implementing the DictWrapper class and change it to the EventWrapper class (since EventWrapper now implements DictWrapper and EventWrapper has the parent class implementation of how to unwrap nested events).
  2. Since events using the Firehose Event class have the option to encode the data, should we add a flag to auto decode the data on unwrapping? Otherwise, it would return encoded data and that data would not be able to unwrap to the next level like you see in the firehose_sns_event in nested_test_events.py (https://github.com/aws-powertools/powertools-lambda-python/pull/4069/files#diff-c01fab973bea227eb17986da25f24715386579f71e68297cd520929431bc0778R5-R14).

@seshubaws
Copy link
Contributor Author

seshubaws commented May 2, 2024

Discussed the above questions:

  1. Should we take every instance of a class implementing the DictWrapper class and change it to the EventWrapper class (since EventWrapper now implements DictWrapper and EventWrapper has the parent class implementation of how to unwrap nested events).

Probably ok to do this, but @leandrodamascena will confirm.

  1. Since events using the Firehose Event class have the option to encode the data, should we add a flag to auto decode the data on unwrapping? Otherwise, it would return encoded data and that data would not be able to unwrap to the next level like you see in the firehose_sns_event in nested_test_events.py (https://github.com/aws-powertools/powertools-lambda-python/pull/4069/files#diff-c01fab973bea227eb17986da25f24715386579f71e68297cd520929431bc0778R5-R14).

Decided we should just decode without needing a flag since customers will always need to decode their data. Still discussing how to decode for different data classes.

Copy link

Quality Gate Passed Quality Gate passed

Issues
4 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
1.3% Duplication on New Code

See analysis details on SonarCloud

@heitorlessa
Copy link
Contributor

@leandrodamascena think this is something @sthulb could review and take it to the finish line?

@leandrodamascena
Copy link
Contributor

@leandrodamascena think this is something @sthulb could review and take it to the finish line?

Hello @heitorlessa! I completely missed that comment. @seshubaws has done a great job so far and we can help finalize this PR and launch this new feature.

I'll assing to Simon after we finish some internal things related to V3.

@dreamorosi dreamorosi added the on-hold This item is on-hold and will be revisited in the future label Aug 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or functionality on-hold This item is on-hold and will be revisited in the future size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants