-
Notifications
You must be signed in to change notification settings - Fork 34
[WIP] examples: Add http-filter-cc using bzlmod #1041
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: main
Are you sure you want to change the base?
Conversation
81b7d03 to
e1a1fd4
Compare
| namespace Envoy { | ||
| namespace Http { | ||
|
|
||
| class HttpSampleDecoderFilterConfig { |
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.
If the purpose is to act as an example, I would split FilterConfig out into a separate .h and .cc file (this isn't common in other filter implementations, but it is clearer and makes pieces easier to find - I think the main reason it isn't common is that everyone is cargo-cult-copying from existing examples), and put everything in a namespace more similar to the namespaces other HTTP filters do. (Envoy / Extensions / HttpFilters / Sample)
| namespace Server { | ||
| namespace Configuration { |
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 should be the same namespace as the rest of the filter, and just do using Server::Configuration::NamedHttpFilterConfigFactory etc.
But also, don't use NamedHttpFilterConfigFactory, use Extensions::HttpFilters::Common::DualFactoryBase, and override createFilterFactoryFromProtoTyped and createRouteSpecificFilterConfigTyped, and use resolveMostSpecificPerFilterConfig in the filter, to provide a really good example. Maybe also register a factory for running the filter as an upstream filter.
| } | ||
| }; | ||
|
|
||
| static Registry::RegisterFactory<HttpSampleDecoderFilterConfigFactory, NamedHttpFilterConfigFactory> |
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 think the preferred form for this is
REGISTER_FACTORY(HttpSampleDecoderFilterConfigFactory, Server::Configuration::NamedHttpFilterConfigFactory);
(Even if you're using DualFactoryBase it's still NamedHttpFilterConfigFactory)
e1a1fd4 to
263c82d
Compare
263c82d to
e466dad
Compare
e466dad to
e3eba93
Compare
23edfa9 to
5367dbb
Compare
Co-authored-by: phlax <454682+phlax@users.noreply.github.com>
be6ccdb to
9a2a4f9
Compare
Signed-off-by: Ryan Northey <ryan@synca.io>
9a2a4f9 to
d39e010
Compare
No description provided.