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(generator): add options support to generated clients #7683

Merged
merged 4 commits into from
Dec 2, 2021

Conversation

devbww
Copy link
Contributor

@devbww devbww commented Dec 2, 2021

The generated client constructor now takes a defaulted Options argument,
which is merged with the default options for the service, and then stored
as a member.

Similarly, the generated client operations also take a defaulted Options
argument, which is then merged with the client options from above, and
installed as the prevailing options for the duration of the operation by
instantiating an OptionsSpan.

Any module within the scope of that operation, or its continuations, can
then retrieve those prevailing options. So, for example, a metadata
decorator could add a header based on a per-operation option, or a retry
policy might modify its behavior based on another option.


This change is Reviewable

The generated client constructor now takes a defaulted `Options` argument,
which is merged with the default options for the service, and then stored
as a member.

Similarly, the generated client operations also take a defaulted `Options`
argument, which is then merged with the client options from above, and
installed as the prevailing options for the duration of the operation by
instantiating an `OptionsSpan`.

Any module within the scope of that operation, or its continuations, can
then retrieve those prevailing options.  So, for example, a metadata
decorator could add a header based on a per-operation option, or a retry
policy might modify its behavior based on another option.

This PR just changes the generator and its tests.  Regeneration of the
libraries will happen in a separate PR.
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Dec 2, 2021
@google-cloud-cpp-bot
Copy link
Collaborator

Google Cloud Build Logs
For commit: 5c2f284c6b4f4dd98560d7285873f086fd28b633

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

@codecov
Copy link

codecov bot commented Dec 2, 2021

Codecov Report

Merging #7683 (3e73363) into main (823c121) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #7683   +/-   ##
=======================================
  Coverage   95.28%   95.29%           
=======================================
  Files        1254     1254           
  Lines      113503   113579   +76     
=======================================
+ Hits       108155   108230   +75     
- Misses       5348     5349    +1     
Impacted Files Coverage Δ
...egration_tests/golden/golden_kitchen_sink_client.h 100.00% <ø> (ø)
...tegration_tests/golden/golden_thing_admin_client.h 100.00% <ø> (ø)
generator/internal/descriptor_utils_test.cc 97.24% <ø> (ø)
...gration_tests/golden/golden_kitchen_sink_client.cc 100.00% <100.00%> (ø)
...egration_tests/golden/golden_thing_admin_client.cc 100.00% <100.00%> (ø)
generator/internal/client_generator.cc 100.00% <100.00%> (ø)
generator/internal/descriptor_utils.cc 95.69% <100.00%> (+<0.01%) ⬆️
...bigtable/examples/bigtable_hello_instance_admin.cc 81.31% <0.00%> (-2.20%) ⬇️
google/cloud/pubsub/samples/samples.cc 92.02% <0.00%> (-0.24%) ⬇️
... 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 823c121...3e73363. Read the comment docs.

@google-cloud-cpp-bot
Copy link
Collaborator

Google Cloud Build Logs
For commit: c58c31339413f1862fb2e8b4fd96666a7f48e610

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

@google-cloud-cpp-bot
Copy link
Collaborator

Google Cloud Build Logs
For commit: fd0e3edc05e9c55ff6b6a0ad7bb7adf759e32e0f

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

@google-cloud-cpp-bot
Copy link
Collaborator

Google Cloud Build Logs
For commit: 3e73363c696d7324b36c7dd956f074d667c01e12

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

@devbww devbww marked this pull request as ready for review December 2, 2021 09:46
@devbww devbww requested a review from a team as a code owner December 2, 2021 09:46
Copy link
Contributor

@coryan coryan 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 a question for you, if you agree with my reasoning please go ahead and merge.

@@ -51,6 +51,9 @@ mapfile -t actual_dirs < <(env -C "${INSTALL_PREFIX}" find -type d)
mapfile -t expected_dirs < <(cat ci/etc/expected_install_directories)
expected_dirs+=(
./include/google/api
./include/google/bigtable
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this change seems unrelated, but I guess you had to do it as part of regenerating all the libraries?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. That is, apart from the generator changes themselves, everything else here was either generated or required by a test because of the regeneration.

@@ -91,7 +91,7 @@ Status ClientGenerator::GenerateHeader() {
"$class_comment_block$\n"
"class $client_class_name$ {\n"
" public:\n"
" explicit $client_class_name$(std::shared_ptr<$connection_class_name$> connection);\n"
" explicit $client_class_name$(std::shared_ptr<$connection_class_name$> connection, Options options = {});\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think using an unqualified Options here is Okay, because this code lives in google::cloud::$library$::v1_X_Y any Options class or struct (say one generated by protos) would be in google::cloud::$library$::v1 or some such namespace.

Is my reasoning correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A little simpler that that, I think, in that I don't think we're expecting to find anything other than google::cloud::Options. We already do this for Options in existing library-qualified namespaces, and also for other "common" symbols.

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.

3 participants