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

Fix issues with GceAssertionCredentials in Python 3 #254

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

charlesccychen
Copy link
Contributor

This change fixes issues with GceAssertionCredentials in Python 3.

@charlesccychen
Copy link
Contributor Author

CC: @aaltay, @tvalentyn, @markflyhigh

@coveralls
Copy link

coveralls commented Feb 2, 2019

Coverage Status

Coverage remained the same at 91.697% when pulling 3f3fca1 on charlesccychen:fix-gce-py3 into 5cc8d57 on google:master.

@charlesccychen charlesccychen force-pushed the fix-gce-py3 branch 3 times, most recently from ebcc7e7 to 2917f22 Compare February 2, 2019 01:42
@charlesccychen
Copy link
Contributor Author

Hey @kevinli7, can you please review this change? Thanks!

@tvalentyn
Copy link
Contributor

There is a test failure under Python 3.5:

ERROR: testGceServiceAccounts (apitools.base.py.credentials_lib_test.CredentialsLibTest)
Traceback (most recent call last):
  File "/home/travis/build/google/apitools/apitools/base/py/credentials_lib_test.py", line 78, in testGceServiceAccounts
    scopes=None)
  File "/home/travis/build/google/apitools/apitools/base/py/credentials_lib_test.py", line 71, in _GetServiceCreds
    scopes=scopes)
  File "/home/travis/build/google/apitools/apitools/base/py/credentials_lib_test.py", line 57, in _RunGceAssertionCredentials
    scopes, **kwargs)
  File "/home/travis/build/google/apitools/apitools/base/py/credentials_lib.py", line 271, in __init__
    super(GceAssertionCredentials, self).__init__(scope=scopes, **kwds)
  File "/home/travis/build/google/apitools/.tox/py35-oauth2client1/lib/python3.5/site-packages/oauth2client/util.py", line 137, in positional_wrapper
    return wrapped(*args, **kwargs)
  File "/home/travis/build/google/apitools/.tox/py35-oauth2client1/lib/python3.5/site-packages/oauth2client/gce.py", line 58, in __init__
    self.scope = util.scopes_to_string(scope)
  File "/home/travis/build/google/apitools/.tox/py35-oauth2client1/lib/python3.5/site-packages/oauth2client/util.py", line 163, in scopes_to_string
    return ' '.join(scopes)
TypeError: sequence item 0: expected str instance, bytes found

with mock.patch.object(six.moves.urllib.request, 'build_opener',
return_value=opener,
autospec=True) as build_opener:
creds.GetServiceAccount('default')
creds.GetServiceAccount(b'default')
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this might be at fault for the Python 3.5 regression mentioned in this comment.

Maybe something like this would help remedy the regression:

if six.PY2:
    creds.GetServiceAccount(b'default')
else:
    creds.GetServiceAccount('default')

Copy link
Contributor

@catleeball catleeball Feb 20, 2019

Choose a reason for hiding this comment

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

Marking this above comment "request changes", sorry I'm still a little new to the github workflow. 🙂

with mock.patch.object(six.moves.urllib.request, 'build_opener',
return_value=opener,
autospec=True) as build_opener:
creds.GetServiceAccount('default')
creds.GetServiceAccount(b'default')
Copy link
Contributor

@catleeball catleeball Feb 20, 2019

Choose a reason for hiding this comment

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

Marking this above comment "request changes", sorry I'm still a little new to the github workflow. 🙂

@@ -352,7 +352,7 @@ def _ScopesFromMetadataServer(self, scopes):
def GetServiceAccount(self, account):
relative_url = 'instance/service-accounts'
response = _GceMetadataRequest(relative_url)
response_lines = [line.rstrip('/\n\r')
response_lines = [line.rstrip(b'/\n\r').decode('utf-8')
Copy link

Choose a reason for hiding this comment

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

Looks like #271 fixes this Py3 incompatibility, but not the one in _do_refresh_request below.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants