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 without post-processing #1935

Merged
merged 110 commits into from
Sep 12, 2023

Conversation

diegomarquezp
Copy link
Contributor

@diegomarquezp diegomarquezp commented Aug 20, 2023

Modifies the logic of #1916 to support generation of showcase

NOTES

  • Referencing an external repository as the source of the protos when building showcase makes the proto folder to contain a path that includes the external reference. Googleapis-based generated libraries do not contain this difference (example) but instead contain the proto_path (i.e. path from repository root to proto location).
    • To fix this, I added an extra postprocessing step in the current showcase generation
    • I also tried modifying the bazel BUILD file to copy the protos into showcase but it produced a similar result - a path like proto/bazel-out/.../schema/....
  • The enable-golden-tests profile was updated so it compares the current source code of showcase against a recently generated library via the hermetic build scripts.
  • The verify profile calls the new scripts without merging them into the current showcase source and running a diff to confirm nothing has changed

PENDING

  • use current snapshot of gapic-generator-java to generate showcase (currently uses a published version)

function cleanup {
script_dir=$1
cd "${script_dir}"
rm -rdf gapic-generator-java* google schema protobuf-* protoc-gen-grpc-java* showcase-output out
Copy link
Member

Choose a reason for hiding this comment

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

Can we keep all of these folders in a temporary directory because when the script fails midway and cleanup is skipped, it's petty messy? It would be easier if there was a subdirectory under showcase like build that contained all of this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1. Similar to the temp bazel folders like bazel-out when we were using bazel.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@meltsufin would it make sense to have a separate PR for this? This will also affect the main generation scripts (cc: @blakeli0 )

PROTO_UNPACK_DIR=$PWD
# compare with proto library
PROTO_PROJECT_DIR='proto-gapic-showcase-v1beta1'
diff -ru "${SHOWCASE_DIR}/${PROTO_PROJECT_DIR}/src" "${SCRIPT_DIR}/showcase-output/proto-showcase-output/src"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now that we are generating all three modules at the same time, is it possible to have a diff for all three modules?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unless there is a better way than preparing two special folders with a similar structure and run diff on their roots I think the simplest is to just run diff one time for each of the folders (proto, gapic and grpc) in a single call to verify.sh. We don't compare all the contents of gapic-showcase or grpc-gapic-showcase-v1beta1 because there are other special files not generated by the scripts (e.g. ITs).

function cleanup {
script_dir=$1
cd "${script_dir}"
rm -rdf gapic-generator-java* google schema protobuf-* protoc-gen-grpc-java* showcase-output out
Copy link
Collaborator

Choose a reason for hiding this comment

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

+1. Similar to the temp bazel folders like bazel-out when we were using bazel.

Copy link
Member

@meltsufin meltsufin left a comment

Choose a reason for hiding this comment

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

You follow up on the outstanding comments in a separate PR.

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.

LGTM. It would be better if we can add unit tests for new utility functions as @JoeWang1127 mentioned if it's not too much, otherwise it's OK to add it in another PR.

@diegomarquezp
Copy link
Contributor Author

diegomarquezp commented Sep 12, 2023

LGTM. It would be better if we can add unit tests for new utility functions as @JoeWang1127 mentioned if it's not too much, otherwise it's OK to add it in another PR.

Thanks @blakeli0 and @JoeWang1127 . I added a couple tests for info extraction functions.

@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
0.0% 0.0% Duplication

@sonarqubecloud
Copy link

[java_showcase_unit_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
0.0% 0.0% Duplication

@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
0.0% 0.0% Duplication

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.

4 participants