feat: Support LRO mixins over REST #1118
Conversation
Still to do: - fix generated import path in operations - goread service config to change HTTP bindings
This works from the command line: curl -i "http://localhost:7469/v1/operations/google.showcase.v1beta1.Echo/Wait/CgwIidqOlQYQl93JqQEaDAoKcmFpbnkganVuZQ==" -H "X-Goog-Api-Client: rest/0.0.0 gapic/0.0.0" -H 'Content-Type: application/json' Still todo: - Override HTTP annotations from the config file
|
Note that we also need to add an http rule to the WaitOperation rpc in api-common-proto submodule, which is what's causing CI to fail. What's SOP for doing this? |
|
Sent out a PR with the HTTP rule: googleapis/api-common-protos#111 |
server/genrest/genrest.go
Outdated
| router.HandleFunc("/v1beta1/{name:operations/.+}", rest.HandleGetOperation).Methods("GET") | ||
| router.HandleFunc("/v1beta1/{name:operations/.+}:delete", rest.HandleDeleteOperation).Methods("DELETE") | ||
| router.HandleFunc("/v1beta1/{name:operations/.+}:cancel", rest.HandleCancelOperation).Methods("POST") | ||
| router.HandleFunc("/v1/{name:operations/.+}:wait", rest.HandleWaitOperation).Methods("GET") |
There was a problem hiding this comment.
I took the approach of generating all the methods in the mixin protos. This means that if they are not referenced in the service config, their http bindings do not get overriden.
And adding WaitOperation to this service config causes a conflict with EchoClient.WaitOperation
There was a problem hiding this comment.
So we either live with the extra methods, or implement a strategy closer to that in the Go generator to ensure we don't generate the extra methods. Thoughts?
|
Restructured the code so we only generate code for the listed mixin methods. Service config path is still hard-coded at the moment. |
| @@ -13,6 +14,7 @@ require ( | |||
| github.com/spf13/viper v1.12.0 | |||
| golang.org/x/oauth2 v0.0.0-20220608161450-d0670ef3b1eb | |||
| golang.org/x/sync v0.0.0-20220601150217-0de741cfad7f | |||
| golang.org/x/xerrors v0.0.0-20220517211312-f3a8303e98df | |||
There was a problem hiding this comment.
Note to self: I don't think we need this, we can use stdlib errors because we support go1.13 <
| - selector: google.longrunning.Operations.DeleteOperation | ||
| delete: '/v1beta1/{name=/operations/*}' | ||
| delete: '/v1beta1/{name=operations/**}:delete' |
There was a problem hiding this comment.
Note to self: we shouldn't need the :delete suffix, this isn't a custom method.
There was a problem hiding this comment.
True. Thanks for the catch. (And fix)
|
|
||
| func init() { | ||
| mixinDescriptors = map[string][]*descriptor.FileDescriptorProto{ | ||
| "google.longrunning.Operations": { |
There was a problem hiding this comment.
Note to self: add the other mixin services too (Locations, IAMPolicy)
There was a problem hiding this comment.
I was getting some (probably trivial) errors with Locations, so I just excluded the others for now so we could get LRO working. But as you see, I copied your structure from gapic-generator-go so the others would be easy to add back in.
For future reference: Some of this code was adapted from gapic-generator-go.
https://github.com/googleapis/gapic-generator-go/blob/c9e3ce74af160bf72a8d140f2d4ecb167d35df25/internal/gengapic/mixins.go#L66
https://github.com/googleapis/gapic-generator-go/blob/f40c8300f39be4584ff195f1cbc0488bebd563ae/internal/gengapic/generator.go#L103-L117