-
Notifications
You must be signed in to change notification settings - Fork 125
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
BUG: oauth2client deprecated, use google-auth instead. #39
Conversation
Travis build w/ integration tests: https://travis-ci.org/tswast/pandas-gbq/builds/233446493 /cc @jonparrott |
Codecov Report
@@ Coverage Diff @@
## master #39 +/- ##
===========================================
- Coverage 74.42% 28.24% -46.18%
===========================================
Files 4 4
Lines 1552 1540 -12
===========================================
- Hits 1155 435 -720
- Misses 397 1105 +708
Continue to review full report at Codecov.
|
ci/requirements-2.7.pip
Outdated
python-gflags==2.0 | ||
oauth2client==1.5.0 | ||
httplib2>=0.9.1 | ||
google-api-python-client>=1.6.2, <2.0.0dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason you are putting the <2.0.0.dev
, ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, because if there is a 2.0 release by semantic versioning that is a breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks is not common practice and just hides errors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I can drop it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
ci/requirements-3.4.pip
Outdated
httplib2 | ||
google-api-python-client | ||
oauth2client | ||
httplib2>=0.9.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the idea of the different pip files would be to different versions, so for sure these should NOT have an upper bound
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove httplib2
from setup.py
altogether and use from googleapiclient.http import build_http
in the code?
http = httplib2.Http()
becomes
http = build_http()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we explicitly need httplib2
transport for google-auth-httplib2
to work, I'd prefer to keep this httplib2.Http()
.
Also, it's the way recommended here: https://github.com/GoogleCloudPlatform/google-auth-library-python-httplib2/blob/587e34ea4c9d331422e40ba8a55e9a8b8c946778/google_auth_httplib2.py#L71
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, Thanks for clarifying.
docs/source/changelog.rst
Outdated
@@ -4,7 +4,8 @@ Changelog | |||
0.1.7 / 2017-??-?? | |||
------------------ | |||
|
|||
- Resolve issue where the optional ``--noauth_local_webserver`` command line argument would not be propagated during the authentication process. | |||
- Use the `google-auth <https://google-auth.readthedocs.io/en/latest/>`__ library for authentication because oauth2client is deprecated. :issue:`37` | |||
- ``read_gbq`` now has a ``auth_local_webserver`` boolean argument for controlling whether to use web server or console flow when getting user credentials. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use the PR number as the issue number and (use parens)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
pandas_gbq/gbq.py
Outdated
@@ -6,57 +6,19 @@ | |||
import time | |||
import sys | |||
|
|||
import google.auth |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, you have to have a _try_imports
to guard against these missing. The parent package (pandas) only checks that gbq itself can be imported, w/o a helpful message (hey you are missing a dependency).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I included google-auth in the dependencies in setup.py? What case would the try imports protect against?
gbq is an optional dependency, but once you install it, pip will install all of its dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because u get unintelligible import exceptions otherwise
pandas_gbq/gbq.py
Outdated
credentials do not have access to the project (self.project_id) | ||
on BigQuery. | ||
""" | ||
with open('bigquery_credentials.dat') as credentials_file: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will hard fail with an odd message if this file doesn't exist. is there a reason this is hardcoded? (not to mention pathed), better to have its location specified by argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The file path was already hard-coded with the previous oauth2client.fileStorage('bigquery_credentials.dat')
call.
I've filed #41 to track adding a parameter to override this. Since this would be a new feature, I'd prefer to do this in a separate pull request.
I have added a check for if this file is unable to be opened/parsed plus an integration test for this case.
pandas_gbq/gbq.py
Outdated
""" | ||
Saves user account credentials to a local file. | ||
""" | ||
with open('bigquery_credentials.dat', 'wb') as credentials_file: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method will now log a message if the file cannot be saved. Since this doesn't prevent the credentials being used, it no longer throws an exception.
I've added a test for the case.
pandas_gbq/gbq.py
Outdated
client_secret='kOc9wMptUtxkcIFbtZCcrEAc', | ||
scope=self.scope, | ||
redirect_uri='urn:ietf:wg:oauth:2.0:oob') | ||
def get_user_account_credentials(self, auth_local_webserver=True): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you expecting a user to call this method directly? should this not be in the read_/to_ API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The get_user_account_credentials method was already there, I'm just swapping the implementation for google-auth instead of oauth2client.
|
||
client_config = { | ||
'installed': { | ||
'client_id': ('495642085510-k0tmvj2m941jhre2nbqka17vqpjfddtd' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this hardcoded?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it was hard coded previously. I just converted the variable to the expected format. This config variable is how Google knows to say Pandas GBQ is requesting access versus some other project.
pandas_gbq/tests/test_gbq.py
Outdated
@@ -83,100 +83,7 @@ def _get_private_key_contents(): | |||
return f.read() | |||
|
|||
|
|||
def _test_imports(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls don't remove things like this. just change to use the new deps. This is going to hard fail if deps are not installed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
furthermore the point of this is to have informative messages.
setup.py
Outdated
'pandas', | ||
'httplib2>=0.9.1', | ||
'google-api-python-client>=1.6.2, <2.0.0dev', | ||
'google-auth>=1.0.0, <2.0.0dev', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you need to verify whether these dep packages are avail on conda-forge as well (and if not, prod to have them built there). furthermore unless there is a really really good reason don't put an upper bound on deps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They aren't, yet. I'm prodding.
@tswast Thanks for the PR! I'm going to try to test this tonight. I'm hitting this error in my local testing. Have you encountered it before?
EDIT:
I am also seeing the following error. Could you try deleting your
|
You're right, I forgot to check for if the token file exists, and I didn't fail gracefully if application default credentials were not set. I'll add some tests for those cases tomorrow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've addressed most of your review comments and added a couple tests for things that the test suite and I missed the first time.
Building at:
https://travis-ci.org/tswast/pandas-gbq/builds/233838814
I'll push the google-auth maintainers to make a conda package.
ci/requirements-2.7.pip
Outdated
python-gflags==2.0 | ||
oauth2client==1.5.0 | ||
httplib2>=0.9.1 | ||
google-api-python-client>=1.6.2, <2.0.0dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
docs/source/changelog.rst
Outdated
@@ -4,7 +4,8 @@ Changelog | |||
0.1.7 / 2017-??-?? | |||
------------------ | |||
|
|||
- Resolve issue where the optional ``--noauth_local_webserver`` command line argument would not be propagated during the authentication process. | |||
- Use the `google-auth <https://google-auth.readthedocs.io/en/latest/>`__ library for authentication because oauth2client is deprecated. :issue:`37` | |||
- ``read_gbq`` now has a ``auth_local_webserver`` boolean argument for controlling whether to use web server or console flow when getting user credentials. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
ci/requirements-3.4.pip
Outdated
httplib2 | ||
google-api-python-client | ||
oauth2client | ||
httplib2>=0.9.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we explicitly need httplib2
transport for google-auth-httplib2
to work, I'd prefer to keep this httplib2.Http()
.
Also, it's the way recommended here: https://github.com/GoogleCloudPlatform/google-auth-library-python-httplib2/blob/587e34ea4c9d331422e40ba8a55e9a8b8c946778/google_auth_httplib2.py#L71
setup.py
Outdated
'pandas', | ||
'httplib2>=0.9.1', | ||
'google-api-python-client>=1.6.2, <2.0.0dev', | ||
'google-auth>=1.0.0, <2.0.0dev', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They aren't, yet. I'm prodding.
pandas_gbq/gbq.py
Outdated
client_secret='kOc9wMptUtxkcIFbtZCcrEAc', | ||
scope=self.scope, | ||
redirect_uri='urn:ietf:wg:oauth:2.0:oob') | ||
def get_user_account_credentials(self, auth_local_webserver=True): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The get_user_account_credentials method was already there, I'm just swapping the implementation for google-auth instead of oauth2client.
pandas_gbq/gbq.py
Outdated
credentials do not have access to the project (self.project_id) | ||
on BigQuery. | ||
""" | ||
with open('bigquery_credentials.dat') as credentials_file: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The file path was already hard-coded with the previous oauth2client.fileStorage('bigquery_credentials.dat')
call.
I've filed #41 to track adding a parameter to override this. Since this would be a new feature, I'd prefer to do this in a separate pull request.
I have added a check for if this file is unable to be opened/parsed plus an integration test for this case.
pandas_gbq/gbq.py
Outdated
""" | ||
Saves user account credentials to a local file. | ||
""" | ||
with open('bigquery_credentials.dat', 'wb') as credentials_file: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method will now log a message if the file cannot be saved. Since this doesn't prevent the credentials being used, it no longer throws an exception.
I've added a test for the case.
@tswast At your earliest convenience, please could you rebase to resolve conflicts? Also, please run
|
@tswast It's great to see that most integration tests are passing with the improvements made in this PR. Do you have any thoughts on how to resolve the following integration test failure? https://travis-ci.org/tswast/pandas-gbq/jobs/233838815#L1293 |
Could we change
to
in Separately, if I click 'Deny' instead of 'Allow' during the local user web authentication flow I receive the following exception. Would it be helpful to provide a better error message than the one provided by
|
I received a
|
If I set my
|
Yes, we should set the auth server to true in setup as you suggest. It's a good default to be false so it works in remote Jupyter notebooks, but py.test doesn't like it due to the output capturing. I think resource in use errors are happening because the table deletes are eventually consistent. I will add a loop to list the tables until they are gone. The GOOGLE_APPLICATION_CREDENTIALS set to empty string error sounds like a bug in google-auth. Let's file an issue over there for it. |
Okay, I've rebased, modified the integration tests to use local webserver by default, and added a couple more checks for invalid credentials. Integration tests are running here: |
Looks like all the test failures are in the dataset listing
I'll ping the BQ team that this is still an issue. Not sure how to work around that one, since it seems pretty clearly a backend bug (we're getting a token from the API, but then they can't find it when we use it) |
Oh, and latest pandas removed assertRaises, assert_equals. Tests running here: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tswast looks pretty good. a few doc comments. also pls add another CI build that works on the current version of deps. The idea is to make sure we fail gracefully, IOW. it is VERY likely that people will upgrade this and NOT there deps. SO need to make sure it still 'work' (in that it gives nice error messages). I don't believe pip will correctly upgrade things on a default installation, though conda will.
google-api-python-client==1.2 | ||
python-gflags==2.0 | ||
oauth2client==1.5.0 | ||
google-api-python-client |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you fix one of these builts to the oldest versions that are possible to make this work (of google-api-python-client at least)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
docs/source/changelog.rst
Outdated
- Drop support for Python 3.4 (:issue:`40`) | ||
- Use the `google-auth <https://google-auth.readthedocs.io/en/latest/>`__ library for authentication because oauth2client is deprecated. (:issue:`39`) | ||
- ``read_gbq`` now has a ``auth_local_webserver`` boolean argument for controlling whether to use web server or console flow when getting user credentials. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this a replacement for 35? if so, pls list that here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, done.
google_api_minimum_version = '1.2.0' | ||
# Version 1.6.0 is the first version to support google-auth. | ||
# https://github.com/google/google-api-python-client/blob/master/CHANGELOG | ||
google_api_minimum_version = '1.6.0' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, so put this version fixed in the ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌
return _try_credentials(self.project_id, credentials) | ||
|
||
def save_user_account_credentials(self, credentials): | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for new functions, add a versionadded 0.2.0
in the doc-string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
|
||
def get_service_account_credentials(self): | ||
import httplib2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are any of these new functions user API visible? if so, do a _try_imports at the top of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the get_*_credentials
functions will be used directly.
I noticed that read_gbq
and to_gbq
did not have the checks, so I've added it there.
pandas_gbq/gbq.py
Outdated
@@ -686,6 +750,9 @@ def read_gbq(query, project_id=None, index_col=None, col_order=None, | |||
Service account private key in JSON format. Can be file path | |||
or string contents. This is useful for remote server | |||
authentication (eg. jupyter iPython notebook on remote host) | |||
auth_local_webserver : boolean (default False) | |||
Use a local webserver when getting user credentials to handle | |||
OAuth authorization flow redirects. | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
versionadded tag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
pandas_gbq/gbq.py
Outdated
@@ -686,6 +750,9 @@ def read_gbq(query, project_id=None, index_col=None, col_order=None, | |||
Service account private key in JSON format. Can be file path | |||
or string contents. This is useful for remote server | |||
authentication (eg. jupyter iPython notebook on remote host) | |||
auth_local_webserver : boolean (default False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
boolean, default False
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
pandas_gbq/gbq.py
Outdated
@@ -686,6 +750,9 @@ def read_gbq(query, project_id=None, index_col=None, col_order=None, | |||
Service account private key in JSON format. Can be file path | |||
or string contents. This is useful for remote server | |||
authentication (eg. jupyter iPython notebook on remote host) | |||
auth_local_webserver : boolean (default False) | |||
Use a local webserver when getting user credentials to handle |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you can add a doc-link link on how this works would be great (in addition or just to gbq docs is fine as well)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added links.
The google-auth-oauthlib docs explain it pretty well.
pandas_gbq/gbq.py
Outdated
@@ -815,6 +883,9 @@ def to_gbq(dataframe, destination_table, project_id, chunksize=10000, | |||
Service account private key in JSON format. Can be file path | |||
or string contents. This is useful for remote server | |||
authentication (eg. jupyter iPython notebook on remote host) | |||
auth_local_webserver : boolean (default False) | |||
Use a local webserver when getting user credentials to handle | |||
OAuth authorization flow redirects. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
versionadded tag & doc-link
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed you comments. Build seems happier now, too. https://travis-ci.org/tswast/pandas-gbq/builds/234178457
google-api-python-client==1.2 | ||
python-gflags==2.0 | ||
oauth2client==1.5.0 | ||
google-api-python-client |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
docs/source/changelog.rst
Outdated
- Drop support for Python 3.4 (:issue:`40`) | ||
- Use the `google-auth <https://google-auth.readthedocs.io/en/latest/>`__ library for authentication because oauth2client is deprecated. (:issue:`39`) | ||
- ``read_gbq`` now has a ``auth_local_webserver`` boolean argument for controlling whether to use web server or console flow when getting user credentials. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, done.
google_api_minimum_version = '1.2.0' | ||
# Version 1.6.0 is the first version to support google-auth. | ||
# https://github.com/google/google-api-python-client/blob/master/CHANGELOG | ||
google_api_minimum_version = '1.6.0' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌
pandas_gbq/gbq.py
Outdated
@@ -686,6 +750,9 @@ def read_gbq(query, project_id=None, index_col=None, col_order=None, | |||
Service account private key in JSON format. Can be file path | |||
or string contents. This is useful for remote server | |||
authentication (eg. jupyter iPython notebook on remote host) | |||
auth_local_webserver : boolean (default False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
pandas_gbq/gbq.py
Outdated
@@ -686,6 +750,9 @@ def read_gbq(query, project_id=None, index_col=None, col_order=None, | |||
Service account private key in JSON format. Can be file path | |||
or string contents. This is useful for remote server | |||
authentication (eg. jupyter iPython notebook on remote host) | |||
auth_local_webserver : boolean (default False) | |||
Use a local webserver when getting user credentials to handle |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added links.
The google-auth-oauthlib docs explain it pretty well.
pandas_gbq/gbq.py
Outdated
@@ -686,6 +750,9 @@ def read_gbq(query, project_id=None, index_col=None, col_order=None, | |||
Service account private key in JSON format. Can be file path | |||
or string contents. This is useful for remote server | |||
authentication (eg. jupyter iPython notebook on remote host) | |||
auth_local_webserver : boolean (default False) | |||
Use a local webserver when getting user credentials to handle | |||
OAuth authorization flow redirects. | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
pandas_gbq/gbq.py
Outdated
@@ -815,6 +883,9 @@ def to_gbq(dataframe, destination_table, project_id, chunksize=10000, | |||
Service account private key in JSON format. Can be file path | |||
or string contents. This is useful for remote server | |||
authentication (eg. jupyter iPython notebook on remote host) | |||
auth_local_webserver : boolean (default False) | |||
Use a local webserver when getting user credentials to handle | |||
OAuth authorization flow redirects. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
return _try_credentials(self.project_id, credentials) | ||
|
||
def save_user_account_credentials(self, credentials): | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
|
||
def get_service_account_credentials(self): | ||
import httplib2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the get_*_credentials
functions will be used directly.
I noticed that read_gbq
and to_gbq
did not have the checks, so I've added it there.
pandas_gbq/gbq.py
Outdated
from googleapiclient.errors import HttpError # noqa | ||
except ImportError as ex: | ||
raise ImportError( | ||
"pandas requires google-api-python-client for Google BigQuery " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a stray space between Google
and BigQuery
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, removed. Thanks.
pandas_gbq/gbq.py
Outdated
except ImportError as ex: | ||
raise ImportError( | ||
"pandas requires google-api-python-client for Google BigQuery " | ||
"support: {0}".format(str(ex))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When using .format
, you don't need to str
, format
will call __str__
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
pandas_gbq/gbq.py
Outdated
|
||
http = httplib2.Http() | ||
try: | ||
http = AuthorizedHttp(credentials, http=http) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: shadowing variables makes me uncomfortable, I'd prefer you call this one authed_http
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Thanks for patching those in. I've rebased on master. |
Remove the use of oauth2client and use google-auth library, instead. See GH#37. Rather than check for multiple versions of the libraries, use the setup.py to specify compatible versions. I believe this is safe since Pandas checks for the pandas_gbq package. Since google-auth does not use the argparse module to override user authentication flow settings, add a parameter to choose between the web and console flow. Addresses some eventual consistency issues in table/dataset listing in the integration tests.
This method was removed in pandas-dev/pandas#16089 in favor of pytest.raises.
This method was removed in pandas-dev/pandas#16017 in favor of pytest.raises.
@jreback I'm going to merge this now. All review comments have been addressed and integration tests passed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks @tswast !
There was a conflict with the change log that was really painless to fix so I just corrected it myself to speed up the process. Authorship should be maintained.
Awesome. Thanks @parthea. |
google-auth, google-auth-oauthlib and google-auth-httplib2 are all conda packages. |
@parthea thanks for helping us get on conda! |
@jonparrott No problem. It was a great learning experience! Would you like to be added to the maintainer's list for the |
Sure.
…On Fri, Jun 16, 2017, 9:38 AM Anthonios Partheniou ***@***.***> wrote:
@jonparrott <https://github.com/jonparrott> No problem. It was a great
learning experience! Would you like to be added to the maintainer's list
for the google-auth* conda packages?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#39 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAPUc_kgIR-7ym19g4q_PMVyuyoaPG6vks5sEq9ugaJpZM4Nelh1>
.
|
manually check the installation to make sure it's picking things up may need to update install ibstructions as well |
ok need to change one of the builds to install all the deps via conda rather than pip maybe the 3.6 build volunteers? |
You mean in Travis? I've been meaning to learn Conda, so I'll volunteer. |
yep |
The installation of google-auth* conda packages appear to be working correctly with the following commands:
Yes, we'll need to update the documentation as well. I'll open a new issue. |
great job guys! |
Remove the use of oauth2client and use google-auth library, instead.
Rather than check for multiple versions of the libraries, use the
setup.py to specify compatible versions. I believe this is safe since
Pandas checks for the pandas_gbq package.
Since google-auth does not use the argparse module to override user
authentication flow settings, add a parameter to choose between the web
and console flow.
Closes #37.