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

fix(common): resume sending "v" in "gccl" component of API header #7473

Merged
merged 1 commit into from
Oct 15, 2021

Conversation

devbww
Copy link
Contributor

@devbww devbww commented Oct 15, 2021

Resume sending the leading "v" in the value of the "gccl" component
of the X-Goog-Api-Client header (see #7383). The Cloud Spanner
frontend's route lookup service uses the version string in a semver
comparison to determine how the request should be routed, and it
currently expects to see a literal "v". (For the record, the
minimal version supported by the route lookup service is "v1.17.1".)

Changing the route lookup service to not care about the leading "v"
is non-trivial (it uses a single-value map, indexed by language, to
store the minimum supported version, and the value comes from a flag),
and even then the fix will take some time to roll out, so, in the
meantime, we revert to sending the "v".

Eventually eliminating the "v" is still a goal because it makes the
header conform to the specification, and it also makes C++ behave
like the other languages.


This change is Reviewable

Resume sending the  leading "v" in the value of the "gccl" component
of the `X-Goog-Api-Client` header (see googleapis#7383).  The Cloud Spanner
frontend's route lookup service uses the version string in a semver
comparison to determine how the request should be routed, and it
currently expects to see a literal "v".  (For the record, the
minimal version supported by the route lookup service is "v1.17.1".)

Changing the route lookup service to not care about the leading "v"
is non-trivial (it uses a single-value map, indexed by language, to
store the minimum supported version, and the value comes from a flag),
and even then the fix will take some time to roll out, so, in the
meantime, we revert to sending the "v".

Eventually eliminating the "v" is still a goal because it makes the
header conform to the specification, and it also makes C++ behave
like the other languages.
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 15, 2021
@google-cloud-cpp-bot
Copy link
Collaborator

Google Cloud Build Logs
For commit: bfc38af6dc4bfcd7d54a34f53a1691216137c3f7

ℹ️ NOTE: Kokoro logs are linked from "Details" below.

Copy link
Member

@dbolduc dbolduc left a comment

Choose a reason for hiding this comment

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

should we add a TODO that reminds us to remove the v at a later date?

@codecov
Copy link

codecov bot commented Oct 15, 2021

Codecov Report

Merging #7473 (bfc38af) into main (f5e33f3) will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #7473      +/-   ##
==========================================
- Coverage   93.64%   93.63%   -0.01%     
==========================================
  Files        1361     1361              
  Lines      118122   118120       -2     
==========================================
- Hits       110614   110604      -10     
- Misses       7508     7516       +8     
Impacted Files Coverage Δ
google/cloud/internal/api_client_header.cc 92.85% <ø> (-0.90%) ⬇️
google/cloud/grpc_error_delegate.cc 95.83% <0.00%> (-4.17%) ⬇️
...bigtable/examples/bigtable_hello_instance_admin.cc 81.31% <0.00%> (-2.20%) ⬇️
...le/cloud/storage/internal/curl_download_request.cc 88.51% <0.00%> (-1.12%) ⬇️
.../cloud/storage/benchmarks/throughput_experiment.cc 74.37% <0.00%> (-0.51%) ⬇️
...cloud/pubsub/internal/subscription_session_test.cc 97.75% <0.00%> (-0.25%) ⬇️
google/cloud/pubsub/samples/samples.cc 92.02% <0.00%> (-0.08%) ⬇️
...sub/internal/batching_publisher_connection_test.cc 99.60% <0.00%> (+0.19%) ⬆️
google/cloud/storage/internal/curl_handle.h 82.85% <0.00%> (+2.85%) ⬆️

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 f5e33f3...bfc38af. Read the comment docs.

@devbww
Copy link
Contributor Author

devbww commented Oct 15, 2021

should we add a TODO that reminds us to remove the v at a later date?

I was going to create an issue instead.

@devbww devbww marked this pull request as ready for review October 15, 2021 19:09
@devbww devbww requested a review from a team as a code owner October 15, 2021 19:09
@devbww devbww merged commit 575e976 into googleapis:main Oct 15, 2021
@devbww devbww deleted the generated-api-client-header branch October 15, 2021 19:22
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