matcher: allow specifying only an on_no_match #19
Conversation
This provides a way to specify an action to match to without specifying a matcher. Signed-off-by: Snow Pettersen <snowp@lyft.com>
| // Optional OnMatch to use if the matcher above was unspecified or if matching using the specified | ||
| // matcher failed. | ||
| // If not specified, the match will be considered unsucessful if a matcher was not specified or | ||
| // if the specified matcher failed to match. | ||
| OnMatch on_no_match = 3; |
There was a problem hiding this comment.
The name of this field is a bit of a misnomer now (or maybe not if you wrap your head around an unspecified matcher as meaning "match nothing"), but not sure if we want to rename
xds/type/matcher/v3/matcher.proto
Outdated
| // If specified, the OnMatch is used, and the matcher is considered | ||
| // to have matched. | ||
| // If not specified, the matcher is considered not to have matched. | ||
| // Optional OnMatch to use if the matcher above was unspecified or if matching using the specified |
There was a problem hiding this comment.
Suggest phrasing this comment as:
"""
Optional OnMatch to use if no matcher above matched (e.g., if there are no matchers specified above, or if none of the matches specified above succeeded).
If no matcher above matched and this field is not populated, the match will be considered unsuccessful.
"""
|
Let me see what a allow all match action would look like |
Signed-off-by: Snow Pettersen <snowp@lyft.com>
snowp
force-pushed
the
allow-no-matcher
branch
from
df91029
to
b578d5e
Compare
|
IMO, it is better to remove the |
|
Sorry, @snowp, I guess I wasn't clear: I didn't mean to add a new first-class field for always-match. What I meant was that we could add a new custom matcher that always matches any input, which we can plug in as a custom matcher into the extension point. But I suspect that @zhxie is right, probably the original approach of removing the |
|
Yeah, I think the original was cleaner IMHO as well. |
snowp
force-pushed
the
allow-no-matcher
branch
from
3bc859c
to
000ce40
Compare
This reverts commit b578d5e. Signed-off-by: Snow Pettersen <snowp@lyft.com>
snowp
force-pushed
the
allow-no-matcher
branch
from
000ce40
to
699a52f
Compare
|
@htuch It looks like one of the CI checks didn't run for some reason. Is there a way to trigger a re-run? |
|
I think CI is broken as our image is too out-of-date "##[warning]An image label with the label Ubuntu16 does not exist." from https://dev.azure.com/cncf/xds/_build/results?buildId=94428&view=logs&j=8f9c5ecd-7085-541f-b145-4b6a0a921008. I don't see this though in https://github.com/cncf/xds/blob/main/ci/azure-pipelines.yml or if I search the repo. @lizan do you know what might be up here? |
|
Do we have any updates on this PR? |
|
I can force merge this PR if needed, @snowp? |
Signed-off-by: Snow Pettersen <snowp@lyft.com>
|
Let me see if I can successfully pull in this version in Envoy just to sanity check that the protos are good (I think with CI being broken we don't know) |
|
Ok this looks good to me, if we don't have a quick fix to CI I think a force merge would be good |
This provides a way to specify an action to match to without specifying a matcher.
Signed-off-by: Snow Pettersen snowp@lyft.com