-
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
fix(pluginsocket.proto): type for set_upstream
#12727
Conversation
Fix the return type for the `.Service.SetUpstream` external plugin PDK method. Fixes issue #114. Sister PR: Kong/kong#12727
Fix the return type for the `.Service.SetUpstream` external plugin PDK method. Fixes issue Kong/go-pdk#114. Sister PR: Kong/go-pdk#191
13bc72e
to
66712fb
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.
I jumped the gun a little bit with approval. Could you clarify this one question for me?
@@ -328,7 +328,7 @@ service Kong { | |||
rpc Router_GetRoute(google.protobuf.Empty) returns (Route); | |||
rpc Router_GetService(google.protobuf.Empty) returns (Service); | |||
|
|||
rpc Service_SetUpstream(String) returns (google.protobuf.Empty); |
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.
Does it work with only Bool
? The general PDK says that there are 2 values returned: (bool, string) - https://docs.konghq.com/gateway/latest/plugin-development/pdk/kong.service/#kongserviceset_upstreamhost
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.
Good question!
I have verified the change works for both cases.
Longer answer:
Although it fixes the behavior described in Kong/go-pdk#114, the go-pdk does not currently handle consistently the nil, err
protocol adopted in the Lua PDK -- ie, most methods do not return the err
on the Go PDK side (only other errors are reported, like connection errors, protobuf errors, etc). Also, on the Go PDK side the SetUpstream
method does not return a boolean, only an error
. Fixing this means an API breaking change has to be introduced, so this really needs to be done before we release v1.0.0
, but not in the scope of this fix. I will add this to my future improvements notes.
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.
Oh, ok! That makes sense. Thanks 👍 !
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 for clarifying!
Fix the return type for the `.Service.SetUpstream` external plugin PDK method. Fixes issue #114. Sister PR: Kong/kong#12727
Fix the return type for the `.Service.SetUpstream` external plugin PDK method. Fixes issue #114. Sister PR: Kong/kong#12727
Fix the return type for the `.Service.SetUpstream` external plugin PDK method. Fixes issue #114. Sister PR: Kong/kong#12727
Successfully created cherry-pick PR for |
Summary
Fix the return type for the
.Service.SetUpstream
external plugin PDK method.Fixes issue Kong/go-pdk#114.
Sister PR: Kong/go-pdk#191
Issue reference
Fixes issue Kong/go-pdk#114.