fix: add field headers for other http verbs#443
Conversation
googlebot
added
the
cla: yes
software-dov
left a comment
software-dov
left a comment
There was a problem hiding this comment.
Looks good, just some nitpicky codegolf on my end :P
I'll look into why the tests are failing.
Codecov Report
@@ Coverage Diff @@
## master #443 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 26 26
Lines 1453 1453
Branches 300 300
=========================================
Hits 1453 1453
Continue to review full report at Codecov.
|
busunkim96
left a comment
busunkim96
left a comment
There was a problem hiding this comment.
It looks like the last failing unit test is for a streaming method that uses field headers.
| with mock.patch.object( | ||
| type(client._transport.{{ method.name|snake_case }}), | ||
| '__call__') as call: | ||
| {% if method.void -%} |
There was a problem hiding this comment.
Copy-pasted from the unit test below this.
| ) | ||
| request = {{ method.input.ident }}() | ||
|
|
||
| {%- for field_header in method.field_headers %} |
There was a problem hiding this comment.
Tweaking this slightly for field headers like "/v1beta1/{endpoint.name=projects/*/locations/*/namespaces/*/services/*/endpoints/*}"
| metadata = tuple(metadata) + ( | ||
| gapic_v1.routing_header.to_grpc_metadata(( | ||
| {%- for field_header in method.field_headers %} | ||
| {%- if not method.client_streaming %} |
There was a problem hiding this comment.
Added this for a failing showcase test (see streaming method in proto and failing test).
Not sure if this is the right call though. What's the correct way to handle metadata/headers for streaming methods? @software-dov
There was a problem hiding this comment.
I was under the impression that headers for streaming methods don't make a lot of sense because they're not tightly bound to http semantics. If that's not correct, then I think we would need to ask someone who knows better.
There was a problem hiding this comment.
I was under the impression that headers for streaming methods don't make a lot of sense because they're not tightly bound to http semantics. If that's not correct, then I think we would need to ask someone who knows better.
3 participants
Closes #401