Skip to content

test: use semantic network filter names#10122

Merged
mattklein123 merged 4 commits intoenvoyproxy:masterfrom
kyessenov:use_semantic_network_filter_names
Mar 3, 2020
Merged

test: use semantic network filter names#10122
mattklein123 merged 4 commits intoenvoyproxy:masterfrom
kyessenov:use_semantic_network_filter_names

Conversation

@kyessenov
Copy link
Contributor

Description: exercise semantic names for network filter extensions. Follow up to #10071.
Risk Level: low. there is a change to properly support type URL inside typed structs (stripping type.googleapis.com)
Testing: unit tests
Docs Changes: none
Release Notes:
/assign @zuercher

@kyessenov
Copy link
Contributor Author

I have another PR finishing the rename for all extensions. I think it's a good idea to keep it separate to make the review easier.

Copy link
Member

@zuercher zuercher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Are there tests around somewhere that continue to test the old paths?

@kyessenov
Copy link
Contributor Author

Yeah, the envoy.router extension continues to use by-name lookup, since it's easier to type than the whole path to the extension API.

@zuercher
Copy link
Member

I'm worried that we'll eventually excise all those and end up with an untested path without realizing it, and that it might stop working before the point where we intentionally delete it.

@kyessenov
Copy link
Contributor Author

I share the concern. Given that the typed config type URL takes precedence, we can only exercise the non-typed lookup for filters with no config, really. Router is one of them and we should keep it that way. The code path for all extension lookups is the same, so until we merge #10130 , we're safe from the risk. I should add a test case to #10130 to make sure we keep the empty-config lookup by name functional.

@kyessenov
Copy link
Contributor Author

kyessenov commented Feb 27, 2020

@zuercher Do you think we can get this in, and I'll work on the follow-up #10130?

@kyessenov
Copy link
Contributor Author

Hi! Can we merge this or is there something else that needs to be done?

@mattklein123 mattklein123 merged commit 3fb6c41 into envoyproxy:master Mar 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants