[api] Move HeaderMatcher from api.v2.route to type.matcher package#4207
[api] Move HeaderMatcher from api.v2.route to type.matcher package#4207brirams wants to merge 4 commits intoenvoyproxy:masterfrom
Conversation
Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>
…rences Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>
Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>
source/common/common/matchers.cc
Outdated
| k.setCopy(header.key().c_str(), header.key().size()); | ||
| Http::HeaderString v; | ||
| v.setCopy(header.value().c_str(), header.value().size()); | ||
| static_cast<Http::HeaderMapImpl*>(context)->addViaMove(std::move(k), std::move(v)); |
There was a problem hiding this comment.
I'd like to not depend on the common/http package and I believe that means using the HeaderMap interface here -- any objections to changing this to do that?
There was a problem hiding this comment.
I believe you can use addCopy in HeaderMap, and pass the HeaderStrings directly from header here, this would avoid one copy above so it would be same. You might want to change the second parameter of addCopy from const string& to absl::string_view so it would avoid copying the value.
There was a problem hiding this comment.
addCopy takes an Http::LowerCaseString for key have to construct one in a similar way but did change value to be an absl::string_view.
There was a problem hiding this comment.
Ah I missed that HeaderString -> LowerCaseString part, then probably it is better to add addViaMove to the interface itself? I would prefer make those change in another PR and get it merged first.
There was a problem hiding this comment.
sure -- i can definitely do that
…ty to use that Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>
|
Serializing this work behind a more higher priority one here: #4239 |
|
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
|
Working a few higher priority tasks so will be making progress on a slower clip than originally expected. will be moving it forward though so pinging for that. |
|
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
|
just going to close this for now since other things are in flight. |
Description:
Since the
HeaderMatcherstruct is used in a variety of places, it made sense to move it into a common package(as discussed in slack. In doing so, also brought theHeaderUtilityclass along for the ride by colocating it withcommon/common/matchers.[cc|h]. Also updated doc references to new location.Risk Level: Low
Testing: Tests, new and old, pass.
Docs Changes:
version_history.rstto document move