Issue 7067054: Modify oauth2client.multistore_file to store and retrieve credentials using an arbitrary key
|
Created:
11 years, 8 months ago by markpell Modified:
11 years, 7 months ago CC:
google-api-python-client_googlegroups.com Visibility:
Public. |
Patch Set 1 : "Adding new storage key API and associated tests" #
Total comments: 6
Patch Set 2 : "CR feedback" #Patch Set 3 : Some more CR feedback #
Total comments: 4
Patch Set 4 : Fixing spacing #
MessagesTotal messages: 17
Hey guys, finally got around to finishing the changes we talked about for the multistore file. Please have a look and let me know what you think. The changes maintain backward compatibility for current applications and expose a new API for using an arbitrary key for credential storage and retrieval.
Sign in to reply to this message.
Somethings been lost in the upload process, I only see one file in Patch Set 2. Be sure to use the --rev parameter of upload-diffs.py. On 2013/01/08 18:06:41, markpell wrote: > Hey guys, finally got around to finishing the changes we talked about for the > multistore file. Please have a look and let me know what you think. The > changes maintain backward compatibility for current applications and expose a > new API for using an arbitrary key for credential storage and retrieval.
Sign in to reply to this message.
Yeah I was afraid I didn't do it right. The change is in 2 commits that are separated by a pull merge. I did the upload twice for the 2 different revisions so the complete set of changes are both patch set 1 and patch set 2. Is there a way to upload both the commits into a single patch set? -Mark On Tue, Jan 8, 2013 at 1:09 PM, <jcgregorio@google.com> wrote: > Somethings been lost in the upload process, I only see one file in Patch > Set 2. Be sure to use the --rev parameter of upload-diffs.py. > > > On 2013/01/08 18:06:41, markpell wrote: > >> Hey guys, finally got around to finishing the changes we talked about >> > for the > >> multistore file. Please have a look and let me know what you think. >> > The > >> changes maintain backward compatibility for current applications and >> > expose a > >> new API for using an arbitrary key for credential storage and >> > retrieval. > > > > https://codereview.appspot.**com/7067054/<https://codereview.appspot.com/7067... > -- Thanks, -Mark
Sign in to reply to this message.
Yes, $ python upload-diffs.py --rev 1111 --issue 7067054 where the 1111 is replaced with the mercurial revision number that your changes come from. On Tue, Jan 8, 2013 at 1:12 PM, Mark Pellegrini <markpell@google.com> wrote: > Yeah I was afraid I didn't do it right. The change is in 2 commits that are > separated by a pull merge. I did the upload twice for the 2 different > revisions so the complete set of changes are both patch set 1 and patch set > 2. Is there a way to upload both the commits into a single patch set? > > -Mark > > > On Tue, Jan 8, 2013 at 1:09 PM, <jcgregorio@google.com> wrote: >> >> Somethings been lost in the upload process, I only see one file in Patch >> Set 2. Be sure to use the --rev parameter of upload-diffs.py. >> >> >> On 2013/01/08 18:06:41, markpell wrote: >>> >>> Hey guys, finally got around to finishing the changes we talked about >> >> for the >>> >>> multistore file. Please have a look and let me know what you think. >> >> The >>> >>> changes maintain backward compatibility for current applications and >> >> expose a >>> >>> new API for using an arbitrary key for credential storage and >> >> retrieval. >> >> >> >> https://codereview.appspot.com/7067054/ > > > > > -- > Thanks, > -Mark
Sign in to reply to this message.
Ok, I think I have the diffs sorted out correctly. Please have another look. -Mark
Sign in to reply to this message.
https://codereview.appspot.com/7067054/diff/8001/oauth2client/multistore_file.py File oauth2client/multistore_file.py (right): https://codereview.appspot.com/7067054/diff/8001/oauth2client/multistore_file... oauth2client/multistore_file.py:84: @util.positional(2) https://codereview.appspot.com/7067054/diff/8001/oauth2client/multistore_file... oauth2client/multistore_file.py:85: def get_credential_storage_custom_key(filename, key_dict, warn_on_readonly=True): 80 chars https://codereview.appspot.com/7067054/diff/8001/oauth2client/util.py File oauth2client/util.py (right): https://codereview.appspot.com/7067054/diff/8001/oauth2client/util.py#newcode165 oauth2client/util.py:165: def tuple_key_to_dict(key): Don't need a function for this, dict(tuple) does the right thing.
Sign in to reply to this message.
Thanks for the quick feedback. Addressed all comments. https://codereview.appspot.com/7067054/diff/8001/oauth2client/multistore_file.py File oauth2client/multistore_file.py (right): https://codereview.appspot.com/7067054/diff/8001/oauth2client/multistore_file... oauth2client/multistore_file.py:84: On 2013/01/09 14:22:22, jcgregorio_google wrote: > @util.positional(2) Done. https://codereview.appspot.com/7067054/diff/8001/oauth2client/multistore_file... oauth2client/multistore_file.py:85: def get_credential_storage_custom_key(filename, key_dict, warn_on_readonly=True): On 2013/01/09 14:22:22, jcgregorio_google wrote: > 80 chars Done. https://codereview.appspot.com/7067054/diff/8001/oauth2client/util.py File oauth2client/util.py (right): https://codereview.appspot.com/7067054/diff/8001/oauth2client/util.py#newcode165 oauth2client/util.py:165: def tuple_key_to_dict(key): On 2013/01/09 14:22:22, jcgregorio_google wrote: > Don't need a function for this, dict(tuple) does the right thing. Done.
Sign in to reply to this message.
LGTM, but wait for jbeda to review before committing. -joe On 2013/01/09 16:00:24, markpell wrote: > Thanks for the quick feedback. Addressed all comments. > > https://codereview.appspot.com/7067054/diff/8001/oauth2client/multistore_file.py > File oauth2client/multistore_file.py (right): > > https://codereview.appspot.com/7067054/diff/8001/oauth2client/multistore_file... > oauth2client/multistore_file.py:84: > On 2013/01/09 14:22:22, jcgregorio_google wrote: > > @util.positional(2) > > Done. > > https://codereview.appspot.com/7067054/diff/8001/oauth2client/multistore_file... > oauth2client/multistore_file.py:85: def > get_credential_storage_custom_key(filename, key_dict, warn_on_readonly=True): > On 2013/01/09 14:22:22, jcgregorio_google wrote: > > 80 chars > > Done. > > https://codereview.appspot.com/7067054/diff/8001/oauth2client/util.py > File oauth2client/util.py (right): > > https://codereview.appspot.com/7067054/diff/8001/oauth2client/util.py#newcode165 > oauth2client/util.py:165: def tuple_key_to_dict(key): > On 2013/01/09 14:22:22, jcgregorio_google wrote: > > Don't need a function for this, dict(tuple) does the right thing. > > Done.
Sign in to reply to this message.
Based on some CR feedback from rdayal, I added another simple wrapper function for using a simple string as a key rather than a dictionary. Also added a corresponding test. Please have a look when you get a chance.
Sign in to reply to this message.
https://codereview.appspot.com/7067054/diff/15005/oauth2client/multistore_fil... File oauth2client/multistore_file.py (right): https://codereview.appspot.com/7067054/diff/15005/oauth2client/multistore_fil... oauth2client/multistore_file.py:119: key_dict: A dictionary to use as the key for storing this credential. There One space between sentences to keep it consistent with the rest of the code.
Sign in to reply to this message.
On 2013/01/14 16:39:34, jcgregorio_google wrote: > https://codereview.appspot.com/7067054/diff/15005/oauth2client/multistore_fil... > File oauth2client/multistore_file.py (right): > > https://codereview.appspot.com/7067054/diff/15005/oauth2client/multistore_fil... > oauth2client/multistore_file.py:119: key_dict: A dictionary to use as the key > for storing this credential. There > One space between sentences to keep it consistent with the rest of the code. LGTM! Thanks for running it by me. I'll make sure the gcutil guys know this is coming.
Sign in to reply to this message.
Thanks guys. Since I'm not a committer on this project, can one of you land this CL for me? Also, (no real rush) but is there any idea of when this might make a release?
Sign in to reply to this message.
I'll leave it to Joe -- he is the guy. On Wed, Jan 23, 2013 at 3:04 PM, <markpell@google.com> wrote: > Thanks guys. Since I'm not a committer on this project, can one of you > land this CL for me? Also, (no real rush) but is there any idea of when > this might make a release? > > https://codereview.appspot.**com/7067054/<https://codereview.appspot.com/7067... >
Sign in to reply to this message.
https://codereview.appspot.com/7067054/diff/15005/oauth2client/multistore_fil... File oauth2client/multistore_file.py (right): https://codereview.appspot.com/7067054/diff/15005/oauth2client/multistore_fil... oauth2client/multistore_file.py:119: key_dict: A dictionary to use as the key for storing this credential. There This still needs to be addressed, then I'll commit it. On 2013/01/14 16:39:34, jcgregorio_google wrote: > One space between sentences to keep it consistent with the rest of the code.
Sign in to reply to this message.
https://codereview.appspot.com/7067054/diff/15005/oauth2client/multistore_fil... File oauth2client/multistore_file.py (right): https://codereview.appspot.com/7067054/diff/15005/oauth2client/multistore_fil... oauth2client/multistore_file.py:119: key_dict: A dictionary to use as the key for storing this credential. There On 2013/01/14 16:39:34, jcgregorio_google wrote: > One space between sentences to keep it consistent with the rest of the code. Done. https://codereview.appspot.com/7067054/diff/15005/oauth2client/multistore_fil... oauth2client/multistore_file.py:119: key_dict: A dictionary to use as the key for storing this credential. There This was addressed in patch set #4. Sorry, I forgot to close the comment. On 2013/01/24 01:56:30, jcgregorio_google wrote: > This still needs to be addressed, then I'll commit it. > > On 2013/01/14 16:39:34, jcgregorio_google wrote: > > One space between sentences to keep it consistent with the rest of the code. >
Sign in to reply to this message.
Committed in https://code.google.com/p/google-api-python-client/source/detail?r=b4888423b1... On 2013/01/24 15:36:37, markpell wrote: > https://codereview.appspot.com/7067054/diff/15005/oauth2client/multistore_fil... > File oauth2client/multistore_file.py (right): > > https://codereview.appspot.com/7067054/diff/15005/oauth2client/multistore_fil... > oauth2client/multistore_file.py:119: key_dict: A dictionary to use as the key > for storing this credential. There > On 2013/01/14 16:39:34, jcgregorio_google wrote: > > One space between sentences to keep it consistent with the rest of the code. > > Done. > > https://codereview.appspot.com/7067054/diff/15005/oauth2client/multistore_fil... > oauth2client/multistore_file.py:119: key_dict: A dictionary to use as the key > for storing this credential. There > This was addressed in patch set #4. Sorry, I forgot to close the comment. > > On 2013/01/24 01:56:30, jcgregorio_google wrote: > > This still needs to be addressed, then I'll commit it. > > > > On 2013/01/14 16:39:34, jcgregorio_google wrote: > > > One space between sentences to keep it consistent with the rest of the code. > >
Sign in to reply to this message.
Thanks Joe! On Thu, Jan 24, 2013 at 3:53 PM, <jcgregorio@google.com> wrote: > Committed in > https://code.google.com/p/**google-api-python-client/**source/detail?r=** > b4888423b1d3b890ed8300469232f8**a3ed133bf6<https://code.google.com/p/google-api-python-client/source/detail?r=b4888423b1d3b890ed8300469232f8a3ed133bf6> > > > On 2013/01/24 15:36:37, markpell wrote: > > https://codereview.appspot.**com/7067054/diff/15005/** > oauth2client/multistore_file.**py<https://codereview.appspot.com/7067054/diff/15005/oauth2client/multistore_file.py> > >> File oauth2client/multistore_file.**py (right): >> > > > https://codereview.appspot.**com/7067054/diff/15005/** > oauth2client/multistore_file.**py#newcode119<https://codereview.appspot.com/7067054/diff/15005/oauth2client/multistore_file.py#newcode119> > >> oauth2client/multistore_file.**py:119: key_dict: A dictionary to use as >> > the key > >> for storing this credential. There >> On 2013/01/14 16:39:34, jcgregorio_google wrote: >> > One space between sentences to keep it consistent with the rest of >> > the code. > > Done. >> > > > https://codereview.appspot.**com/7067054/diff/15005/** > oauth2client/multistore_file.**py#newcode119<https://codereview.appspot.com/7067054/diff/15005/oauth2client/multistore_file.py#newcode119> > >> oauth2client/multistore_file.**py:119: key_dict: A dictionary to use as >> > the key > >> for storing this credential. There >> This was addressed in patch set #4. Sorry, I forgot to close the >> > comment. > > On 2013/01/24 01:56:30, jcgregorio_google wrote: >> > This still needs to be addressed, then I'll commit it. >> > >> > On 2013/01/14 16:39:34, jcgregorio_google wrote: >> > > One space between sentences to keep it consistent with the rest of >> > the code. > >> > >> > > > https://codereview.appspot.**com/7067054/<https://codereview.appspot.com/7067... > -- Thanks, -Mark
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
