Skip to content
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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Jul 22, 2024
Merged

feat: add transport option to generation_config.yaml #3052

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
42648dc
feat: allow custom `transport` in config yaml
diegomarquezp Jul 17, 2024
6e2686c
format
diegomarquezp Jul 17, 2024
faf5830
add comment to test
diegomarquezp Jul 17, 2024
be42912
clarify effects of transport option in yaml
diegomarquezp Jul 18, 2024
1700842
only allow "rest" as an option to produce "http" metadata
diegomarquezp Jul 18, 2024
3bc9425
move transport inference to model class
diegomarquezp Jul 22, 2024
f7ef615
add tests
diegomarquezp Jul 22, 2024
715667a
restore utils comment
diegomarquezp Jul 22, 2024
53681e9
improve comment
diegomarquezp Jul 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 25 additions & 23 deletions library_generation/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,29 +105,31 @@ The library level parameters define how to generate a (multi-versions) GAPIC
library.
They are shared by all GAPICs of a library.

| Name | Required | Notes |
|:----------------------|:--------:|:-----------------------------------------------------------------------------------|
| api_shortname | Yes | |
| api_description | Yes | |
| name_pretty | Yes | |
| product_docs | Yes | |
| library_type | No | `GAPIC_AUTO` if not specified |
| release_level | No | `preview` if not specified |
| api_id | No | `{api_shortname}.googleapis.com` if not specified |
| api_reference | No | |
| codeowner_team | No | |
| client_documentation | No | |
| distribution_name | No | `{group_id}:google-{cloud_prefix}{library_name}` if not specified |
| excluded_poms | No | |
| excluded_dependencies | No | |
| googleapis_commitish | No | use repository level `googleapis_commitish` if not specified. |
| group_id | No | `com.google.cloud` if not specified |
| issue_tracker | No | |
| library_name | No | `api_shortname` is not specified. This value should be unique among all libraries. |
| rest_documentation | No | |
| rpc_documentation | No | |
| cloud_api | No | `true` if not specified |
| requires-billing | No | `true` if not specified |
| Name | Required | Notes |
|:----------------------|:--------:|:----------------------------------------------------------------------------------------------------------------|
| api_shortname | Yes | |
| api_description | Yes | |
| name_pretty | Yes | |
| product_docs | Yes | |
| library_type | No | `GAPIC_AUTO` if not specified |
| release_level | No | `preview` if not specified |
| api_id | No | `{api_shortname}.googleapis.com` if not specified |
| api_reference | No | |
| codeowner_team | No | |
| client_documentation | No | |
| distribution_name | No | `{group_id}:google-{cloud_prefix}{library_name}` if not specified |
| excluded_poms | No | |
| excluded_dependencies | No | |
| googleapis_commitish | No | use repository level `googleapis_commitish` if not specified. |
| group_id | No | `com.google.cloud` if not specified |
| issue_tracker | No | |
| library_name | No | `api_shortname` is not specified. This value should be unique among all libraries. |
| rest_documentation | No | |
| 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 |
Copy link
Collaborator

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done



Note that `cloud_prefix` is `cloud-` if `cloud_api` is `true`; empty otherwise.

