-
Notifications
You must be signed in to change notification settings - Fork 54
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: generate a GAPIC library from api definition #3208
Conversation
@@ -40,21 +41,21 @@ | |||
repo_prefix = "https://github.com/googleapis" | |||
output_dir = shell_call("get_output_folder") | |||
# this map tells which branch of each repo should we use for our diff tests | |||
committish_map = { | |||
commitish_map = { |
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.
Fun fact: for a long time I thought it only was committish
(two t
s).
I happens to be an alternative convenience term coming from commit-ish
.
The git glossary offers either commit-ish
or committish
. I always thought this would be too much to start a campaign to update all config yamls and its definition.
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 Diego! That's good to know! Yeah I agree we should follow the naming conventions. Maybe an enhancement in the backlog.
def generate( | ||
baseline_generation_config_path: str, | ||
current_generation_config_path: str, | ||
repository_path: str, | ||
api_definition_path: 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.
I think this is more like a root folder of all api definitions. e.g. The proto_path configure in generation_config.yaml is a relative path to this path right?
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.
Maybe change this to api_definitions_path
or api_defs_path
?
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.
Changed it to api_definitions_path
.
# run hermetic code generation docker image. | ||
docker run \ | ||
--rm \ | ||
-u "$(id -u):$(id -g)" \ | ||
-v "$(pwd):${workspace_name}" \ | ||
-v "${m2_folder}":/home/.m2 \ | ||
-v "${api_def_dir}:${workspace_name}/api" \ |
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.
This is essentially googleapis folder, maybe we can just use /googleapis
to make it easier to understand?
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 try to use a generalized name here to specify that the script can generate from any protos, not necessarily from googleapis.
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.
The docker image can be used by any protos, but I think hermetic_library_generation.sh
is only going to be used for generating googleapis. For generating other protos, the volumn name actually doesn't really matter, it's good as long as the volumn and api-definition-path match.
That being said, it's fine if you want to make it generic, but maybe change it to apis
to imply that it could contain multiple.
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.
but I think hermetic_library_generation.sh is only going to be used for generating googleapis
Makes sense, I changed it to googleapis
.
@@ -40,21 +41,21 @@ | |||
repo_prefix = "https://github.com/googleapis" | |||
output_dir = shell_call("get_output_folder") | |||
# this map tells which branch of each repo should we use for our diff tests | |||
committish_map = { | |||
commitish_map = { |
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 Diego! That's good to know! Yeah I agree we should follow the naming conventions. Maybe an enhancement in the backlog.
Thanks Joe! I think this is a good first step, but it still requires us to have a generation_config.yaml and parsing googleapis committish from it. As a follow up, can we try to see how to generate a library with only a folder? Not relying on generation_config.yaml? |
Sure, I'll create a follow up PR to remove Generation config can be optional after all necessary parameters are read from service yaml. |
# run hermetic code generation docker image. | ||
docker run \ | ||
--rm \ | ||
-u "$(id -u):$(id -g)" \ | ||
-v "$(pwd):${workspace_name}" \ | ||
-v "${m2_folder}":/home/.m2 \ | ||
-v "${api_def_dir}:${workspace_name}/api" \ |
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.
The docker image can be used by any protos, but I think hermetic_library_generation.sh
is only going to be used for generating googleapis. For generating other protos, the volumn name actually doesn't really matter, it's good as long as the volumn and api-definition-path match.
That being said, it's fine if you want to make it generic, but maybe change it to apis
to imply that it could contain multiple.
library_generation/DEVELOPMENT.md
Outdated
* `-v /path/to/google-cloud-java:/workspace` maps the host machine's | ||
google-cloud-java folder to the /workspace folder. | ||
The image is configured to perform changes in this directory. | ||
Note that this setup assumes the api definition resides in host machine's |
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 change this example to use the api_definition_path
parameter?
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.46.0</summary> ## [2.46.0](v2.45.0...v2.46.0) (2024-09-23) ### Features * expose property in GrpcTransportChannel if it uses direct path. ([#3170](#3170)) ([9a432f7](9a432f7)) * generate a GAPIC library from api definition ([#3208](#3208)) ([b6b5d7b](b6b5d7b)) * Metrics tracer addAttribute map overload ([#3202](#3202)) ([1a988df](1a988df)) ### Bug Fixes * generate pr description with repo level change ([#3182](#3182)) ([edd2168](edd2168)) ### Dependencies * update dependency com.google.errorprone:error_prone_annotations to v2.32.0 ([#3192](#3192)) ([b280706](b280706)) * update dependency com.google.errorprone:error_prone_annotations to v2.32.0 ([#3193](#3193)) ([ed0cd17](ed0cd17)) * update dependency filelock to v3.16.1 ([#3210](#3210)) ([703ac3d](703ac3d)) * update dependency idna to v3.10 ([#3201](#3201)) ([211c3ec](211c3ec)) * update dependency org.threeten:threetenbp to v1.7.0 ([#3205](#3205)) ([c88a722](c88a722)) * update dependency org.threeten:threetenbp to v1.7.0 ([#3206](#3206)) ([3e9fbac](3e9fbac)) * update dependency platformdirs to v4.3.3 ([#3200](#3200)) ([b62b05d](b62b05d)) * update dependency platformdirs to v4.3.6 ([#3209](#3209)) ([227ffa5](227ffa5)) * update dependency urllib3 to v2.2.3 ([#3194](#3194)) ([f69d511](f69d511)) * update dependency virtualenv to v20.26.5 ([#3212](#3212)) ([d3ef97a](d3ef97a)) * update google api dependencies ([#3183](#3183)) ([02eea8d](02eea8d)) * update google auth library dependencies to v1.26.0 ([#3216](#3216)) ([0b369e9](0b369e9)) * update google auth library dependencies to v1.27.0 ([#3221](#3221)) ([a3cb9e7](a3cb9e7)) * update googleapis/java-cloud-bom digest to 06f632d ([#3198](#3198)) ([49dcd35](49dcd35)) * update googleapis/java-cloud-bom digest to e7d8909 ([#3207](#3207)) ([de497ee](de497ee)) * update opentelemetry-java monorepo to v1.42.1 ([#3189](#3189)) ([38117d8](38117d8)) * Upgrade Protobuf-Java to v3.25.5 ([#3217](#3217)) ([860c1bc](860c1bc)) </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>
In this PR:
To generate a GAPIC library with hermetic build image, the user has to prepare the api definition to a directory and set
--api-definition-path
to this directory.This feature allows the user to generate a GAPIC library from any protos, rather than checking in protos in googleapis repository.
Internal ticket: b/362705386