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

pkg/spanner/client: Add optional option.ClientOption parameters to Ne… #72

Merged
merged 2 commits into from
Sep 17, 2023

Conversation

mskwon
Copy link
Contributor

@mskwon mskwon commented Jan 31, 2023

…wClient constructor

Fixes #59

WHAT

This changes the constructor signature for the wrench client so that client options can be passed to the wrapped spanner clients. As the options are passed as a variadic parameter, there is no impact on pre-existing code.

WHY

The ability to pass client options allows for the passing of option.WithGRPCConn. This makes it so that wrench will work with spannertest - an in-memory fake Cloud Spanner which can be used for unit tests.

@google-cla
Copy link

google-cla bot commented Jan 31, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@mskwon mskwon force-pushed the cons-client-opts-59 branch from 5a30ae1 to 4d71584 Compare January 31, 2023 18:15
@kazegusuri
Copy link
Collaborator

Please sign the CLA.

@mskwon
Copy link
Contributor Author

mskwon commented Mar 20, 2023

Apologies for the delay here

@mskwon
Copy link
Contributor Author

mskwon commented Aug 22, 2023

Bump

@kazegusuri
Copy link
Collaborator

kazegusuri commented Sep 17, 2023

Thank you for making this PR. I thought the option should be inside the wrench's config struct instead of first type options because it's the Spanner specific options. I create another PR on the top of your commit.

@kazegusuri
Copy link
Collaborator

Ah I need to merge this PR first to use your commit because we can't use other merge options except squash and merge.

@kazegusuri kazegusuri merged commit c0cfaef into cloudspannerecosystem:master Sep 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support passing option.ClientOption to NewClient in the spanner pkg
2 participants