-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
dynamic_modules: adds send response #37856
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Ben Plotnick <bplotnick@netflix.com>
Signed-off-by: Ben Plotnick <bplotnick@netflix.com>
Signed-off-by: Ben Plotnick <bplotnick@netflix.com>
Signed-off-by: Ben Plotnick <bplotnick@netflix.com>
@mathetake I took your send response impl from your PoC and ported it here. I haven't yet tested this with my module (hence why the PR is in draft). But i wanted to post the PR so you could take an early look if you wanted to |
Signed-off-by: Ben Plotnick <bplotnick@netflix.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.
thank you for taking a shot at this - i left a coupe of comments. Mainly I do not want to bring in a third party dependency in the core API layer.
thank you for fixing up the unit tests btw! |
Signed-off-by: Ben Plotnick <bplotnick@netflix.com>
Signed-off-by: Ben Plotnick <bplotnick@netflix.com>
Signed-off-by: Ben Plotnick <bplotnick@netflix.com>
Signed-off-by: Ben Plotnick <bplotnick@netflix.com>
80fa93e
to
bfb49f9
Compare
Signed-off-by: Ben Plotnick <bplotnick@netflix.com>
Signed-off-by: Ben Plotnick <bplotnick@netflix.com>
676334d
to
b8a7ef1
Compare
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 good to me, thank you for your work. deferring to @mattklein123 for the rest
Signed-off-by: Ben Plotnick <bplotnick@netflix.com>
Commit Message: Add
envoy_dynamic_module_callback_http_send_response
to the dynamic modules ABI and rust SDKAdditional Description: This adds the ability to send a local response back. I've also fixed up
abi_impl_test.cc
. Before, it was not asserting on the correct headers. Now i've moved to parameterized tests so we can assert on the correct headers and also split out the tests into smaller units.Risk Level: low
Testing: unit tests
Docs Changes:
Release Notes:
Platform Specific Features: