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

Bigtable: add 'client_info' support to client. #7876

Merged
merged 1 commit into from
May 8, 2019
Merged

Bigtable: add 'client_info' support to client. #7876

merged 1 commit into from
May 8, 2019

Conversation

tseaver
Copy link
Contributor

@tseaver tseaver commented May 7, 2019

Forward when constructing GAPIC API client objects.

Toward #7825.

Forward when constructing GAPIC API client objects.
@tseaver tseaver added the api: bigtable Issues related to the Bigtable API. label May 7, 2019
@tseaver tseaver requested a review from tswast May 7, 2019 19:14
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 7, 2019
credentials=None,
read_only=False,
admin=False,
client_info=_CLIENT_INFO,
Copy link
Contributor

Choose a reason for hiding this comment

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

Since _CLIENT_INFO is mutable, should we be defaulting to None and making a copy of _CLIENT_INFO if not set? I guess we don't expect anyone to be mutating it, since _CLIENT_INFO and self._client_info are both private?

client_class = mock.Mock()
credentials = _make_credentials()
client = _Client(credentials)
client_info = client._client_info = mock.Mock()
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe use an autospec here?

client_class = mock.Mock()
emulator_host = emulator_channel = object()
credentials = _make_credentials()
client = _Client(
credentials, emulator_host=emulator_host, emulator_channel=emulator_channel
)
client_info = client._client_info = mock.Mock()
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto. We can use mock.create_autospec with instance=True to make doubly sure we don't use any properties on ClientInfo that don't exist.

@@ -100,6 +102,13 @@ class Client(ClientWithProject):
interact with the Instance Admin or Table Admin APIs. This
requires the :const:`ADMIN_SCOPE`. Defaults to :data:`False`.

:type: client_info: :class:`google.api_core.client_info.ClientInfo`
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be the google.api_core.gapic_v1.client_info.ClientInfo version, instead?

What happens when a google.api_core.client_info.ClientInfo is used? I imagine it might fail to populate the x-goog-api-client header?

@tseaver tseaver merged commit d4e503b into googleapis:master May 8, 2019
@tseaver tseaver deleted the 7825-bigtable-client_info-support branch May 8, 2019 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigtable Issues related to the Bigtable API. 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