Skip to content
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

router: Remove PathMatchType and PathMatchCriterion #20649

Merged

Conversation

tpetkov-VMW
Copy link
Contributor

Signed-off-by: Toma Petkov [email protected]

Commit Message: Remove PathMatchType enum and PathMatchCriterion class, as they aren't used anywhere.
Additional Description: PathMatchCriterion class is never instantiated anywhere, besides for inheritance and testing.
Risk Level: Low
Testing: Unit test (test/common/router)
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

@tpetkov-VMW
Copy link
Contributor Author

Proposal to remove PathMatchCriterion, based only on the codebase.
If there are, for example, pending changes that depend on it, I'll just close the PR.

@wrowe wrowe requested a review from htuch April 6, 2022 00:59
@wrowe
Copy link
Contributor

wrowe commented Apr 6, 2022

@htuch it appears you merged and @mrice32 made the initial contribution, if we can get some insight on why it goes unused, or approval to remove it?

@wrowe
Copy link
Contributor

wrowe commented Apr 6, 2022

/assign-from envoyproxy/envoy-maintainers

@repokitteh-read-only
Copy link

envoyproxy/envoy-maintainers assignee is @mattklein123

🐱

Caused by: a #20649 (comment) was created by @wrowe.

see: more, trace.

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

I don't recall the history on this. It seems fine to remove. Thank you!

@mattklein123 mattklein123 merged commit 7014ce6 into envoyproxy:main Apr 6, 2022
@htuch
Copy link
Member

htuch commented Apr 6, 2022

+1

@tpetkov-VMW tpetkov-VMW deleted the remove_path_match_criterion branch April 6, 2022 05:38
@stevenzzzz
Copy link
Contributor

hi there, this PR breaks our system, I think we need to reverse this PR if no strong opinion.

TLDR we need this field to take a peek into how route selection works.

Please see the original issue/PR for reference: #2531

@stevenzzzz
Copy link
Contributor

@htuch

@mattklein123
Copy link
Member

Sure feel free to revert but please add comments/tests that make it clear this is used in private extensions so it doesn't get removed again.

@stevenzzzz
Copy link
Contributor

thanks. @adisuissa could you please revert this as we've discussed.

adisuissa added a commit that referenced this pull request Apr 12, 2022
adisuissa added a commit to adisuissa/envoy that referenced this pull request Apr 12, 2022
yanavlasov pushed a commit that referenced this pull request Apr 13, 2022
…#20797)

* Revert "router: Remove PathMatchType and PathMatchCriterion (#20649)"

This reverts commit 7014ce6.

Signed-off-by: Adi Suissa-Peleg <[email protected]>
vehre-x41 pushed a commit to vehre-x41/envoy that referenced this pull request Apr 19, 2022
…xy#20649)" (envoyproxy#20797)

* Revert "router: Remove PathMatchType and PathMatchCriterion (envoyproxy#20649)"

This reverts commit 7014ce6.

Signed-off-by: Adi Suissa-Peleg <[email protected]>

Signed-off-by: Andre Vehreschild <[email protected]>
ravenblackx pushed a commit to ravenblackx/envoy that referenced this pull request Jun 8, 2022
ravenblackx pushed a commit to ravenblackx/envoy that referenced this pull request Jun 8, 2022
…xy#20649)" (envoyproxy#20797)

* Revert "router: Remove PathMatchType and PathMatchCriterion (envoyproxy#20649)"

This reverts commit 7014ce6.

Signed-off-by: Adi Suissa-Peleg <[email protected]>
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.

5 participants