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: Add x-goog-api-client header to rest clients #888

Merged
merged 6 commits into from
May 20, 2021

Conversation

vam-google
Copy link
Contributor

@vam-google vam-google commented May 17, 2021

Also add Content-Type: application/json header.

This PR depends on googleapis/python-api-core#189, and assumes that google-api-core version 1.27.0 has been already released (which was not the case on the moment of creation of this PR).

Also add `Content-Type: application/json` header
@vam-google vam-google requested a review from a team as a code owner May 17, 2021 09:50
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label May 17, 2021
@codecov
Copy link

codecov bot commented May 17, 2021

Codecov Report

Merging #888 (870124f) into master (7c185e8) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #888   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           26        27    +1     
  Lines         1608      1697   +89     
  Branches       328       347   +19     
=========================================
+ Hits          1608      1697   +89     
Impacted Files Coverage Δ
gapic/samplegen_utils/types.py 100.00% <ø> (ø)
gapic/samplegen_utils/utils.py 100.00% <ø> (ø)
gapic/utils/case.py 100.00% <ø> (ø)
gapic/utils/reserved_names.py 100.00% <ø> (ø)
gapic/generator/generator.py 100.00% <100.00%> (ø)
gapic/samplegen/samplegen.py 100.00% <100.00%> (ø)
gapic/schema/api.py 100.00% <100.00%> (ø)
gapic/schema/metadata.py 100.00% <100.00%> (ø)
gapic/schema/wrappers.py 100.00% <100.00%> (ø)
gapic/utils/__init__.py 100.00% <100.00%> (ø)
... and 3 more

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 ce558ac...870124f. Read the comment docs.

@@ -207,8 +207,11 @@ class {{ service.name }}RestTransport({{ service.name }}Transport):
url += '?{}'.format('&'.join(query_params)).replace(' ', '+')

# Send the request
headers = dict(metadata)
headers['Content-Type'] = 'application/json'
Copy link
Contributor

Choose a reason for hiding this comment

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

Naive question: this is not x-goog-api-client. Where does the name content-type come from? Is there documentation somewhere describing it?

Copy link
Contributor Author

@vam-google vam-google May 18, 2021

Choose a reason for hiding this comment

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

The Content-Type header is a part of HTTP standard: https://datatracker.ietf.org/doc/html/rfc2616#section-14.17

Copy link
Contributor

Choose a reason for hiding this comment

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

@software-dov See also the internal REGAPIC gRPC transcoding design doc, where we call this out explicitly (relatively recent addition, since we found some RPCs fail without it)

@vam-google
Copy link
Contributor Author

@busunkim96 PTAL

Copy link
Contributor

@busunkim96 busunkim96 left a comment

Choose a reason for hiding this comment

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

LGTM.

I suspect the coverage drop is from the clauses I added to keep compatibility with older google-auth and google-api-core versions in 89d6f35#diff-f7a16a65f061822bcc73b8296f4dc837353d379d8d9cc5307982cb6941442835.

@busunkim96
Copy link
Contributor

Opened PR to remove code / tests for google-api-core < 1.26.0. See #893

Once that is merged updating this branch should resolve the coverage issues.

@busunkim96
Copy link
Contributor

@vam-google CI is now passing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants