-
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
downstream: refactoring code to remove listener hard deps #24394
Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@@ -288,6 +288,13 @@ class FilterChainFactoryContext : public virtual FactoryContext { | |||
}; | |||
|
|||
using FilterChainFactoryContextPtr = std::unique_ptr<FilterChainFactoryContext>; | |||
using FilterChainsByName = absl::flat_hash_map<std::string, Network::DrainableFilterChainSharedPtr>; | |||
|
|||
class FilterChainBaseAction : public Matcher::Action { |
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.
in retrospect I should have commented this up when moving it. If there end up being no other comments I'm inclined to do in the immediate follow-up to spare the 4 hour CI process.
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.
SGTM! I did have one additional comment but it was to add a comment, so feel free to address both in a follow up instead of waiting on CI again!
I think clang tidy is one of those ones we have to force merge around: /build/tmp/_bazel_envoybuild/b570b5ccd0454dc9af9f65ab1833764d/execroot/envoy/./source/common/common/hash.h:13:10: error: 'xxhash.h' file not found [clang-diagnostic-error] https://github.com/envoyproxy/envoy/blob/main/source/common/common/BUILD#L142 |
const StreamInfo::StreamInfo& info) const { | ||
const Network::FilterChain* | ||
ActionImpl::get(const Server::Configuration::FilterChainsByName& filter_chains_by_name, | ||
const StreamInfo::StreamInfo& info) const { |
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.
Out of curiosity, why did this line change? It looks identical but just re-wrapped. Did clang-format dislike the old formatting, I guess?
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.
Server::FilterChainsByName -> Server::Configuration::FilterChainsByName
because it was declared in the envoy/ file which has a different namespace
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.
Wow! I read that probably 10 times before sending that comment and yet still failed to notice the difference! facepalm
Thanks!
namespace Envoy { | ||
namespace Server { | ||
|
||
#define ALL_LISTENER_STATS(COUNTER, GAUGE, HISTOGRAM) \ |
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.
Consider adding a brief comment here.
@@ -288,6 +288,13 @@ class FilterChainFactoryContext : public virtual FactoryContext { | |||
}; | |||
|
|||
using FilterChainFactoryContextPtr = std::unique_ptr<FilterChainFactoryContext>; | |||
using FilterChainsByName = absl::flat_hash_map<std::string, Network::DrainableFilterChainSharedPtr>; | |||
|
|||
class FilterChainBaseAction : public Matcher::Action { |
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.
SGTM! I did have one additional comment but it was to add a comment, so feel free to address both in a follow up instead of waiting on CI again!
….h-into-multiple-header-files * origin/main: Add setRequestDecoder to ResponseEncoder interface (#24368) downstream: refactoring code to remove listener hard deps (#24394) lb api: moving load balancing policy specific configuration to extension configuration (#23967) ci: Skip docker/examples verification for docs or mobile only changes (#24417) ci: run mobile GitHub Actions on every PR (#24407) mobile: remove `bump_lyft_support_rotation.sh` script (#24404) Add file size to DirectoryEntry (#24176) bazel: update to 6.0.0rc4 (#24235) bazel: update rules_rust (#24409) Ecds config dump recommit (#24384) bazel: add another config_setting incompatible flag (#24270) listeners: moving listeners to extension directory (#24248) mobile: build Swift with whole module optimization (#24396) ci: update `actions/setup-java` from v1 to v3.8 (#24393) Signed-off-by: JP Simard <jp@jpsim.com>
…-cpp-to-latest-version * origin/main: (23 commits) Reduce Route memory utilization by avoiding RuntimeData instances when not needed (#24327) build: fix compile error for mac (#24429) postgres: support for upstream SSL (#23990) iOS: split `EnvoyEngine.h` into multiple header files (#24397) mobile: check for pending exceptions after JNI call (#24361) Remove uneccessary `this->` from mobile engine builder (#24389) Add setRequestDecoder to ResponseEncoder interface (#24368) downstream: refactoring code to remove listener hard deps (#24394) lb api: moving load balancing policy specific configuration to extension configuration (#23967) ci: Skip docker/examples verification for docs or mobile only changes (#24417) ci: run mobile GitHub Actions on every PR (#24407) mobile: remove `bump_lyft_support_rotation.sh` script (#24404) Add file size to DirectoryEntry (#24176) bazel: update to 6.0.0rc4 (#24235) bazel: update rules_rust (#24409) Ecds config dump recommit (#24384) bazel: add another config_setting incompatible flag (#24270) listeners: moving listeners to extension directory (#24248) mobile: build Swift with whole module optimization (#24396) ci: update `actions/setup-java` from v1 to v3.8 (#24393) ... Signed-off-by: JP Simard <jp@jpsim.com>
plus follow-ups from #24394 Commit Message: n/a Additional Description: n/a Risk Level: low Testing: n/a Docs Changes: n/a Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Risk Level: low (simply moving code around)
Testing: n/a
Docs Changes: n/a
Release Notes: n/a