-
Notifications
You must be signed in to change notification settings - Fork 469
feat: add HttpResolverAlpha resolver #7913
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
base: develop
Are you sure you want to change the base?
Conversation
|
3 similar comments
|
|
|
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #7913 +/- ##
===========================================
- Coverage 96.72% 96.71% -0.01%
===========================================
Files 275 276 +1
Lines 13214 13401 +187
Branches 1006 1030 +24
===========================================
+ Hits 12781 12961 +180
- Misses 325 327 +2
- Partials 108 113 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
|
|
I have some questions/concerns around whether this fits in the library. Our first tenet says we focus on AWS Lambda runtime while this goes quite outside of that. I can see some value in having a regular resolver running locally for local development, but in the current shape the new resolver requires a code change which is creates a precedent in us adding a significant runtime implementation for a non-Lambda environment. I am not opposing this feature, but I wonder if we can frame it differently or make it work differently. As a quick win, I'd feel more comfortable if this was 1/ listed under the testing section of the docs, and 2/ named in a way that clearly conveys it's meant to be used for local testing only. In both cases my concern is that this gives the false impression that we want to support non-Lambda environments - which is not the case I hope. Along the same lines, is there any way at all to get an existing resolver to work with the ASGI protocol when an env variable is set? I don't know the specifics but what I have in mind is basically customers using one of the already existing resolvers, and then when they set an env variable like Again, my goal here is mainly to make sure this is positioned in the correct way. Happy to discuss. |
|
Those are fair points. Thanks for raising them.
Actually there is a minimal code change between local and Lambda 1/ Change the resolver
I am good to name this Tests, local run, CI and more.
Hmm no, there isn't because I'll change the code behavior and it's not ideal.
Sure thing, I think your point about tests and naming make it clear. WDYT? |
dreamorosi
left a comment
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.
Sounds good, thanks for explaining.
I am ok with moving the section in the docs under testing plus renaming.
I'd suggest HttpResolverLocalDev to make it extra clear, but if it's too long HttpResolverLocal that you suggested + a call out in the docstring of the class is enough.
For future reference/readers, in similar cases where the boundary of what we build is not clear, we'll still want to evaluate whether something fits or not - so our tenet still stands.



Issue number: closes #7912
Summary
Adds
HttpResolverAlpha- a new resolver that implements ASGI for running Event Handler APIs locally with uvicorn or any ASGI server.Changes
HttpResolverAlphaclass inaws_lambda_powertools/event_handler/http_resolver.pyUser experience
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.