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.

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 showcase using docker image #3568

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

diegomarquezp
Copy link
Contributor

Fixes #2470

Approach

In hermetic_build/

  • Introduce a new configuration entry in the library model where we can specify the folder name when in a monorepo (i.e. allows creation of sdk-platform/showcase instead of sdk-platform/java-showcase)
  • In gapic_inputs, allow nested parentheses, allow parsing additional protos from java_gapic_library (this is necessary since the original showcase repo doesn't specify additional protos and we use our own custom BUILD.bazel with the additional protos specified in the java library rule).

In showcase/

  • Revamp the generation and verification scripts used by -Penable-golden-tests and -Pupdate to build and use the Hermetic Build Docker image

Effects

  • New samples/ folder added in showcase
  • Reformatted all of grpc, proto and gapic libraries in showcase/

@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Jan 14, 2025
@diegomarquezp diegomarquezp self-assigned this Jan 14, 2025
@diegomarquezp diegomarquezp marked this pull request as draft January 14, 2025 23:36
@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Jan 15, 2025
Copy link

Quality Gate Failed Quality Gate failed for 'gapic-generator-java-root'

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)
27.8% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

Copy link

Quality Gate Passed Quality Gate passed for 'java_showcase_integration_tests'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@diegomarquezp diegomarquezp marked this pull request as ready for review January 15, 2025 18:25
Copy link
Collaborator

@blakeli0 blakeli0 left a comment

Choose a reason for hiding this comment

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

Thanks Diego! This approach looks good in general, however, have we ever considered treating showcase a dedicate repo instead of a library in sdk-platform-java? Meaning that we create a separate generation_config.yaml in showcase module?

@@ -95,6 +95,14 @@ changed_libraries=$(python hermetic_build/common/cli/get_changed_libraries.py cr
--current-generation-config-path="${generation_config}")
echo "Changed libraries are: ${changed_libraries:-"No changed library"}."

# do not generate showcase automatically
Copy link
Collaborator

Choose a reason for hiding this comment

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

We probably do want to generate showcase automatically? For example, if we update protoc version in the docker image, we do want the showcase module to be updated.

libraries:
- api_shortname: showcase
folder_name: showcase # prevents java-showcase
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel it is probably too much effort to introduce a new argument folder_name, just to prevent showcase to be renamed to java-showcase. I think it is OK to rename it.

- proto_path: google/rpc/context
- proto_path: google/shopping/type
- proto_path: google/type
- proto_path: google/api
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this change intended?

# fixed.
rest_numeric_enums = False,
service_yaml = "showcase_v1beta1.yaml",
test_deps = [
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 make this file minimum? For example, I don't think we need any test_deps since we only parses rest_numeric_enums, transport and include_samples.

@@ -17,3 +17,6 @@ proto-google-iam-v2:1.45.1:1.45.1
grpc-google-iam-v2:1.45.1:1.45.1
google-cloud-core:2.49.1:2.49.1
google-cloud-shared-dependencies:3.41.1:3.41.1
gapic-showcase:0.0.0:0.0.1-SNAPSHOT
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is OK as long as release-please does not start bumping versions for showcase. Can you confirm it or maybe @suztomo can confirm?

# download api definitions from googleapis repository
googleapis_commitish=$(grep googleapis_commitish generation_config.yaml | cut -d ":" -f 2 | xargs)
api_def_dir=$(mktemp -d)
git clone https://github.com/googleapis/googleapis.git "${api_def_dir}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe I missed it but I don't see these repos being cleaned up after the generation is completed.

# looks at sdk-platform-java/showcase/gapic-showcase/pom.xml to extract the
# version of gapic-showcase
# see https://github.com/googleapis/gapic-showcase/releases
showcase_version=$(get_version_from_pom \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this used? We should still use it to checkout showcase repo with a release tag.

--transport "${transport}"
echo "generating showcase"
workspace_name="/workspace"
docker run \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this is using docker now, I don't think we can run them from local Mac or Windows anymore, can you please update the README of the showcase module to indicate it?

}
# This is a special case for showcase, where the additional protos
# are not declared in the gapic library instead
pattern_gapic_library_to_additional_protos = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you mind elaborating on why we need this special case?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

showcase samples should be part of golden tests
2 participants