Remove bogus log messages and add cache for 2LO #7

bochunz · 2016-08-03T18:41:20Z

Remove the bogus log messages generated by the old multistore_file module by switching to multiprocess_file_storage module.
Add cache for getting 2LO token by using the same multiprocess_file_storage module.

coveralls · 2016-08-03T18:42:54Z

Coverage decreased (-0.2%) to 99.624% when pulling 460c516 on bochunz:log into 3b8c369 on google:master.

craigcitro · 2016-08-03T18:44:22Z

In general, it's better to split these into separate pull requests, since there are two separate things happening.

We also want a longer description:

Mention issues being fixed by number (so that submitting this closes those issues with a reference)
Some details about what's happening in each of the fixes

Someone who comes along in the future will not be able to figure out much based on the description here.

craigcitro · 2016-08-03T18:45:01Z

oauth2l/__init__.py

    return credentials

+def _GetCredentialForServiceAccount(json_keyfile, scopes, 
+    credentials_filename=None):


just curious: did this not fit on the line above?

coveralls · 2016-08-03T20:11:57Z

Coverage decreased (-0.2%) to 99.624% when pulling 02271a9 on bochunz:log into 3b8c369 on google:master.

coveralls · 2016-08-03T20:15:19Z

Coverage decreased (-0.2%) to 99.623% when pulling a923bd0 on bochunz:log into 3b8c369 on google:master.

bochunz · 2016-08-03T20:16:49Z

@wora @craigcitro PTAL

wora · 2016-08-03T21:41:29Z

Please bump up the version.

bochunz · 2016-08-03T22:29:36Z

So we bump up from 0.9.0 --> 0.9.1?

coveralls · 2016-08-04T18:44:30Z

Coverage decreased (-0.2%) to 99.623% when pulling ab0cecb on bochunz:log into 3b8c369 on google:master.

craigcitro · 2016-08-04T18:47:49Z

I'm going to get back to this later today -- but do not include a version bump in this PR. It's already got too many things going on. 😉

bochunz · 2016-08-04T20:32:12Z

Thanks Craig

coveralls · 2016-08-04T20:38:09Z

Coverage decreased (-0.2%) to 99.623% when pulling f9f8214 on bochunz:log into 3b8c369 on google:master.

wora · 2016-08-05T01:41:46Z

oauth2l/__init__.py

+    credentials_filename = _GetCredentialsFilename(credentials_filename)
+    storage_key = '3LO-{}-{}-{}'.format(
        client_info['client_id'],
        client_info['user_agent'],


Why add user agent to the key?

I thought there are some reason to put user agent so I didn't remove it. Removed.

wora · 2016-08-05T01:46:50Z

If I have a cached token of scopes A,B, should I use it for request that only needs scope A?

bochunz · 2016-08-05T04:24:31Z

As for the token scope subset problem, I think we should keep it simple. i.e. get token with the exact scopes. Otherwise this tool will have inconsistent outputs: when user ask for token with scope A, he/she may get token with any other scope.

coveralls · 2016-08-05T04:34:48Z

Coverage decreased (-0.2%) to 99.623% when pulling 9049355 on bochunz:log into 3b8c369 on google:master.

craigcitro · 2016-08-05T06:23:32Z

oauth2l/__init__.py

+                                           ' '.join(sorted(scopes)))
+    credentials = credential_store.get()
+    if credentials is None or credentials.invalid:
+        credentials = service_account.ServiceAccountCredentials \


silly: the \ is awkward; if nothing else, you could do

