chore: add a GitHub Action for generating new clients using the hermetic build scripts#10488
Conversation
|
From https://github.com/googleapis/google-cloud-java/actions/runs/8181196370/job/22370571531?pr=10488 WIP fix #10496 This is unrelated to the work of this PR |
diegomarquezp
changed the title
diegomarquezp
changed the title
There was a problem hiding this comment.
Can we rename this file to something like add-new-client-config.py?
|
|
||
| client_documentation = f"https://cloud.google.com/java/docs/reference/{distribution_name_short}/latest/overview" | ||
|
|
||
| if library_name is None: |
There was a problem hiding this comment.
I think library_name is not required and will be default to api_shortname in our scripts, so we don't have to specify it.
There was a problem hiding this comment.
Makes sense, I removed this.
| type: string | ||
| description: | | ||
| `proto_path`: Path to proto file from the root of the googleapis repository to the | ||
| directory that contains the proto files (without the version). |
There was a problem hiding this comment.
Why not just use the versioned proto_path? I think it fits better since we can just add it as a GAPIC directly.
There was a problem hiding this comment.
This is to match the existing new-client.py script. The old one accepts root-level proto_paths, so a PR with all the released versions get created. See this example PR
There was a problem hiding this comment.
I was initially forcing the user to provide a versioned path until I saw the behavior of the old script
There was a problem hiding this comment.
I don't think we have to follow the same behavior, its a tool that is used only by our own team, if the new behavior means less code and no additional toil, then we should go for it.
There was a problem hiding this comment.
I don't think we have to follow the same behavior, its a tool that is used only by our own team, if the new behavior means less code and no additional toil, then we should go for it.
There was a problem hiding this comment.
Agreed. As discussed, I modified the script to support versioned proto_paths only.
| def __is_releasable_version(versioned_proto_path): | ||
| """true if the library definition found in `versioned_proto_path`: | ||
| - has a BUILD file | ||
| - the BUILD file has a `java_gapic_assembly_gradle_pkg` rule |
There was a problem hiding this comment.
Is there any use case that the BUILD file does not have a java_gapic_assembly_gradle_pkg rule?
There was a problem hiding this comment.
This is the heuristic we determined with @JoeWang1127 to tell if a library version has been released. @JoeWang1127 Would you mind adding a link to some doc or source to validate this?
There was a problem hiding this comment.
For example, https://github.com/googleapis/googleapis/tree/master/google/bigtable/v1 has no BUILD file, but I'm still looking for one that doesn't have a java_gapic_assembly_gradle_pkg rule.
There was a problem hiding this comment.
Here is a search query of a few libraries that do not have the java_gapic_assembly_gradle_pkg rule.
There was a problem hiding this comment.
I'm removing this logic in favor of supporting versioned proto_paths only
There was a problem hiding this comment.
I remembered I have seen an example of this, however I can't find it now.
We can ignore this case since we plan to use versioned proto_path.
| name: Generate new GAPIC client library (Hermetic Build) | ||
| on: | ||
| workflow_dispatch: | ||
| # some inputs are ommited due to limit of 10 input arguments |
There was a problem hiding this comment.
I know this is an existing limitation of Github actions, but IMO this is a indication that Github action may not be a good fit for new client library generation. Because we are clearly omitting some parameters here, they may not be "required" to generate a new library, but repo_metadata.json would be missing some info without those parameters.
I think we should find a different way sooner than later, but it would be out of scope for this PR. cc: @suztomo
There was a problem hiding this comment.
repo_metadata.json would be missing some info without those parameters.
What are missing?
There was a problem hiding this comment.
For example, these four in repo_metadata.json, we might be able to get them heuristically, but if we want to override them, there is no way other than manually adding them.
There was a problem hiding this comment.
As we discussed today, the long term solution is to
- Make the PR generation logic more generic so that it can recognize any changes to the generation config
- Trigger the generation process on every generation config change.
- Create a PR(Manually or still use this Github action) that adds a new section for the new client, which should trigger the process above.
CC: @JoeWang1127
There was a problem hiding this comment.
Thank you for the example.
manually adding them.
The best way is to automatically get the data from the service config yaml.
| return f'java-{library["api_shortname"]}' | ||
|
|
||
|
|
||
| def __get_proto_paths(proto_path: str, committish: str) -> List[str]: |
There was a problem hiding this comment.
This whole function can go away if we provide versioned proto path, so we have less code to maintain.
There was a problem hiding this comment.
I guess this is an unintentional change?
There was a problem hiding this comment.
Phew, yes it was. I restored it.
generation_config.yaml
Outdated
| gapic_generator_version: 2.37.0 | ||
| protobuf_version: '25.2' | ||
| googleapis_commitish: 6500290663163ba7dc6e0a35231772f5f78c3b62 | ||
| googleapis_commitish: e960a82d36e3ddaeb62f549dbd4c300e11e240dc |
There was a problem hiding this comment.
It shows as changing but it's the same as the latest:
There was a problem hiding this comment.
Can we revert this whole file?
blakeli0
left a comment
blakeli0
left a comment
There was a problem hiding this comment.
LGTM. Please resolve merge conflict before merging.
diegomarquezp
added
the
owlbot:run
gcf-owl-bot
bot
removed
the
owlbot:run
diegomarquezp
changed the title
4 participants
This PR enables new client generation using the hermetic build docker image.
It uses a combination of
Why a new workflow and README?
We do have https://github.com/googleapis/google-cloud-java/blob/main/.github/workflows/generate_new_client.yaml already, which relies on the (from now) old generation scripts. We are not replacing this workflow because we want to keep a fall back option in case anything happens while we roll out the hermetic build scripts.
Steps
new_client_hermetic_build/new-client.pythat adds an entry togeneration_config.yamlwith the specified arguments. The arguments are the 10 minimal arguments we find in https://github.com/googleapis/google-cloud-java/blob/main/.github/workflows/generate_new_client.yamla. It will search for all versioned
proto_paths found in the rootproto_path. If the givenproto_pathis versioned, the script will fail with a messageDemo PR
diegomarquezp#19
Demo diff - generate
cloudcontrolspartnerwith both github actions:I generated the libraries using a more up-to-date
googleapis_committishWe are currently ignoring differences in README and
metadata['repo']in our ITsnew-library/cloudcontrolspartner-EvjMmcomes from the old generation workflownew-library/cloudcontrolspartner-56m5lcomes from the hermetic build generation