Expand Down
7 changes: 5 additions & 2 deletions library_generation/generate_composed_library.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,13 +72,16 @@ def generate_composed_library(
gapic_inputs = parse_build_file(build_file_folder, gapic.proto_path)
# generate postprocessing prerequisite files (.repo-metadata.json, .OwlBot-hermetic.yaml,
# owlbot.py) here because transport is parsed from BUILD.bazel,
# which lives in a versioned proto_path.
# which lives in a versioned proto_path. The value of transport will be
# overriden by the config object if specified. Note that this override
# does not affect library generation but instead used only for
# generating postprocessing files such as README.
util.generate_postprocessing_prerequisite_files(
config=config,
library=library,
proto_path=util.remove_version_from(gapic.proto_path),
transport=gapic_inputs.transport,
library_path=library_path,
gapic_inputs=gapic_inputs,
Copy link
Contributor Author

@diegomarquezp diegomarquezp Jul 18, 2024 •
edited
Loading

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

)
temp_destination_path = f"java-{gapic.proto_path.replace('/','-')}"
effective_arguments = __construct_effective_arg(
Expand Down
4 changes: 4 additions & 0 deletions library_generation/model/library_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ def __init__(
extra_versioned_modules: Optional[str] = None,
recommended_package: Optional[str] = None,
min_java_version: Optional[int] = None,
transport: Optional[str] = None,
):
self.api_shortname = api_shortname
self.api_description = api_description
Expand All @@ -78,6 +79,7 @@ def __init__(
self.recommended_package = recommended_package
self.min_java_version = min_java_version
self.distribution_name = self.__get_distribution_name(distribution_name)
self.transport = transport

def get_library_name(self) -> str:
"""
Expand Down Expand Up @@ -144,6 +146,7 @@ def __eq__(self, other):
and self.extra_versioned_modules == other.extra_versioned_modules
and self.recommended_package == other.recommended_package
and self.min_java_version == other.min_java_version
and self.transport == other.transport
)

def __hash__(self):
Expand Down Expand Up @@ -175,6 +178,7 @@ def __hash__(self):
self.extra_versioned_modules,
self.recommended_package,
self.min_java_version,
self.transport,
]
+ [config.proto_path for config in self.gapic_configs]
).encode("utf-8")
Expand Down
6 changes: 6 additions & 0 deletions library_generation/model/repo_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,12 @@ def get_library_version(self, artifact_id: str) -> str:
return self.library_versions.get(artifact_id, NEW_CLIENT_VERSION)

def __parse_versions(self) -> dict[str, str]:
"""
For a given versions.txt file (defined in self.versions_file)
creates a map of artifact-id to its version

:return: a map "artifact-id -> version"
"""
library_versions = dict()
with open(self.versions_file) as f:
for line in f.readlines():
Expand Down
16 changes: 16 additions & 0 deletions library_generation/test/resources/goldens/.repo-metadata-custom-transport-golden.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
{
"api_shortname": "secretmanager",
"name_pretty": "Secret Management",
"product_documentation": "https://cloud.google.com/solutions/secrets-management/",
"api_description": "allows you to encrypt, store, manage, and audit infrastructure and application-level secrets.",
"client_documentation": "https://cloud.google.com/java/docs/reference/google-cloud-secretmanager/latest/overview",
"release_level": "preview",
"transport": "http",
"language": "java",
"repo": "googleapis/google-cloud-java",
"repo_short": "java-secretmanager",
"distribution_name": "com.google.cloud:google-cloud-secretmanager",
"api_id": "secretmanager.googleapis.com",
"library_type": "GAPIC_AUTO",
"requires_billing": true
}
32 changes: 30 additions & 2 deletions library_generation/test/utilities_unit_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
from pathlib import Path
from library_generation.utils import utilities as util
from library_generation.model.gapic_config import GapicConfig
from library_generation.model.gapic_inputs import GapicInputs
from library_generation.model.generation_config import GenerationConfig
from library_generation.model.library_config import LibraryConfig
from library_generation.test.test_utils import FileComparator
Expand Down Expand Up @@ -57,6 +58,14 @@
api_description="example description",
gapic_configs=list(),
)
test_library_with_custom_transport = LibraryConfig(
api_shortname="secretmanager",
name_pretty="Secret Management",
product_documentation="https://cloud.google.com/solutions/secrets-management/",
api_description="allows you to encrypt, store, manage, and audit infrastructure and application-level secrets.",
gapic_configs=list(),
transport="http",
)


class UtilitiesTest(unittest.TestCase):
Expand Down Expand Up @@ -225,6 +234,26 @@ def test_generate_postprocessing_prerequisite_files_proto_only_repo_success(self
)
self.__remove_postprocessing_prerequisite_files(path=library_path)

def test_generate_postprocessing_prerequisite_files__custom_transport_set_in_config__success(
self,
):
"""
This test generates files for `test_library_with_custom_transport`, which
has an explicit value for transport declared (http). This is expected to
override the value obtained in BUILD.bazel via gapic_inputs.parse(). For
testing purposes, we test with a default GapicInputs object, whose transport
is set to "grpc".
"""
library_path = self.__setup_postprocessing_prerequisite_files(
combination=2, library=test_library_with_custom_transport
)

file_comparator.compare_files(
f"{library_path}/.repo-metadata.json",
f"{library_path}/.repo-metadata-custom-transport-golden.json",
)
self.__remove_postprocessing_prerequisite_files(path=library_path)

def test_prepare_repo_monorepo_success(self):
gen_config = self.__get_a_gen_config(2)
repo_config = util.prepare_repo(
Expand Down Expand Up @@ -276,12 +305,11 @@ def __setup_postprocessing_prerequisite_files(
library.library_type = library_type
config = self.__get_a_gen_config(combination, library_type=library_type)
proto_path = "google/cloud/baremetalsolution/v2"
transport = "grpc"
util.generate_postprocessing_prerequisite_files(
config=config,
library=library,
proto_path=proto_path,
transport=transport,
gapic_inputs=GapicInputs(), # defaults to transport=grpc
library_path=library_path,
)
return library_path
Expand Down
13 changes: 10 additions & 3 deletions library_generation/utils/utilities.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import shutil
from pathlib import Path
from library_generation.model.generation_config import GenerationConfig
from library_generation.model.gapic_inputs import GapicInputs
from library_generation.model.library_config import LibraryConfig
from typing import List
from library_generation.model.repo_config import RepoConfig
Expand Down Expand Up @@ -186,7 +187,7 @@ def generate_postprocessing_prerequisite_files(
config: GenerationConfig,
library: LibraryConfig,
proto_path: str,
transport: str,
gapic_inputs: GapicInputs,
library_path: str,
language: str = "java",
) -> None:
Expand All @@ -198,11 +199,14 @@ def generate_postprocessing_prerequisite_files(
:param library: the library configuration
:param proto_path: the path from the root of googleapis to the location of the service
protos. If the path contains a version, it will be removed
:param transport: transport supported by the library
:param gapic_inputs: gapic_inputs obtained from the library's BUILD.bazel
:param library_path: the path to which the generated file goes
:param language: programming language of the library
:return: None
"""
transport = (
Copy link
Collaborator

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?

Copy link
Contributor Author

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?

Copy link
Contributor Author

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

gapic_inputs.transport if library.transport is None else library.transport
)
library_name = library.get_library_name()
artifact_id = library.get_artifact_id()
if config.contains_common_protos():
Expand All @@ -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 [
Copy link
Collaborator

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

"rest",
"http",
]: # http can also be specified via generation_config.yaml
converted_transport = "http"
else:
converted_transport = "both"
Expand Down
Loading