credentials = ( service_account.ServiceAccountCredentials.form_json_keyfile_dict(

or just make an alias, eg

ServiceAccountCredentials = service_account.ServiceAccountCredentials credentials = ServiceAccountCredentials.from_json_keyfile_dict(...)

(or some other variation)

craigcitro · 2016-08-05T06:27:55Z

LGTM

I think you can also tell the coverage tool to not worry with just

 # pragma: NO COVER

which I would say you should go ahead and do here or in a related PR.

craigcitro · 2016-08-05T06:28:04Z

@wora letting you merge if you're happy with it.

coveralls · 2016-08-05T16:47:11Z

Coverage increased (+0.02%) to 99.811% when pulling 08fb81d on bochunz:log into 3b8c369 on google:master.

wora · 2016-08-05T21:52:02Z

oauth2l/__init__.py

+
+def _GetCredentialStore(credentials_filename, key_id, scopes):
+    credentials_filename = _GetCredentialsFilename(credentials_filename)
+    storage_key = '{}-{}'.format(key_id, scopes)


is not very safe. # is better.

_GetCredentialStore takes sorted and joined scopes as one string

Actually I took the "-" from this example https://github.com/google/oauth2client/blob/master/oauth2client/contrib/multiprocess_file_storage.py#L60

Changed to #

wora · 2016-08-05T21:59:59Z

oauth2l/oauth2l_test.py

+import oauth2client.contrib.multiprocess_file_storage
 import oauth2client.service_account
 import oauth2client.tools
 import six


Can we get dependency on six?

wora · 2016-08-05T22:00:30Z

Couple of minor questions.

1. Remove the bogus log messages generated by the old multistore_file module by switching to multiprocess_file_storage module. 2. Add cache for getting 2LO token by using the same multiprocess_file_storage module.

wora · 2016-08-05T22:17:01Z

LGTM

coveralls · 2016-08-05T22:17:27Z

Coverage increased (+0.02%) to 99.811% when pulling 2262d7f on bochunz:log into 3b8c369 on google:master.

googlebot added the cla: yes label Aug 3, 2016

craigcitro reviewed Aug 3, 2016
View reviewed changes

bochunz force-pushed the log branch from 460c516 to 02271a9 Compare August 3, 2016 20:09

bochunz force-pushed the log branch from 02271a9 to a923bd0 Compare August 3, 2016 20:13

bochunz force-pushed the log branch from a923bd0 to ab0cecb Compare August 4, 2016 18:42

bochunz force-pushed the log branch from ab0cecb to f9f8214 Compare August 4, 2016 20:31

wora reviewed Aug 5, 2016
View reviewed changes

bochunz force-pushed the log branch from f9f8214 to 9049355 Compare August 5, 2016 04:33

craigcitro reviewed Aug 5, 2016
View reviewed changes

bochunz force-pushed the log branch from 9049355 to 08fb81d Compare August 5, 2016 16:45

wora reviewed Aug 5, 2016
View reviewed changes

Remove bogus log messages and add cache for 2LO

2262d7f

1. Remove the bogus log messages generated by the old multistore_file module by switching to multiprocess_file_storage module. 2. Add cache for getting 2LO token by using the same multiprocess_file_storage module.

bochunz force-pushed the log branch from 08fb81d to 2262d7f Compare August 5, 2016 22:16

wora merged commit afc56ef into google:master Aug 5, 2016

bochunz deleted the log branch August 24, 2016 01:21

Remove bogus log messages and add cache for 2LO #7

Remove bogus log messages and add cache for 2LO #7

Uh oh!

Conversation

bochunz commented Aug 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Aug 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

craigcitro commented Aug 3, 2016

Uh oh!

craigcitro Aug 3, 2016

Choose a reason for hiding this comment

Uh oh!

bochunz Aug 3, 2016

Choose a reason for hiding this comment

Uh oh!

coveralls commented Aug 3, 2016

Uh oh!

coveralls commented Aug 3, 2016

Uh oh!

bochunz commented Aug 3, 2016

Uh oh!

wora commented Aug 3, 2016

Uh oh!

bochunz commented Aug 3, 2016

Uh oh!

coveralls commented Aug 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

craigcitro commented Aug 4, 2016

Uh oh!

bochunz commented Aug 4, 2016

Uh oh!

coveralls commented Aug 4, 2016

Uh oh!

wora Aug 5, 2016

Choose a reason for hiding this comment

Uh oh!

bochunz Aug 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wora commented Aug 5, 2016

Uh oh!

bochunz commented Aug 5, 2016

Uh oh!

coveralls commented Aug 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

craigcitro Aug 5, 2016

Choose a reason for hiding this comment

Uh oh!

bochunz Aug 5, 2016

Choose a reason for hiding this comment

Uh oh!

craigcitro commented Aug 5, 2016

Uh oh!

craigcitro commented Aug 5, 2016

Uh oh!

coveralls commented Aug 5, 2016

Uh oh!

wora Aug 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bochunz Aug 5, 2016

Choose a reason for hiding this comment

Uh oh!

bochunz Aug 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wora Aug 5, 2016

Choose a reason for hiding this comment

Uh oh!

wora commented Aug 5, 2016

Uh oh!

wora commented Aug 5, 2016

bochunz commented Aug 3, 2016 •

edited

Loading

coveralls commented Aug 3, 2016 •

edited

Loading

coveralls commented Aug 4, 2016 •

edited

Loading

bochunz Aug 5, 2016 •

edited

Loading

coveralls commented Aug 5, 2016 •

edited

Loading

wora Aug 5, 2016 •

edited

Loading

bochunz Aug 5, 2016 •

edited

Loading

coveralls commented Aug 5, 2016 •

edited

Loading