ecds: refactor FilterConfigProviderManagerImpl into base and derived classes#17852
ecds: refactor FilterConfigProviderManagerImpl into base and derived classes#17852snowp merged 3 commits intoenvoyproxy:mainfrom
Conversation
…classes Signed-off-by: Taylor Barrella <tabarr@google.com>
Signed-off-by: Taylor Barrella <tabarr@google.com>
Signed-off-by: Taylor Barrella <tabarr@google.com>
snowp
left a comment
snowp
left a comment
There was a problem hiding this comment.
Makes sense to me, thanks! @kyessenov do you want to take a look as well?
| const envoy::config::core::v3::ExtensionConfigSource& config_source, | ||
| const std::string& filter_config_name, Server::Configuration::FactoryContext& factory_context, | ||
| const std::string& stat_prefix, bool last_filter_in_filter_chain, | ||
| const std::string& stat_prefix, bool last_filter_in_filter_config, |
There was a problem hiding this comment.
I think we should keep the name last_filter_in_filter_chain. The condition is that only terminal filters are last in chains.
There was a problem hiding this comment.
Ah ok. I did it due to this since that code is going to be moved into the header. When I do that I can rename config -> chain from the code's body and elsewhere in the file as well
There was a problem hiding this comment.
So the plan is to handle this in a follow up or in this PR?
There was a problem hiding this comment.
A follow up. config is used in multiple other places too, not just here
There was a problem hiding this comment.
SGTM, let's merge this one then
kyessenov
left a comment
kyessenov
left a comment
There was a problem hiding this comment.
Looks reasonable to me except I disagree with the rename of a parameter.
3 participants
Commit Message:
ecds: refactor FilterConfigProviderManagerImpl into base and derived classes
Signed-off-by: Taylor Barrella tabarr@google.com
Additional Description: Part of #14696 (comment). The plan is for
FilterConfigProviderManagerImplto become a templateRisk Level: Low
Testing: Existing
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A
#14696