Skip to content
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

Retry HTTP downloads for 5xx errors #479

Merged
merged 7 commits into from
Nov 19, 2018
Merged

Conversation

dmacvicar
Copy link
Owner

@dmacvicar dmacvicar commented Nov 13, 2018

This PR implements what is suggested on #455

Most of the work was to do the proper testcase.

I also discovered a bug: we where not erroring at all for non-200 codes. We ignored this part of the documentation:

An error is returned if caused by client policy (such as CheckRedirect), or failure to speak HTTP (such as a network connectivity problem). A non-2xx status code doesn't cause an error.

So Import did not error, and the error we spit to the user was probably some garbage when trying to read an empty response. This is now fixed in 5fa3124

TODO

  • wait/backoff between retries

Copy link
Collaborator

@MalloZup MalloZup left a comment

Choose a reason for hiding this comment

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

looks really good and clean 🎸 i added just some minor comments.

libvirt/utils_volume.go Outdated Show resolved Hide resolved
libvirt/utils_volume.go Outdated Show resolved Hide resolved
libvirt/utils_volume.go Show resolved Hide resolved
@dmacvicar
Copy link
Owner Author

Incorporated latest feedback from @MalloZup. Waiting for tests.

@dmacvicar dmacvicar merged commit 4d1703b into master Nov 19, 2018
@dmacvicar dmacvicar changed the title WIP: Retry HTTP downloads for 5xx errors Retry HTTP downloads for 5xx errors Nov 19, 2018
@dmacvicar dmacvicar moved this from Doing to Done in terraform-provider-libvirt Nov 20, 2018
@MalloZup MalloZup deleted the dmacvicar_download_retry branch January 8, 2019 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants