-
Notifications
You must be signed in to change notification settings - Fork 53
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
feat: add transport
option to generation_config.yaml
#3052
Conversation
library_generation/README.md
Outdated
| rpc_documentation | No | | | ||
| cloud_api | No | `true` if not specified | | ||
| requires-billing | No | `true` if not specified | | ||
| transport | No | must be one of `grpc`, `rest`, `http` (alias) or `both`. This will override the value obtained from BUILD.bazel | |
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 think we use transport
from BUILD.bazel to determine if we should generate a grpc module or not, can you please confirm that? If yes, we should update the description to something like This value would only be used for generating .repo-metadata.json and relevant sections in README
.
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.
Done
:param library_path: the path to which the generated file goes | ||
:param language: programming language of the library | ||
:return: None | ||
""" | ||
transport = ( |
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.
Can we move this logic to generate_composed_library.py
, right before calling the utilities.py
, for simplicity?
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 initially did it that way, but then I found that it would be better for testing if we leave the transport
inference logic to generate_postprocessing_prerequisite_files
.
This test helper passes a hard-coded value:
sdk-platform-java/library_generation/test/utilities_unit_tests.py
Lines 279 to 286 in e46648f
transport = "grpc" | |
util.generate_postprocessing_prerequisite_files( | |
config=config, | |
library=library, | |
proto_path=proto_path, | |
transport=transport, | |
library_path=library_path, | |
) |
And we don't have unit tests for generate_composed_library
, so the only way to have a realistic execution path (other than the integration test) is by moving this logic to this method.
Do you think it's enough to make an exception?
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.
Resolution from discussion with @blakeli0: let's move the logic to the model class
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.
Done
@@ -224,7 +228,10 @@ def generate_postprocessing_prerequisite_files( | |||
# is one of grpc, http and both, | |||
if transport == "grpc": | |||
converted_transport = "grpc" | |||
elif transport == "rest": | |||
elif transport in [ |
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 guess this is because we use rest
in BUILD but http
in repo-metadata.json
? This is convenient but I feel we should just stick with rest
for consistency and simplicity.
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.
Done.
library_path=library_path, | ||
gapic_inputs=gapic_inputs, |
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.
from @blakeli0: let's encapsulate this logic in a function of model.LibraryConfig
so it's easier to test and we avoid passing gapic_inputs
down the call
@@ -101,6 +104,13 @@ def get_artifact_id(self) -> str: | |||
""" | |||
return self.get_maven_coordinate().split(MAVEN_COORDINATE_SEPARATOR)[-1] | |||
|
|||
def get_transport(self, gapic_inputs: GapicInputs) -> str: |
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.
Nit: Add a comment mentioning that the value returned from this method is only used for generating post-processing prerequisite files.
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.
Done
Quality Gate passed for 'gapic-generator-java-root'Issues Measures |
Quality Gate passed for 'java_showcase_integration_tests'Issues Measures |
🤖 I have created a release *beep* *boop* --- <details><summary>2.43.0</summary> ## [2.43.0](v2.42.0...v2.43.0) (2024-07-25) ### Features * add `transport` option to `generation_config.yaml` ([#3052](#3052)) ([3b1a915](3b1a915)) * get released version from versions.txt to render `README.md` ([#3007](#3007)) ([99bb2b3](99bb2b3)) * Introduce java.time to Gax-Java ([#1872](#1872)) ([308aeaf](308aeaf)) * Mark `getDefaultEndpoint()` with @ObsoleteApi ([#2347](#2347)) ([e46648f](e46648f)) * parse `BUILD.bzel` to determine whether a commit that only changed `BUILD.bazel` is a qualified commit ([#2937](#2937)) ([502f801](502f801)) ### Bug Fixes * Fix: ([d996c2d](d996c2d)) * `BaseApiTracer` to noop on attemptFailed via overloaded method call ([#3016](#3016)) ([2fc938a](2fc938a)) * Generator to skip generation for empty services. ([#3051](#3051)) ([ff2c485](ff2c485)) * restore hermetic build image publication ([#2952](#2952)) ([97a6d67](97a6d67)) ### Dependencies * update dependency com.fasterxml.jackson:jackson-bom to v2.17.2 ([#3028](#3028)) ([d16f9d1](d16f9d1)) * update dependency com.google.cloud.opentelemetry:detector-resources-support to v0.30.0 ([#2975](#2975)) ([b3ec93f](b3ec93f)) * update dependency com.google.cloud.opentelemetry:detector-resources-support to v0.31.0 ([#3044](#3044)) ([6bd07dc](6bd07dc)) * update dependency com.google.errorprone:error_prone_annotations to v2.29.2 ([#3058](#3058)) ([8ea0868](8ea0868)) * update dependency com.google.errorprone:error_prone_annotations to v2.29.2 ([#3059](#3059)) ([81b23dc](81b23dc)) * update dependency com.google.guava:guava to v33.2.1-jre ([#3027](#3027)) ([12ee456](12ee456)) * update dependency commons-codec:commons-codec to v1.17.1 ([#3049](#3049)) ([58d94b7](58d94b7)) * update dependency dev.cel:cel to v0.6.0 ([#3050](#3050)) ([bc332d9](bc332d9)) * update dependency net.bytebuddy:byte-buddy to v1.14.18 ([#3029](#3029)) ([8799cf6](8799cf6)) * update dependency org.apache.commons:commons-lang3 to v3.15.0 ([#3060](#3060)) ([2538334](2538334)) * update dependency org.checkerframework:checker-qual to v3.45.0 ([#2988](#2988)) ([4edd216](4edd216)) * update google api dependencies ([#2951](#2951)) ([c16f6c9](c16f6c9)) * update google auth library dependencies to v1.24.0 ([#3039](#3039)) ([98b5bd7](98b5bd7)) * update googleapis/java-cloud-bom digest to 47c5dbc ([#2974](#2974)) ([57623f0](57623f0)) * update grpc dependencies to v1.65.1 ([#3061](#3061)) ([27497e2](27497e2)) * update junit5 monorepo to v5.10.3 ([#2963](#2963)) ([bc55fe1](bc55fe1)) * update netty dependencies to v4.1.112.final ([#3057](#3057)) ([5af127b](5af127b)) * update opentelemetry-java monorepo to v1.40.0 ([#3035](#3035)) ([5c31c42](5c31c42)) * Use Gapic-Showcase v0.35.1 ([#3018](#3018)) ([43773f0](43773f0)) ### Documentation * add support option to 'new issue' choices ([#3055](#3055)) ([6a2a17d](6a2a17d)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
This PR adds a new optional entry "
transport
" togeneration_config.yaml
.The
transport
entry will only have effect in the postprocessing output (i.e. generated libraries will still have its transport inferred via BUILD.bazel). The main output file affected by this is.repo-metadata.json
and its derived files (e.g. README.md).This addresses the need to allow a custom transport for java-storage.