Skip to content

storage: Add Boto3 List Objects in GCS#2041

Merged
frankyn merged 11 commits intomasterfrom
boto3-list-objects
Apr 3, 2019
Merged

storage: Add Boto3 List Objects in GCS#2041
frankyn merged 11 commits intomasterfrom
boto3-list-objects

Conversation

@frankyn
Copy link
Copy Markdown
Contributor

@frankyn frankyn commented Mar 14, 2019

Hi @engelke,

Sample to list objects in GCS using S3 SDK.

@frankyn frankyn requested a review from engelke March 14, 2019 00:15
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 14, 2019
@frankyn frankyn changed the title storage: Add Boto3 List Objects workaround using GCS API storage: Add Boto3 List Objects in GCS Mar 14, 2019
@frankyn frankyn added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Mar 14, 2019
@frankyn frankyn added kokoro:force-run Add this label to force Kokoro to re-run the tests. and removed kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Apr 2, 2019

# Print object names
print("Objects:")
for bucket in response["Contents"]:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since this is listing objects in a bucket, it would be clearer for this to say for object in ... instead of for bucket in .... (Or another word for object, just not bucket.)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Or "blobs". That's a word I like.

@@ -78,8 +78,10 @@ def generate_signed_url(service_account_file, bucket_name, object_name,
ordered_headers = collections.OrderedDict(sorted(headers.items()))
for k, v in ordered_headers.items():
lower_k = str(k).lower()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lower_k and strip_v? name and value would be better variable name.

strip_v = str(v).lower()
strip_v = v
canonical_headers += '{}:{}\n'.format(lower_k, strip_v)
canonical_headers = "host:storage.googleapis.com\nx-goog-encryption-algorithm:AES56\n" # "x-goog-encryption-key:AAryxNglNkXQY0Wa+h9+7BLSFMhCzPo22MtXUWjOBbI=\nx-goog-encryption-key-sha256:QlCdVONb17U1aCTAjrFvMbnxW/Oul8VAvnG1875WJ3k=\n"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why is this comment here?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be += instead of =? Otherwise earlier values are lost.

@@ -88,6 +90,7 @@ def generate_signed_url(service_account_file, bucket_name, object_name,
lower_k = str(k).lower()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Similar issue with names. Also, these aren't signed_headers, they're names of the headers that need to be signed, so a different name would be more clear. And no need to use items() since you just want the keys, no values.

lower_k = str(k).lower()
signed_headers += '{};'.format(lower_k)
signed_headers = signed_headers[:-1] # remove trailing ';'
signed_headers = 'host;x-goog-encryption-algorithm' #;x-goog-encryption-key;x-goog-encryption-key-sha256'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Again, why the comment? If not used, shouldn't they be removed?

@@ -105,7 +108,7 @@ def generate_signed_url(service_account_file, bucket_name, object_name,
for k, v in ordered_query_parameters.items():
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Variable names. Something like safe_name, safe_value would be clearer than encoded_k and encoded_v

parser.add_argument('expiration', help='Expiration Time.')

args = parser.parse_args()
headers = {'X-Goog-Encryption-Key': 'AAryxNglNkXQY0Wa+h9+7BLSFMhCzPo22MtXUWjOBbI=',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We don't usually include secrets in samples, instead using a placeholder or getting them from command line arguments or environment variables.

@frankyn frankyn removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Apr 3, 2019
Copy link
Copy Markdown
Contributor

@engelke engelke left a comment

Choose a reason for hiding this comment

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

LGTM

@frankyn frankyn merged commit 2d4ab38 into master Apr 3, 2019
@frankyn frankyn deleted the boto3-list-objects branch April 3, 2019 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants