Import FunctionRpc.proto as a subtree its repo #101
Conversation
elprans
force-pushed
the
import-proto
branch
3 times, most recently
from
33fc6ad to
755be3b
Compare
mhoeger
left a comment
mhoeger
left a comment
There was a problem hiding this comment.
One quick note on the semantics of enable_content_negotiation. Otherwise, looks great and thank you for doing this so quickly!
azure/worker/bindings/http.py
Outdated
| status_code=str(status), | ||
| headers=headers, | ||
| is_raw=True, | ||
| enable_content_negotiation=True, |
There was a problem hiding this comment.
Could you set this to False? Or to let the user choose True if they want to?
On consuming this change, we'll change the host so that is_raw = !enable_content_negotiation. This means that enable_content_negotiation will default to false (same effects as is_raw = true), and we'd like to let users opt-in to the c# content negotiation if they were using it before.
There was a problem hiding this comment.
Which version of the host contains the inverted logic? We cannot just flip it, because currently enable_content_negotiation is the same field as is_raw, so flipping it will cause tests to fail.
There was a problem hiding this comment.
No current version of the host contains this logic. I was going to consume new language workers and update the host at the same time. Since that requires more coordination, I can also update the .proto file to accept both is_raw and enable_content_negotiation fields. Please let me know if you'd rather see that change!
is_raw will be deprecated no matter what, so I was hoping to get rid of it in the same step. But I might be getting ahead of myself :)
There was a problem hiding this comment.
The problem is that you've reused the the field in the proto spec, so we have to wait for a host version that supports the new interpretation of the field.
There was a problem hiding this comment.
Looks like we're at a bit of a deadlock since the host would have to consume a newer version of the language worker to work too. I'll update the .proto file to have enable_content_negotiation as a new field instead of a replacement!
There was a problem hiding this comment.
@mhoeger Yes, enable_content_negotiation=True works fine.
There was a problem hiding this comment.
Looking at the host PR, I actually think that the logic has not been reversed in all places, e.g line 166 in src/WebJobs.Script/Binding/Http/HttpBinding.cs
There was a problem hiding this comment.
Sorry I'm very confused - line 166 is the only place the logic is reversed (besides test cases)? (note: the ScriptObjectResult will throw a 406, but RawScriptResult will not)
There was a problem hiding this comment.
Sorry, this was a mistake on my end. Didn't flip enable_content_negotiation back to False when bumping to 2.0.11737. I amended the PR.
There was a problem hiding this comment.
Sounds great. Thanks! You actually helped us catch an unwanted revert in the master branch of functions-host as well, so thank you :)
|
@mhoeger can you please take a look and confirm the host version that has the changes? |
Added subtree from https://github.com/azure/azure-functions-language-worker-protobuf. Branch: dev. Commit: 894bebb70bf89f7f4017ec0a5c1d30c1b8f0f776 Closes: #99.
|
This needs to be merged as well for the current release. @elprans - anything else pending here? |
|
@elprans thank you! :) |
Added subtree from https://github.com/azure/azure-functions-language-worker-protobuf.
Branch: dev. Commit: 894bebb70bf89f7f4017ec0a5c1d30c1b8f0f776