-
Notifications
You must be signed in to change notification settings - Fork 344
Implementing Python 3 Support #7
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
Conversation
hiranya911
commented
Mar 28, 2017
- Using the six library to implement Python 2 and 3 cross-version compatibility
- Updating initialize_app() function to use the ApplicationDefault credentials when necessary
- New test cases
- Python3 config for tox
…lemented AppDefault credential support in initialize_app()
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.
Just a couple minor comments. Also, the CI failed with this error:
============================= test session starts ==============================
platform linux2 -- Python 2.7.5, pytest-3.0.7, py-1.4.33, pluggy-0.4.0
rootdir: /home/jenkins/.jenkins/jobs/continuous-integration-admin-sdk-python-public/workspace, inifile:
collected 135 items
tests/test_app.py .....F........................................
tests/test_auth.py .................................................................................
tests/test_credentials.py ........
=================================== FAILURES ===================================
___________ TestFirebaseApp.test_non_default_app_init[refreshtoken] ____________
self = <tests.test_app.TestFirebaseApp object at 0x2ccf890>
app_credential = None
def test_non_default_app_init(self, app_credential):
> app = firebase_admin.initialize_app(app_credential, name='myApp')
tests/test_app.py:87:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
firebase_admin/__init__.py:38: in initialize_app
credential = credentials.ApplicationDefault()
firebase_admin/credentials.py:96: in __init__
self._g_credential = client.GoogleCredentials.get_application_default()
../../../../.local/lib/python2.7/site-packages/oauth2client/client.py:1264: in get_application_default
return GoogleCredentials._get_implicit_credentials()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
cls = <class 'oauth2client.client.GoogleCredentials'>
@classmethod
def _get_implicit_credentials(cls):
"""Gets credentials implicitly from the environment.
Checks environment in order of precedence:
- Environment variable GOOGLE_APPLICATION_CREDENTIALS pointing to
a file with stored credentials information.
- Stored "well known" file associated with `gcloud` command line tool.
- Google App Engine (production and testing)
- Google Compute Engine production environment.
Raises:
ApplicationDefaultCredentialsError: raised when the credentials
fail to be retrieved.
"""
# Environ checks (in order).
environ_checkers = [
cls._implicit_credentials_from_files,
cls._implicit_credentials_from_gae,
cls._implicit_credentials_from_gce,
]
for checker in environ_checkers:
credentials = checker()
if credentials is not None:
return credentials
# If no credentials, fail.
> raise ApplicationDefaultCredentialsError(ADC_HELP_MSG)
E ApplicationDefaultCredentialsError: The Application Default Credentials are not available. They are available if running in Google Compute Engine. Otherwise, the environment variable GOOGLE_APPLICATION_CREDENTIALS must be defined pointing to a file defining the credentials. See https://developers.google.com/accounts/docs/application-default-credentials for more information.
../../../../.local/lib/python2.7/site-packages/oauth2client/client.py:1254: ApplicationDefaultCredentialsError
===================== 1 failed, 134 passed in 1.45 seconds =====================
I assume we also need to update the CI to run the tests in both 2.7 and 3.3 now, right?
.github/CONTRIBUTING.md
Outdated
versions of Python as follows: | ||
|
||
``` | ||
pyenv install 2.7.6 # install Python 2.7.6 |
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: add two spaces more before the #
sign to easily distinguish the comment from the command.
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
.github/CONTRIBUTING.md
Outdated
pyenv install 3.3.0 # install Python 3.3.0 | ||
``` | ||
|
||
Refer to the [`tox.ini`](../tox.ini) file for a list of target environments that we usually test |
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.
Suggestion: drop "the Python Admin SDK on."
Side note: the official terminology we use is "Admin SDK"
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
|
||
[testenv] | ||
commands = pytest | ||
deps = | ||
pytest | ||
oauth2client | ||
six |
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.
Add a new line.
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
Made the suggested changed. The CI error was due to a bug in the code (missing return statement from a test fixture), which I have fixed. We should update our CI scripts to use tox soon. But before that I'd like to add a couple more environments to the tox configuration (at least GAE). We can setup the Jenkins server accordingly afterwards. |
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