Skip to content

refactor: consolidate BigQuery client creation and set user-agent #100

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

Merged
merged 6 commits into from
Apr 1, 2021

Conversation

tswast
Copy link
Collaborator

@tswast tswast commented Mar 25, 2021

Creating as draft, as I want to add unit tests first.

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #98 🦕

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Mar 25, 2021
@tswast tswast marked this pull request as ready for review March 31, 2021 17:03
@tswast tswast requested a review from shollyman March 31, 2021 17:05
project_id = default_project

return bigquery.Client(
client_info=google_client_info(),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Need to add hook to check if client_info is available before trying to add it.

https://github.com/pydata/pandas-gbq/blob/853f7922a0e0c853ed4d295ff14c41bfedf7e8d1/pandas_gbq/gbq.py#L364-L369

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, 1.12.0 was released 2019-05-16. That's close enough to the NEP 29 recommended 2 years, that I think it's worth just bumping the minimum version of google-cloud-bigquery in this case.

Copy link

@shollyman shollyman left a comment

Choose a reason for hiding this comment

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

minor comment: I feel like I've seen/heard multiple conversations about resolving default project recently. I'd love it if this is going to go the route of ADC that we're consistent about the lookup order across languages and clients.

@tswast tswast merged commit cc4e23d into googleapis:master Apr 1, 2021
@tswast tswast deleted the issue98-user-agent branch April 1, 2021 14:19
@tswast
Copy link
Collaborator Author

tswast commented Apr 1, 2021

@shollyman re: default project lookup, most of the hard work is done by google.auth.default. That should stay consistent with the ADC spec.

https://github.com/googleapis/python-bigquery-sqlalchemy/pull/100/files#diff-d0e22c51d2de44de91fbaefb7a9daa158cd6ae091e840a51984a8a3c05d8bb58R49

We don't hit that line if someone specifies a service account key, in which case we use the project ID from the JSON key file. That's consistent with what the google-cloud-core library does

https://github.com/googleapis/python-cloud-core/blob/1c9e969f8b05098e1b91ea71d62ca141e2304a6c/google/cloud/client.py#L85

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.

Include sqlalchemy/VERSION in the user-agent string when constructing BQ client(s)
2 participants