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

Jump to bottom

feat: add gapic options as inputs to generate_library.sh #2121

Merged
merged 12 commits into from
Oct 17, 2023
Merged

feat: add gapic options as inputs to generate_library.sh #2121

merged 12 commits into from
Oct 17, 2023

Conversation

JoeWang1127
Copy link
Collaborator

In this PR:

  • add three optional inputs to generate_library.sh:
    • gapic_yaml
    • grpc_service_config
    • service_yaml
  • add parsing methods to parse three inputs from BUILD
  • add docs

@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Oct 13, 2023
@JoeWang1127
Copy link
Collaborator Author

Tested the PR in the following libraries:

# There are two grpc_service_config.json in proto_path
google/cloud/commerce/consumer/procurement/v1alpha1 google-cloud-consumer-procurement-v1alpha1-java
# There are two grpc_service_config.json exists in proto_path
google/cloud/optimization/v1 google-cloud-optimization-v1-java
# Service yaml is in the parent directory
google/cloud/videointelligence/v1beta2 google-cloud-videointelligence-v1beta2-java
google/cloud/videointelligence/v1p1beta1 google-cloud-videointelligence-v1p1beta1-java
google/cloud/videointelligence/v1p2beta1 google-cloud-videointelligence-v1p2beta1-java
google/cloud/videointelligence/v1p3beta1 google-cloud-videointelligence-v1p3beta1-java

@JoeWang1127 JoeWang1127 marked this pull request as ready for review October 16, 2023 16:17
@JoeWang1127 JoeWang1127 requested a review from a team as a code owner October 16, 2023 16:17
diegomarquezp
diegomarquezp reviewed Oct 16, 2023
View reviewed changes
Copy link
Contributor

@diegomarquezp diegomarquezp left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together. Just a couple comments.
I think we can expand proto_path_list.txt to include at least one library per case that is fixed by this PR

library_generation/test/test_utilities.sh
if [ -f "${file_path}/${gapic_option}" ]; then
gapic_option="${file_path}/${gapic_option}"
else
gapic_option=""
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean that the BUILD file may reference a non-existent yaml or json file? If not defaulting to an error, would it make sense to produce a warning to stderr to explain that the specified file by the BUILD file was not found and will just use the default empty string?

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 didn't find a case to use a non-existent file.

I'll add a warning in such case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done.

blakeli0
blakeli0 reviewed Oct 16, 2023
View reviewed changes
library_generation/utilities.sh Outdated
if [ "${rest_numeric_enums}" == "true" ]; then
rest_numeric_enums="rest-numeric-enums,"
else
rest_numeric_enums=""
fi
echo "transport=${transport},${rest_numeric_enums}grpc-service-config=${grpc_service_config},${gapic_config}api-service-config=${api_service_config}"
gapic_yaml="gapic-config=${gapic_yaml},"
echo "transport=${transport},${rest_numeric_enums}grpc-service-config=${grpc_service_config},${gapic_yaml}api-service-config=${service_yaml}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
echo "transport=${transport},${rest_numeric_enums}grpc-service-config=${grpc_service_config},${gapic_yaml}api-service-config=${service_yaml}"
echo "transport=${transport},rest_numeric_enums=${rest_numeric_enums},service-config=${grpc_service_config},gapic_yaml=${gapic_yaml},service-yaml=${service_yaml}"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

rest_numeric_enums is not like other gapic options, where key and value separated by "=". When set to false, it can't present in the argument, otherwise the generator will treat it as true.

sdk-platform-java/gapic-generator-java/src/main/java/com/google/api/generator/gapic/protoparser/PluginArgumentParser.java

Lines 60 to 62 in 4c96063

static boolean hasNumericEnumFlag(CodeGeneratorRequest request) {
return hasFlag(request.getParameter(), KEY_NUMERIC_ENUM);
}

@sonarqubecloud
Copy link

[gapic-generator-java-root] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sonarqubecloud
Copy link

[java_showcase_integration_tests] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@JoeWang1127 JoeWang1127 merged commit b17d8a1 into main Oct 17, 2023
@JoeWang1127 JoeWang1127 deleted the feat/add-inputs branch October 17, 2023 19:16
@release-please release-please bot mentioned this pull request Oct 17, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blakeli0 blakeli0 blakeli0 approved these changes

@diegomarquezp diegomarquezp diegomarquezp approved these changes

@meltsufin meltsufin Awaiting requested review from meltsufin

Assignees
No one assigned
Labels
size: m Pull request size is medium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JoeWang1127 @blakeli0 @diegomarquezp