-
Notifications
You must be signed in to change notification settings - Fork 409
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(parser): Add IoT registry events models #5892
base: develop
Are you sure you want to change the base?
feat(parser): Add IoT registry events models #5892
Conversation
Thanks a lot for your first contribution! Please check out our contributing guidelines and don't hesitate to ask whatever you need. |
IoTCRUDEvent
modelIoTCRUDEvent
model
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #5892 +/- ##
===========================================
+ Coverage 96.25% 96.27% +0.02%
===========================================
Files 234 235 +1
Lines 11140 11204 +64
Branches 822 822
===========================================
+ Hits 10723 10787 +64
Misses 327 327
Partials 90 90 ☔ View full report in Codecov by Sentry. |
Hey @basvandriel, we need to address this comment before reviewing this PR: #5891 (comment). Thanks |
Hi @leandrodamascena, I updated my branch to include all registry events, documented on this page. Please let me know if you would like to see additional changes, unit tests or any documentation. We could also add aliases such as |
IoTCRUDEvent
modelThere 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 @basvandriel! Thank you so much for working on this PR and adding all the events! I have some feedback to make it more consistent and establish a naming standard.
Also, we need to include tests to verify that this is serializing as expected, you can copy this AWS events to test. In the documentation section https://docs.powertools.aws.dev/lambda/python/latest/utilities/parser/#built-in-models we need to add the new Models.
I also have a suggestion for the variable names, but I'd rather you correct the feedback I gave now and then we'll go to round two, okay?
aws_lambda_powertools/utilities/parser/models/iot_registry_events.py
Outdated
Show resolved
Hide resolved
aws_lambda_powertools/utilities/parser/models/iot_registry_events.py
Outdated
Show resolved
Hide resolved
aws_lambda_powertools/utilities/parser/models/iot_registry_events.py
Outdated
Show resolved
Hide resolved
aws_lambda_powertools/utilities/parser/models/iot_registry_events.py
Outdated
Show resolved
Hide resolved
aws_lambda_powertools/utilities/parser/models/iot_registry_events.py
Outdated
Show resolved
Hide resolved
aws_lambda_powertools/utilities/parser/models/iot_registry_events.py
Outdated
Show resolved
Hide resolved
aws_lambda_powertools/utilities/parser/models/iot_registry_events.py
Outdated
Show resolved
Hide resolved
aws_lambda_powertools/utilities/parser/models/iot_registry_events.py
Outdated
Show resolved
Hide resolved
aws_lambda_powertools/utilities/parser/models/iot_registry_events.py
Outdated
Show resolved
Hide resolved
aws_lambda_powertools/utilities/parser/models/iot_registry_events.py
Outdated
Show resolved
Hide resolved
Hey, please move on with this! |
…nts.py Co-authored-by: Leandro Damascena <lcdama@amazon.pt> Signed-off-by: Bas <contact@basvandriel.nl>
…nts.py Co-authored-by: Leandro Damascena <lcdama@amazon.pt> Signed-off-by: Bas <contact@basvandriel.nl>
|
…nts.py Co-authored-by: Leandro Damascena <lcdama@amazon.pt> Signed-off-by: Bas <contact@basvandriel.nl>
…nts.py Co-authored-by: Leandro Damascena <lcdama@amazon.pt> Signed-off-by: Bas <contact@basvandriel.nl>
…nts.py Co-authored-by: Leandro Damascena <lcdama@amazon.pt> Signed-off-by: Bas <contact@basvandriel.nl>
…nts.py Co-authored-by: Leandro Damascena <lcdama@amazon.pt> Signed-off-by: Bas <contact@basvandriel.nl>
…nts.py Co-authored-by: Leandro Damascena <lcdama@amazon.pt> Signed-off-by: Bas <contact@basvandriel.nl>
|
Hi @basvandriel! Please let me know when you need me to review it again, okay? |
Hi @leandrodamascena, I made a misclick. I'll let you know! |
Hi @leandrodamascena, I've added the serialization tests and the documentation. I've tried to use the same format as the previous rows, e.g. |
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 @basvandriel! Thanks for working on this PR and resolving all the comments! This is really great work and contribution!
I just pushed a commit with a few changes:
1 - Added docstrings to all classes
2 - Moved JSON events to the events folder because people often come to our repository to copy these events as a source of truth.
3 - I have a feeling that rootToParentThingGroups
is a list with arbitrary fields and not onlyarnid
. So I converted it to a list of Dicts to have a guardrail.
4 - I added the even_type field with literal values to the corresponding classes.
I'm approving this PR and we'll merge it next week. We will release it in the next release of Powertools, which is expected on 2/25.
|
Thanks @leandrodamascena. Lets stay in touch - I'd love to work on the project some more. |
That's great to know! How about you add these events to our Event Source Data Class utility? The main difference is that in this utility we don't use Pydantic to do strict validation, but instead we convert the event to an object to help developers access properties like a python object. Here are some examples: https://github.com/aws-powertools/powertools-lambda-python/tree/develop/aws_lambda_powertools/utilities/data_classes. Please let me know what you think and if you agree, feel free to open an issue and send a PR. |
Issue number: #5891
Summary
This implements the missing model for IoT registry events.
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.