-
Notifications
You must be signed in to change notification settings - Fork 924
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add method ex_copy_image
to the GCE driver.
#258
Conversation
/cc @wrigri |
|
||
# the URL for an image can start with gs:// | ||
if url.startswith('gs://'): | ||
url = ('http://storage.googleapis.com/' + url[len('gs://'):]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The URL should be 'https:' not 'http:'
Also, the [len('gs://'):] construct seems overly complicated. Maybe something like this instead?
url.replace('gs://', 'https://storage.googleapis.com/', 1)
(I can't imagine what a url would look like that had more than one 'gs://' in it, but added the '1' to the parameters just in case.)
Looks good overall. A couple of minor things, but otherwise it seems like a good addition. |
:param name: The name of the image | ||
:type name: ``str`` | ||
|
||
:param url: The URL to the image. The URL can starts with `gs://` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/starts/start/
Thanks @wrigri, I'll update this patch later today. |
|
||
# the URL for an image can start with gs:// | ||
if url.startswith('gs://'): | ||
url.replace('gs://', 'https://storage.googleapis.com/', 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Python strings are immutable and .replace
returns a new string. This means you should do:
url = url.replace('gs://', 'https://storage.googleapis.com/', 1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides that, LGTM.
The `ex_copy_image` method allow to copy an image from a Google Cloud Storage URL in the specified project.
The
ex_copy_image
method allow to copy an image from a Google CloudStorage URL in the specified project.
I'm going to need this to implement this part in Ansible soon.
Thanks.