-
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
Changed TestHooks to ListenerHooks #6642
Conversation
Signed-off-by: Randy Smith <rdsmith@google.com>
Signed-off-by: Randy Smith <rdsmith@google.com>
/retest |
🔨 rebuilding |
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.
Basically looks fine. Be sure to put this interface change in our import release notes, as this is an exposed C++ API change.
source/exe/main_common.h
Outdated
@@ -121,7 +121,7 @@ class MainCommon { | |||
PlatformImpl platform_impl_; | |||
Envoy::OptionsImpl options_; | |||
Event::RealTimeSystem real_time_system_; | |||
DefaultTestHooks default_test_hooks_; | |||
DefaultListenerHooks default_test_hooks_; |
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.
rename this variable?
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.
Done (grepped for and renamed all other test_hooks variables as well).
source/exe/main_common.h
Outdated
MainCommonBase(const OptionsImpl& options, Event::TimeSystem& time_system, TestHooks& test_hooks, | ||
Server::ComponentFactory& component_factory, | ||
MainCommonBase(const OptionsImpl& options, Event::TimeSystem& time_system, | ||
ListenerHooks& test_hooks, Server::ComponentFactory& component_factory, |
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.
This was pre-existing (I don't think I was in the review) but this might be IMO a little smoother to integrate if caller didn't have to specify these in the constructor, but could instead call addHook(...) or something.
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.
I don't offhand see how to implement that without either moving the server_ construction into the run() method (which I don't like since it means you can't poke at the constructed server before you run it) or completely refactoring how hooks are done through ServerImpl (since Test/ListenerHooks is an argument to the Server::InstanceImpl constructor). Given that I think I'll say I'm not doing this, and if you poke holes in my reasoning I'll do it in a later CL. But I'll respond now to give you as much chance as I can to argue with me :-}.
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.
I don't feel that strongly about it, now that it's in.
I would've suggested plumbing that addHook() functionality thru to the ServerImpl if I had been reviewing when this was initially added, but it's not the most important thing on our list of priorities at this point.
Signed-off-by: Randy Smith <rdsmith@google.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.
@lizan for senior committer and non-googler approval.
Ping? |
* master: docs: add extension policy (envoyproxy#6678) ext_authz: added ability to detect partial request body data (envoyproxy#6583) version_history.rst: jwt_authn change missed 1.10.0 (envoyproxy#6684) docs: fix link in pull request template (envoyproxy#6679) Explicitly convert absl::string_view to std::string. (envoyproxy#6687) docs: improving watermark docs/comments (envoyproxy#6683) http filter: add CSRF filter (envoyproxy#6470) event: reintroduce dispatcher stats (envoyproxy#6659) security: postmortem for CVE-2019-990[01] (envoyproxy#6597) Improve build rules for (test only) library quic_port_utils. (envoyproxy#6672) spell check: skip unsupported extensions when called with a file (envoyproxy#6648) Changed TestHooks to ListenerHooks (envoyproxy#6642) proto: move extension-specific linking validation into extensions (envoyproxy#6657) stats: add/test heterogenous set of StatNameStorage objects. (envoyproxy#6504) docs: move xds protocol to rst (envoyproxy#6670) fix version history order (envoyproxy#6671) Signed-off-by: Michael Puncel <mpuncel@squareup.com>
Signed-off-by: Randy Smith rdsmith@google.com
For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md
Description: Change name of class TestHooks to ListenerHooks
Risk Level: Low; name change only.
Testing: Existing tests
Docs Changes: n/a
Release Notes: n/a
Fixes #6641