-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Enable proto schema for router_check_tool #6992
Conversation
Thanks for working on this @jyotima this will be great to have. I will take a pass in a couple of days, but in the interim can you work on fixing DCO, fixing format, and adding tests for the new code? Thank you! /wait |
Oh also this new feature will need version history notes and documentation here: https://www.envoyproxy.io/docs/envoy/latest/install/tools/route_table_check_tool. Thank you! |
02e1075
to
318a08a
Compare
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.
Thank you. Looking good. Flushing out some comments. We still need some tests for the new code here https://github.com/envoyproxy/envoy/tree/master/test/tools/router_check/test. @hennna can you potentially also take a look at this change?
/wait
77a93b6
to
413f59b
Compare
Signed-off-by: Jyoti Mahapatra <jmahapatra@lyft.com>
@mattklein123 I have modified route_tests.sh to make sure it exercises the new code path. Essentially, the tests are the same, but use a different schema based on the proto. |
@jyotima will take a look. Friendly request to not force push new commits in the future. It makes your change much more difficult to review. Thanks! |
You might consider setting up the support hooks or finding some other workflow that you like: https://github.com/envoyproxy/envoy/tree/master/support |
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.
Thanks looks good. Few small comments and we can ship!
/wait
@mattklein123 |
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.
LGTM, thanks for the great work! One small question.
/wait
@@ -46,7 +46,7 @@ message ValidationInput { | |||
|
|||
// This pseudo-header field includes the HTTP method. | |||
// The options are GET, PUT, or POST. When not specified the field defaults to GET. | |||
string method = 4 [(validate.rules).string = {in: ["GET", "POST", "PUT", ""]}]; |
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 method should still be required? WDYT?
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.
@mattklein123 Based on existing documentation method is optional
:method
(optional, string) The request method. If not specified, the default method is GET. The options are GET, PUT, or POST.
We can make this a breaking change too if it brings more clarity in usage. I think defaulting to GET makes sense.
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.
IMO I would make it required. I think the tests should be very specific so they are easy to visually understand.
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.
agree with the part about making it very explicit for consistency. I will make the change right away.
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.
@mattklein123 I made the change to make method
mandatory. I have put min_bytes=3
. I didn't want to put the exhaustive method list there, but all methods have a length of 3. Is it a good constraint to have?
@mattklein123 We are hitting |
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.
Thanks! Coverage is broken in master. Since you didn't add to the problem I will force merge this. Great work!
@mattklein123 Thank you so much!!! |
* master: (65 commits) proto: Add PATCH method to RequestMethod enum (envoyproxy#6737) exe: drop unused deps on zlib compressor code (envoyproxy#7022) coverage: fix some misc coverage (envoyproxy#7033) Enable proto schema for router_check_tool (envoyproxy#6992) stats: rework stat sink flushing to centralize counter latching (envoyproxy#6996) [test] convert lds api test config stubs to v2 (envoyproxy#7021) router: scoped rds (2c): implement scoped rds API (envoyproxy#6932) build: Add option for size-optimized binary (envoyproxy#6960) test: adding an integration test framework for file-based LDS (envoyproxy#6933) doc: update obsolete ref to api/XDS_PROTOCOL.md (envoyproxy#7002) dispatcher: faster runOnAllThreads (envoyproxy#7011) example: add csrf sandbox (envoyproxy#6805) fix syntax of gcov exclusion zone. (envoyproxy#7023) /runtime_modify: add support for query params in body (envoyproxy#6977) stats: Create stats for http codes with the symbol table. (envoyproxy#6733) health check: fix more fallout from inline deletion change (envoyproxy#6988) Max heap fix (envoyproxy#7016) Add support to unregister from lifecycle notifications (envoyproxy#6984) build spdy_core_alt_svc_wire_format (envoyproxy#7010) ext_authz: Make sure initiateCall only called once (envoyproxy#6949) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>
Description: Enable proto schema in router check tool to prepare future deprecation of json schema.
Risk Level: Low. Adds a new code path for router check tool.
Testing:
Docs Changes:
Release Notes: