Skip to content

test: Add a sample generate_config yaml file. #2337

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

Merged
merged 9 commits into from
Jan 11, 2024
Merged

Conversation

blakeli0
Copy link
Collaborator

@blakeli0 blakeli0 commented Jan 4, 2024

Add a sample generation_config.yaml file for google-cloud-java.

This is intended to be a sample file to make sure we agree on the high level design of the config file. This does not intend to be a exhaustive list of parameters at this moment, we will refine the parameters along the way.

@blakeli0 blakeli0 requested a review from a team as a code owner January 4, 2024 18:19
@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Jan 4, 2024
#Required.
apis:
#Required. The folder name of the module. This is the destination-name in new-client.py.
- name: java-asset
Copy link
Contributor

Choose a reason for hiding this comment

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

How are we planning to deal with owlbot files owlbot.py and .OwlBot.yaml? Are we just going to assume they are in google-cloud-java/${name}?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think you can assume they exist for each api when the script make it to post-processing. If it does not exist, new-client.py or generate_library.sh should create it. cc: @JoeWang1127

#Optional.
rpc_documentation:
#Required.
services:
Copy link
Collaborator

Choose a reason for hiding this comment

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

What happens if there's a new version defined in googleapis?
Do we want to dynamically get the versioned path or add a service in this file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A new service will be added. Are new versions of APIs automatically updated now? I guess yes with proper owlbot.yaml config? if so, we may want to automatically update it with a separate process, or replace owlbot.yaml's functionality with this file. That's probably why @suztomo mentioned this idea in the meeting.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Are new versions of APIs automatically updated now?

Yes, OwlBot cli copies all versions of the services using .OwlBot.yaml.

I'm thinking the wrapper file (the one reading the config file and generate libraries) can find versioned directory within proto_path.

proto_path in new-client.py is not a versioned directory, i.e., google/cloud/alloydb, while proto_path in generate_library.sh is a versioned directory, i.e., google/cloud/alloydb/v1.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it's better to explicit list them, and/or support a wildcard configuration like google/cloud/asset/**. Because some product like Bigtable needs to include google/bigtable/admin/v2 and google/bigtable/v2, if we only supply google/bigtable, we will have to find all the services recursively and smartly, there might be some folders are excluded as well.

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 the wildcard/regex is fine.
For example: google/bigtable/v[1-9].* and google/bigtable/admin/v[1-9].*

#Required.
products:
#Required. Can be used for populating the folder name java-{api_shortName}. This is also the destination-name in new-client.py.
- api_shortname: asset
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you write an example of java-bigtable, which combines two clients: bigtable and bigtable-admin?

#Optional. This is a relative path to destination-path above. We will create a new versions.txt file if it is not specified
versions_path: versions.txt
#Required.
products:
Copy link
Contributor

Choose a reason for hiding this comment

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

Since gapic-generator-java updates don't reflect immediately in a single PR on google-cloud-java but rather gradually updating the libraries, would it make sense to have products with an optional gapic-generator-java version that could override the repo level config?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that's acceptable for now, but we should remove it and have the whole repo generated with a single version of generator all the time once we switch to the new process.

Copy link
Collaborator

@JoeWang1127 JoeWang1127 left a comment

Choose a reason for hiding this comment

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

Let's merge the config file and refine it in later PRs as it's difficult to set an appropriate format at this moment.

Comment on lines 15 to 16
#Optional. The root folder name of generated client libraries. If empty, modules will be created under current folder, useful for single module
destination-path: google-cloud-java
Copy link
Member

@suztomo suztomo Jan 11, 2024

Choose a reason for hiding this comment

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

useful for single module

By reading the comment, I'm not sure when I want to set a value for destination-path. Is the google-cloud-java repo or the java-bigtable repo a single module?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can have detailed instruction later. I mentioned single module because if we set destination-path to java-bigtable, the code will be generated into java-bigtable/java-bigtable, which is not ideal.

Copy link

Quality Gate Passed Quality Gate passed for 'gapic-generator-java-root'

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@blakeli0 blakeli0 changed the title test: Add a hermetic build test config yaml file. test: Add a sample generate_config yaml file. Jan 11, 2024
Copy link

Quality Gate Passed Quality Gate passed for 'java_showcase_integration_tests'

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@blakeli0
Copy link
Collaborator Author

@suztomo @JoeWang1127 @diegomarquezp, I'm merging this PR now. Please add additional comments regarding the parameters in internal docs.

@diegomarquezp Please use this as a template and add more configs for libraries/GAPICs to the file, so that it can replace proto_path_list.txt in integration tests. Also, I'm expecting a separate config file for each handwritten library, looking at the content of proto_path_list.txt, we will need one for bigtable/logging/pubsub/storage. Supporting all handwritten libraries can be a separate PR, testing java-bigtable in this PR should be good enough.

@blakeli0 blakeli0 merged commit c3d1142 into main Jan 11, 2024
@blakeli0 blakeli0 deleted the add-hermetic-build-config branch January 11, 2024 22:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants