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(parser): add models for API GW Websockets events #5597

Merged
merged 15 commits into from
Nov 24, 2024

Conversation

ran-isenberg
Copy link
Contributor

@ran-isenberg ran-isenberg commented Nov 20, 2024

Issue number: #5543

Summary

Added support for parser and websockers api

Changes

  • Parser envelope for the message type.
  • models for message, connect and disconnect
  • unit tests
  • 3 input files
  • documentation

Used this doc and my own files.
https://docs.aws.amazon.com/apigateway/latest/developerguide/apigateway-websocket-api-integration-requests.html

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:

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.

@ran-isenberg ran-isenberg requested a review from a team as a code owner November 20, 2024 07:24
@boring-cyborg boring-cyborg bot added documentation Improvements or additions to documentation tests labels Nov 20, 2024
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 20, 2024
@ran-isenberg ran-isenberg changed the title feature(parser): Parser models for API GW Websockets Events feat(parser): Parser models for API GW Websockets Events Nov 20, 2024
Copy link
Contributor

No related issues found. Please ensure there is an open issue related to this change to avoid significant delays or closure.

@github-actions github-actions bot added do-not-merge need-issue PRs that are missing related issues feature New feature or functionality and removed documentation Improvements or additions to documentation labels Nov 20, 2024
@leandrodamascena
Copy link
Contributor

Hi @ran-isenberg! Thanks for submitting this PR!

Assigning @anafalcao to review this.

@leandrodamascena leandrodamascena linked an issue Nov 20, 2024 that may be closed by this pull request
2 tasks
Copy link

codecov bot commented Nov 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.17%. Comparing base (3726a4b) to head (a149ac1).
Report is 2 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #5597      +/-   ##
===========================================
+ Coverage    96.14%   96.17%   +0.02%     
===========================================
  Files          229      231       +2     
  Lines        10854    10915      +61     
  Branches      2018     2018              
===========================================
+ Hits         10436    10497      +61     
  Misses         329      329              
  Partials        89       89              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@boring-cyborg boring-cyborg bot added the documentation Improvements or additions to documentation label Nov 20, 2024
@github-actions github-actions bot removed the documentation Improvements or additions to documentation label Nov 20, 2024
@boring-cyborg boring-cyborg bot added the documentation Improvements or additions to documentation label Nov 20, 2024
@anafalcao
Copy link
Collaborator

Thank you for submitting this PR so promptly, @ran-isenberg ! I was off today, but tomorrow I'll begin to review it :)

@anafalcao anafalcao changed the title feat(parser): Parser models for API GW Websockets Events feat(parser): add models for API GW Websockets events Nov 21, 2024
Copy link
Collaborator

@anafalcao anafalcao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @ran-isenberg, I suggested some changes in a initial review.
I see the CI is failing, but don't worry, it's not related to your PR. We are verifying it

@ran-isenberg
Copy link
Contributor Author

@anafalcao all fixed! also renamed one of the files and found a documentation mistake.
Please review again :)

Copy link
Collaborator

@anafalcao anafalcao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @ran-isenberg ! Looking good so far, but there are a few comments. Can you take a look, please?

docs/utilities/parser.md Outdated Show resolved Hide resolved
@anafalcao
Copy link
Collaborator

Np, thanks again @ran-isenberg ! I pushed those small changes, but the overall looks very good!
@leandrodamascena can you review it as well?
Thanks!

Copy link
Contributor

@leandrodamascena leandrodamascena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ran-isenberg! Thank you very much for sending this PR, it's very good as always.

I know that the event sent by AWS uses CamelCase by default and we can't change it for obvious reasons, but I would like to make the fields snake case on our side and work with Pydantic Fields. I also know that most of our events use CamelCase, but I would like to have this convention about capitalization - We've done this on newer models like Cloudformation Custom Resource, VPC Lattice 2, and Bedrock. Can you please change this @anafalcao? I left 2 examples that you can extend to any other.

After this change we are good to merge this PR! 🚀

Copy link
Contributor

@leandrodamascena leandrodamascena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing all the feedback @anafalcao!

I'm approving this PullRequest and please open a new issue to add the same support for Event Source Data Classes!

APPROVED!!!

@leandrodamascena leandrodamascena merged commit 20c0b74 into aws-powertools:develop Nov 24, 2024
10 of 12 checks passed
@ran-isenberg
Copy link
Contributor Author

Whoohooo! Thx @anafalcao and @leandrodamascena

@ran-isenberg ran-isenberg deleted the websocket branch November 25, 2024 04:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge documentation Improvements or additions to documentation feature New feature or functionality need-issue PRs that are missing related issues size/L Denotes a PR that changes 100-499 lines, ignoring generated files. tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: Parser models for API GW Websockets Events
3 participants