Skip to content
This repository was archived by the owner on Jul 13, 2025. It is now read-only.

fix: correct gapic generator template logic#3185

Merged
stephaniewang526 merged 1 commit intogoogleapis:masterfrom
stephaniewang526:stephwang-fix
Apr 24, 2020
Merged

fix: correct gapic generator template logic#3185
stephaniewang526 merged 1 commit intogoogleapis:masterfrom
stephaniewang526:stephwang-fix

Conversation

@stephaniewang526
Copy link
Copy Markdown
Contributor

@stephaniewang526 stephaniewang526 commented Apr 24, 2020

Fixes #3181

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 24, 2020
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 24, 2020 •
edited
Loading

Codecov Report

Merging #3185 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #3185   +/-   ##
=========================================
  Coverage     87.13%   87.13%           
  Complexity     6075     6075           
=========================================
  Files           494      494           
  Lines         24039    24039           
  Branches       2610     2610           
=========================================
  Hits          20946    20946           
  Misses         2232     2232           
  Partials        861      861

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8a7d76c...6b03e53. Read the comment docs.

vam-google
vam-google reviewed Apr 24, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@vam-google vam-google left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good to me, but please validate that it actually generates what you want for your API (and for some other api if you can find some, which relies on same type of Mocks).

To do that, please pull googleapis repo, replace the gapic-generator repository target in WORKSPACE#L62:

http_archive(
    name = "com_google_api_codegen",
    strip_prefix = "gapic-generator-6b03e539065bece1ce84816ec212851885bca013",
    urls = ["https://github.com/stephaniewang526/gapic-generator/archive/6b03e539065bece1ce84816ec212851885bca013.zip"],
)

This change makes googleapis generation to pull gapic-generator from your fork and the specific commit in it with the fix.

Then on Linux machine, go to the pulled repo root dir (googleapis) and run:

bazel test //google/cloud/bigquery/storage/v1:storage_java_gapic_test_suite

and

bazel build //google/cloud/bigquery/storage/v1:google-cloud-bigquery-storage-v1-java

The last command will put generated .tar archive (with the client in it) to the following location (relative to the build root):

bazel-bin/google/cloud/bigquery/storage/v1/google-cloud-bigquery-storage-v1-java.tar.gz

Unpack that and check that the files are what they should be.

If all that works, then LGTM

@vam-google vam-google changed the title feat: correct gapic generator template logic fix: correct gapic generator template logic Apr 24, 2020
@stephaniewang526
Copy link
Copy Markdown
Contributor Author

stephaniewang526 commented Apr 24, 2020

Hi Vadym, thanks for the instructions. I tested this change in v1alpha2 (where we introduced write api changes) and was able to verify the following in the generated tar file google-cloud-bigquery-storage-v1alpha2-java.tar.gz:

public StreamObserver<AppendRowsRequest> appendRows(
      final StreamObserver<AppendRowsResponse> responseObserver) {
    StreamObserver<AppendRowsRequest> requestObserver =
        new StreamObserver<AppendRowsRequest>() {
          @Override
          public void onNext(AppendRowsRequest value) {
            requests.add(value);
            final Object response = responses.remove();
...

vam-google
vam-google approved these changes Apr 24, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@vam-google vam-google left a comment

Choose a reason for hiding this comment

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

LGTM

@stephaniewang526 stephaniewang526 merged commit a59457d into googleapis:master Apr 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@yihanzhen yihanzhen Awaiting requested review from yihanzhen

1 more reviewer

@vam-google vam-google vam-google approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Incorrect code generated

3 participants

@stephaniewang526 @vam-google @googlebot