CDN samples dir with snippet for signing urls#1383
CDN samples dir with snippet for signing urls#1383theacodes merged 13 commits intoGoogleCloudPlatform:masterfrom
Conversation
Implements the Cloud Storage documentation on signing urls found at https://cloud.google.com/storage/docs/access-control/create-signed-urls-program
CDN samples dir with snippet for signing urls
googlebot
added
the
cla: yes
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. |
googlebot
added
cla: no
cdn/snippets.py
Outdated
|
|
||
| import argparse | ||
| import base64 | ||
| <<<<<<< HEAD |
There was a problem hiding this comment.
looks like merge conflict indicator is a leftover here
cdn/snippets.py
Outdated
|
|
||
| args = parser.parse_args() | ||
|
|
||
| args.url = "http://35.186.234.33/index.html" |
There was a problem hiding this comment.
delete these 3 to not to overwrite the args?
| hmac.new(key, url_to_sign, hashlib.sha1).digest()) | ||
|
|
||
| return '{url}&Signature={signature}'.format( | ||
| url=url_to_sign, signature=signature) |
There was a problem hiding this comment.
can you put a region tag from the beginning of imports to the end of def sign_url (so that we don't include what's below in the documentation)?
|
Sample flow and algorithm LGTM. Needs a Python approver. |
theacodes
left a comment
theacodes
left a comment
There was a problem hiding this comment.
There are no tests, not even a sanity test that just imports this. All samples must have tests.
cdn/snippets.py
Outdated
| import datetime | ||
| import hashlib | ||
| import hmac | ||
| import urlparse |
There was a problem hiding this comment.
you'll need to use six in order to get python 2/3 compatibility.
| expiration_timestamp = int((expiration_datetime - epoch).total_seconds()) | ||
|
|
||
| url_to_sign = '{url}{separator}Expires={expires}&KeyName={key_name}'.format( | ||
| url=stripped_url, |
There was a problem hiding this comment.
This is indented way too far. Indents in python are 4 spaces with a single indent level for continuations.
cdn/snippets.py
Outdated
| args = parser.parse_args() | ||
|
|
||
| if args.command == 'sign-url': | ||
| expiration_datetime = datetime.datetime.utcfromtimestamp( |
There was a problem hiding this comment.
don't put any logic here, move this into the sample.
There was a problem hiding this comment.
I think this part is correct (and is coherent with other language samples). We want sign_url to accept a language-native date type (in this case datetime).
So here, we're converting Unix epoch CLI arg to datetime to pass to sign_url. Most people would be integrating sign_url to their programs, and not this part. Hence, we should probably keep it as is.
cdn/snippets.py
Outdated
|
|
||
| Returns: | ||
| Returns the Signed URL appended with the query parameters based on the | ||
| specified configuration. Roughly of the form: |
There was a problem hiding this comment.
Roughly of the form: {url}{separator}Expires={expiration}&KeyName={key_name}&Signature={signature}
seems unnecessary. it's kind of implicit from the docs, gcloud and the way feature works in general.
Change-Id: Ib5b67ddcc761aae2731cd6c7d74ba4bd3ff59f5d
|
nox > Ran multiple sessions: BOOM |
|
CLA bot is glitchy nowadays. This is likely ready to merge. |
4 participants
Add a new directory for CDN.
Include a snippet for signing urls, as specified by the Cloud Storage documentation at https://cloud.google.com/storage/docs/access-control/create-signed-urls-program