-
Notifications
You must be signed in to change notification settings - Fork 458
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
Support GET-only HTTP servers for image sources #472
Conversation
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.
thx @moio looks good i will have a detailed look once i have time
thank you much for this , hope to see you again in a PR 😎
libvirt/resource_libvirt_volume.go
Outdated
@@ -143,12 +143,11 @@ func resourceLibvirtVolumeCreate(d *schema.ResourceData, meta interface{}) error | |||
|
|||
// update the image in the description, even if the file has not changed | |||
size, err := img.Size() | |||
if err != nil { | |||
return err | |||
if err == nil { |
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.
if we do so, we are not checking and returning the error of the above function
or i am missing something here? 🤔 ( i think in case we need to handle a speicif error, we should do it instead of bypassing the whole errors)
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.
At the moment there's not many other failure scenarios but I see the general point. If this is the only thing holding back merging just +1 this and I will fix it of course.
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.
@moio thanks, yop otherwise looks clean to me 🎸
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.
I like how you've pushed the size information down into the copier (most of a2e28a3). But I'd rather leave this particular hunk as it stands. To have httpImage.Size()
support endpoints that 403 HEAD
:
$ curl -sLI https://github.com/moio/sumaform-images/releases/download/4.0.0/centos7.qcow2 | grep 'HTTP\|Location'
HTTP/1.1 302 Found
Location: https://github-production-release-asset-2e65be.s3.amazonaws.com/...
HTTP/1.1 403 Forbidden
how about a "you can't make me read the body" GET
;) :
func (i *httpImage) Size() (uint64, error) {
response, err := http.Head(i.url.String())
if err != nil {
return 0, err
}
if response.StatusCode == 403 {
response, err = http.Get(i.url.String())
if err != nil {
return 0, err
}
response.Body.Close()
}
if response.StatusCode != 200 {
...
}
...
}
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.
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.
🎴
Oooh how much more I like my patch now! Thank you very much @wking! |
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.
Niiicee now
For curiosity how @wking helped here? :) i am walking at moment i will merge all pr after including the wking one. Thx for prs |
Some HTTP services, notably GitHub Releases, do not support HEAD requests to their endpoints.
terraform-provider-libvirt
will thus fail on something along the lines of:With:
I observed this phenomenon in sumaform when trying to use images built with Packer in CircleCI (sumaform-images project).
This PR removes the assumption a HEAD request will succeed. If it doesn't, it will defer getting the
Content-Length
we need for the libvirt volume definition at GET time.Test code has been adapted too, namely, the internal Web Server also sets
Content-Length
now.