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

ENH: Save BigQuery account credentials in a hidden user folder #83

Merged

Conversation

parthea
Copy link
Contributor

@parthea parthea commented Aug 21, 2017

Closes #41

This PR adds ability for users to choose the location where BigQuery user account credentials will be stored via the credentials_path parameter.

This PR also changes the location of the file bigquery_credentials.dat that contains the BigQuery user account credentials. Previously this file was saved in the current working directory. The credentials file will now be stored in an application-specific hidden user folder on the operating system. On windows, this path is specified in the APPDATA environment variable as defined here which is typically 'C:\Documents and Settings\username\Application Data\pandas-gbq\' . Otherwise, credentials will be stored in '~/.config/pandas-gbq/'. If the file bigquery_credentials.dat is found in the current working directory, the file will be moved to the new location.

@tswast @jreback Please could you take a look at your earliest convenience?

@parthea parthea requested review from tswast and jreback August 21, 2017 22:15
@codecov-io
Copy link

codecov-io commented Aug 21, 2017

Codecov Report

Merging #83 into master will decrease coverage by 45.37%.
The diff coverage is 5.88%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #83       +/-   ##
===========================================
- Coverage   73.38%   28.01%   -45.38%     
===========================================
  Files           4        4               
  Lines        1563     1578       +15     
===========================================
- Hits         1147      442      -705     
- Misses        416     1136      +720
Impacted Files Coverage Δ
pandas_gbq/tests/test_gbq.py 27.76% <0%> (-54.7%) ⬇️
pandas_gbq/gbq.py 19.34% <6.25%> (-56.17%) ⬇️

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 dbfb4e9...15f24e8. Read the comment docs.

Copy link
Collaborator

@tswast tswast left a comment

Choose a reason for hiding this comment

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

Love it.

Will need a corresponding update to pandas.io.gbq to document the new parameter.

# Create a pandas_gbq directory in an application-specific hidden
# user folder on the operating system.
if not os.path.exists(config_path):
os.mkdir(config_path)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this recursive if .config doesn't exist?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great catch! os.mkdir is not recursive. Fixed.

else:
config_path = os.path.join(os.path.expanduser('~'), '.config')

config_path = os.path.join(config_path, 'pandas_gbq')
Copy link
Contributor

Choose a reason for hiding this comment

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

You know this better than I do, but is there a case for having the default path be the default gcloud path, rather than anything specific to pandas?

Copy link
Contributor Author

@parthea parthea Aug 22, 2017

Choose a reason for hiding this comment

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

This is a great question! I've given this some thought and I don't feel comfortable writing default credentials into the gcloud path in case some users don't want to have their default gcloud credentials set (and I'm also worried about colliding with gcloud). Alternatively, users could run gcloud auth application-default login if they want to configure default credentials and the default gcloud credentials should be picked up. In that case, we shouldn't hit this code path. https://github.com/pydata/pandas-gbq/blob/master/pandas_gbq/gbq.py#L222

@tswast Thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1 to not clobbering gcloud credentials. The token we save doesn't have the full Google Cloud Platform scope so that would break other applications that depend on those credentials.

If folks set up application default credentials it won't hit this code anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

users could run gcloud auth application-default login

If folks set up application default credentials it won't hit this code anyway.

👍

@parthea
Copy link
Contributor Author

parthea commented Aug 22, 2017

I've also made the changes required for pandas.io.gbq in the following branch:
pandas-dev/pandas@master...parthea:sync-with-pandas-gbq

I'll hold off on creating a PR in pandas until this is merged.

@parthea parthea force-pushed the store-credentials-in-user-directory branch 3 times, most recently from e455f18 to 0c713b6 Compare September 19, 2017 11:30
@parthea parthea force-pushed the store-credentials-in-user-directory branch from 0c713b6 to d0cd4f2 Compare September 19, 2017 11:39
@parthea
Copy link
Contributor Author

parthea commented Sep 19, 2017

@tswast I've removed the credentials_path parameter in favour of the environment variable added in #87. I've rebased the changes with the latest master and everything appears to be working as expected in my local testing. Would you be able to take another look?

@@ -1,12 +1,12 @@
Changelog
=========

0.2.1 / 2017-??-??
0.3.0 / 2017-??-??
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we should do an incremental release before 0.3.0 if we plan to have that be the one where we swap to the google-cloud-python libraries? Seems like that PR has stalled and I don't have time to rebase it either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@parthea parthea merged commit 9eb9d77 into googleapis:master Sep 19, 2017
@parthea parthea deleted the store-credentials-in-user-directory branch September 19, 2017 15:54
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.

4 participants