feat: forward compatible diregapic LRO support #1085
Conversation
Detect whether a method fulfills the criteria for DIREGAPIC LRO. If so, fudge the name of the generated method by adding the suffix '_primitive'. This change is made for both the synchronous and async client variants. Any generated unit tests are changed to use and reference the fudged name. The names of the corresponding transport method is NOT changed.
software-dov
requested review from
kbandes,
vchudnov-g,
noahdietz,
busunkim96,
atulep,
vam-google and
a team
google-cla
bot
added
the
cla: yes
There was a problem hiding this comment.
Looks good! Thanks for doing it. I have only one major comment: most probably we would need to populate https://github.com/googleapis/python-api-common-protos with externded_operations stuff (see the corresponding comment for details), like we have done with the other langauges.
Another question: have we agreed on the _primitive suffix? I do not have a strong opinion on the suffix itself, as long as it is as consistent as possible with other languages.
| @@ -0,0 +1,132 @@ | |||
| # -*- coding: utf-8 -*- | |||
There was a problem hiding this comment.
I believe we need to add these files into
https://github.com/googleapis/python-api-common-protos/tree/main/google
We have done simiilar procedure in other langauges already, for example:
Java: https://github.com/googleapis/java-common-protos/tree/main/proto-google-common-protos/src/main/java/com/google/cloud
Ruby: https://github.com/googleapis/common-protos-ruby/tree/main/google-cloud-common/lib/google/cloud
Once we do that, I believe we can remove most of these files from this PR.
There was a problem hiding this comment.
https://github.com/googleapis/python-cloud-common/tree/main/google/cloud/common/types was created for google/cloud/common_resources.proto, so I think it will be a more appropriate repo/package for extended_operations.proto.
Can the BUILD.bazel https://github.com/googleapis/googleapis/blob/master/google/cloud/BUILD.bazel be updated to generate this proto as well?
Another note, we've switched to generating proto-plus types by default instead of _pb2.py, will that be a problem for the generator or generated library code? @software-dov
I'm happy to assist with getting the protos published to the appropriate package once the questions above are sorted out.
There was a problem hiding this comment.
https://github.com/googleapis/googleapis/blob/master/google/cloud/BUILD.bazel#L51 already has a python target for extended_operations. But that one will create default stubs. If you want generate proto-plus ones, then it is still straightforward, but woluld require changing that target to py_gapic_library and py_gapic_assembly targets instead. I'm happy to assist with that.
There was a problem hiding this comment.
The extended_operations_pb2.py file is just used by the generator; I don't believe generating proto-plus types will work because proto-plus can't handle message extensions.
There was a problem hiding this comment.
@vam-google The other files added to the PR are necessary for the fragment tests. The only file that can and should be moved is extended_operations_pb2.py.
gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2
Show resolved
Hide resolved
|
@vam-google with regards to the |
|
@software-dov Which ones do ruby and go use? |
There was a problem hiding this comment.
LGTM, but please first finalize the suffix we want to use for this (_primitive or something else) with @vchudnov-g, and upate this PR accordingly to feature the proper suffix.
🤖 I have created a release \*beep\* \*boop\* --- ## [0.57.0](https://www.github.com/googleapis/gapic-generator-python/compare/v0.56.2...v0.57.0) (2021-11-17) ### Features * forward compatible diregapic LRO support ([#1085](https://www.github.com/googleapis/gapic-generator-python/issues/1085)) ([aa7f4d5](https://www.github.com/googleapis/gapic-generator-python/commit/aa7f4d568f7f43738ab3489fc84ce6bc5d6bda18)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Detect whether a method fulfills the criteria for DIREGAPIC LRO.
If so, fudge the name of the generated method by adding the suffix
'_primitive'. This change is made for both the synchronous and async
client variants. Any generated unit tests are changed to use and
reference the fudged name.
The names of the corresponding transport method is NOT changed.