Image: add basic auth support to download source image #1157
Conversation
|
Build succeeded.
|
|
@pierrchen thanks for the PR. Did you try to set a password in a URL, e.g. |
|
@kayrus It doesn't seems work for me and if the password contains an |
|
According to golang/go#24572 it should work. Have you tried to escape the |
|
See also https://golang.org/src/net/http/client.go#L239 |
|
@kayrus sorry, I made some mistake in my experiment and I can confirm (with a standalone go programe) putting user name and password in url do works (even with However, I have some questions:
And in either case, we need additional check the response status, since an non-2xx response won't cause error be return, and thus it will an error json file be uploaded as an image. Let me know how you want to proceed: Thanks, |
| if resp.StatusCode != http.StatusOK { | ||
| return "", fmt.Errorf("Error downloading image from %q, statusCode is %d", furl, resp.StatusCode) | ||
| } | ||
|
|
There was a problem hiding this comment.
@kayrus this is the required error check I mentioned.
|
@ozerovandrei would you like to have a review on this? thanks |
There was a problem hiding this comment.
Do you think provide separate username and password will be better alternative? IMO having the url and password in the URL is a bad practice and will cause some confusions especially in the Terraform user cases I will mention below.
Yes, I agree.
If we have the credential in the url, when credential changes, does Terraform consider it is a new resource and thus take actions it shouldn't?
Yes it will consider it as a new resource.
Putting credential in the url, will cause the url to be sensitive, and thus be mask out in the plan output, which make it hard for review, say if the url is correctly set.
Agree. See my comments.
And in either case, we need additional check the response status, since an non-2xx response won't cause error be return, and thus it will an error json file be uploaded as an image.
Agree.
A. Have separate user name and password config as this patch does.
B. Putting url and password in the url, but add the response error check.
Both is fine.
|
Build failed.
|
|
Not familiar with how to interpreter the acceptance test failure so not sure if it is related with the my change or it is false positive? |
|
Not entirely sure about this comments and it was marked as resolved so I didn't make any change. Let me know if I should. |
|
Build failed.
|
|
Build succeeded.
|
|
@pierrchen merged, thank you |
No description provided.