-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Emit error on HTTP rules without a matching selector and log warning on unbound methods #1178
Emit error on HTTP rules without a matching selector and log warning on unbound methods #1178
Conversation
The original intention was to make this an error too, but that would break example generation badly.
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
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.
Excellent work! What does this it look like when you run it and there's a route mismatch?
In case of grpc method without HTTP rule:
In case of HTTP Rule without matching grpc method:
|
We submitted the CLA, but not yet processed I guess, I will check daily and update when there is progress. |
You trigger a check manually:
|
Thanks, I know that, but the CLA page does not show me as signed yet and according to the doc:
|
My bad! |
@googlebot I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
Thanks for your contribution! A pleasure working with you 😁 |
This reverts commit ee51345. Original PR: grpc-ecosystem#1178 In my opinion, logging this warning is unnecessary. It also breaks CockroachDB's build; it has broken [Uber's build] before. It is not a good assumption that every gRPC service defined needs to have a HttpRule, and the workarounds are expensive. [Uber's build]: uber/prototool#128
@johanbrandhorst What is rationale behind the warning on unbound methods? This produces a lot of warnings as we only want to generate a gateway for a subset of our gRPC services. |
I'd be happy to remove the unbound method warning, it's much less likely to be unintentional than the HTTP route mismatch. Does that sound okay @andrascz? |
Sure, it seems reasonable to make this configurable. |
Fixes #1175
I could not emit error in case of unbound methods as that would have break all the examples.