Issue 7413046: Add GCS sample demonstrating batch requests, pagination, and service account auth.
|
Created:
11 years, 2 months ago by yovadia Modified:
11 years, 1 month ago CC:
bigstore-team+cr_google.com, marccohen Visibility:
Public. |
DescriptionAdd GCS sample demonstrating batch requests, pagination, and service account auth.
Patch Set 1 #
Total comments: 9
Patch Set 2 : Responded to review comments, fixed service auth, added docs links, fixed service auth usage. #
Total comments: 2
Patch Set 3 : Fixed credential storage usage for service accounts. #MessagesTotal messages: 11
On 2013/02/28 20:59:00, yova... wrote: What repository is this sample being added to?
Sign in to reply to this message.
This is for the GoogleCloudPlatform <https://github.com/GoogleCloudPlatform>organization on github. It might also be worth noting that the file-transfer sample<https://code.google.com/p/google-cloud-platform-samples/source/browse/?repo=storage#git%2Ffile-transfer-json>from last year has moved there too (the Google Code repository is being deprecated), so any links you may have pointing to it should be updated to: https://github.com/GoogleCloudPlatform/storage-file-transfer-json-python On Thu, Feb 28, 2013 at 1:03 PM, <jcgregorio@google.com> wrote: > On 2013/02/28 20:59:00, yova... wrote: > > What repository is this sample being added to? > > https://codereview.appspot.**com/7413046/<https://codereview.appspot.com/7413... >
Sign in to reply to this message.
https://codereview.appspot.com/7413046/diff/1/README.md File README.md (right): https://codereview.appspot.com/7413046/diff/1/README.md#newcode14 README.md:14: (a.k.a. the GCS JSON API) is in Limited Preview, so users must request access just a nit, but it's nice to line these up like this: 1. Foo bar baz. 2. Foo bar baz. https://codereview.appspot.com/7413046/diff/1/acl_setter.py File acl_setter.py (right): https://codereview.appspot.com/7413046/diff/1/acl_setter.py#newcode21 acl_setter.py:21: bucket. If no default object ACL exists, we fabricate one using the buckets ACL buckets -> bucket's https://codereview.appspot.com/7413046/diff/1/acl_setter.py#newcode22 acl_setter.py:22: and converting all WRITER roles to READERs. converting -> convert https://codereview.appspot.com/7413046/diff/1/acl_setter.py#newcode25 acl_setter.py:25: Authenticating with a user account requires a client_secrets.json file, which How about nuke from here down in the comment and just say "See README.md" https://codereview.appspot.com/7413046/diff/1/acl_setter.py#newcode66 acl_setter.py:66: BATCH_SIZE = 1000 # Max: 1000 comment not helpful. Can you describe these three with comments above the variable instead of inline?
Sign in to reply to this message.
Thanks Jeff. I should probably note that service account auth isn't working (apparently earlier testing was succeeding using the credentials file remaining from user-authenticated requests). I'll send a new patch set when that's ready. On Thu, Feb 28, 2013 at 1:11 PM, <jterrace@google.com> wrote: > > https://codereview.appspot.**com/7413046/diff/1/README.md<https://codereview.... > File README.md (right): > > https://codereview.appspot.**com/7413046/diff/1/README.md#**newcode14<https:/... > README.md:14: (a.k.a. the GCS JSON API) is in Limited Preview, so users > must request access > just a nit, but it's nice to line these up like this: > 1. Foo bar > baz. > 2. Foo bar > baz. > > https://codereview.appspot.**com/7413046/diff/1/acl_setter.**py<https://coder... > File acl_setter.py (right): > > https://codereview.appspot.**com/7413046/diff/1/acl_setter.**py#newcode21<htt... > acl_setter.py:21: bucket. If no default object ACL exists, we fabricate > one using the buckets ACL > buckets -> bucket's > > https://codereview.appspot.**com/7413046/diff/1/acl_setter.**py#newcode22<htt... > acl_setter.py:22: and converting all WRITER roles to READERs. > converting -> convert > > https://codereview.appspot.**com/7413046/diff/1/acl_setter.**py#newcode25<htt... > acl_setter.py:25: Authenticating with a user account requires a > client_secrets.json file, which > How about nuke from here down in the comment and just say "See > README.md" > > https://codereview.appspot.**com/7413046/diff/1/acl_setter.**py#newcode66<htt... > acl_setter.py:66: BATCH_SIZE = 1000 # Max: 1000 > comment not helpful. Can you describe these three with comments above > the variable instead of inline? > > https://codereview.appspot.**com/7413046/<https://codereview.appspot.com/7413... >
Sign in to reply to this message.
Service auth is fixed; PTAL. https://codereview.appspot.com/7413046/diff/1/README.md File README.md (right): https://codereview.appspot.com/7413046/diff/1/README.md#newcode14 README.md:14: (a.k.a. the GCS JSON API) is in Limited Preview, so users must request access On 2013/02/28 21:11:17, jterrace wrote: > just a nit, but it's nice to line these up like this: > 1. Foo bar > baz. > 2. Foo bar > baz. Done. https://codereview.appspot.com/7413046/diff/1/acl_setter.py File acl_setter.py (right): https://codereview.appspot.com/7413046/diff/1/acl_setter.py#newcode22 acl_setter.py:22: and converting all WRITER roles to READERs. On 2013/02/28 21:11:17, jterrace wrote: > converting -> convert Done. https://codereview.appspot.com/7413046/diff/1/acl_setter.py#newcode25 acl_setter.py:25: Authenticating with a user account requires a client_secrets.json file, which On 2013/02/28 21:11:17, jterrace wrote: > How about nuke from here down in the comment and just say "See README.md" Done. https://codereview.appspot.com/7413046/diff/1/acl_setter.py#newcode66 acl_setter.py:66: BATCH_SIZE = 1000 # Max: 1000 On 2013/02/28 21:11:17, jterrace wrote: > comment not helpful. Can you describe these three with comments above the > variable instead of inline? Done.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
Ping?
Sign in to reply to this message.
https://codereview.appspot.com/7413046/diff/9001/acl_setter.py File acl_setter.py (right): https://codereview.appspot.com/7413046/diff/9001/acl_setter.py#newcode102 acl_setter.py:102: creds = credential_storage.get() I'm not sure this path will ever get executed. You've created a fresh creds above, and it won't be null, not will it be invalid. Instead, create the store and read the credential from it, if it returns None then instantiate the SignedJwtAssertionCredentials and call set_store() on it so it uses the Storage you created.
Sign in to reply to this message.
Thanks; PTAL. https://codereview.appspot.com/7413046/diff/9001/acl_setter.py File acl_setter.py (right): https://codereview.appspot.com/7413046/diff/9001/acl_setter.py#newcode102 acl_setter.py:102: creds = credential_storage.get() On 2013/03/04 17:22:06, jcgregorio_google wrote: > I'm not sure this path will ever get executed. You've created a fresh creds > above, and it won't be null, not will it be invalid. > > Instead, create the store and read the credential from it, if it returns None > then instantiate the SignedJwtAssertionCredentials and call set_store() on it so > it uses the Storage you created. Sorry, that was pretty silly; thanks for catching it.
Sign in to reply to this message.
LGTM On 2013/03/04 17:57:50, yova... wrote: > Thanks; PTAL. > > https://codereview.appspot.com/7413046/diff/9001/acl_setter.py > File acl_setter.py (right): > > https://codereview.appspot.com/7413046/diff/9001/acl_setter.py#newcode102 > acl_setter.py:102: creds = credential_storage.get() > On 2013/03/04 17:22:06, jcgregorio_google wrote: > > I'm not sure this path will ever get executed. You've created a fresh creds > > above, and it won't be null, not will it be invalid. > > > > Instead, create the store and read the credential from it, if it returns None > > then instantiate the SignedJwtAssertionCredentials and call set_store() on it > so > > it uses the Storage you created. > > Sorry, that was pretty silly; thanks for catching it.
Sign in to reply to this message.
|
