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

Image download #9

Merged
1 commit merged into from Aug 2, 2021
Merged

Image download #9

1 commit merged into from Aug 2, 2021

Conversation

ghost
Copy link

@ghost ghost commented Jul 15, 2021

Please describe the change you are making

This change fixes #7 and adds the ability to download VM images.

This PR is based on #5

@ghost ghost marked this pull request as ready for review July 15, 2021 14:26
@ghost ghost requested a review from Gal-Zaidman July 15, 2021 14:26
@ghost ghost assigned Gal-Zaidman Jul 16, 2021
Copy link
Contributor

@Gal-Zaidman Gal-Zaidman left a comment

Choose a reason for hiding this comment

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

I looked at client_disk and client_disk_download for now.
I think that a lot of the code here can be shared with the
https://github.com/oVirt/go-ovirt-client/blob/main/client_disk_uploadimage.go

There are large parts that are basically the same between the paths, and I think that both paths (upload and download) can improve one another if they will share the same code.
I also liked that on the upload path we are removing the disk in case of failure but in download we didn't handle that...
Plus I don't see that we waited for the disk to be OK or for the job to finish

@ghost ghost marked this pull request as draft July 19, 2021 16:10
@ghost ghost assigned ghost and unassigned Gal-Zaidman Jul 20, 2021
@ghost ghost marked this pull request as ready for review July 24, 2021 07:30
@ghost
Copy link
Author

ghost commented Jul 24, 2021

@Gal-Zaidman I did the refactor, please check again.

@ghost ghost enabled auto-merge (rebase) July 24, 2021 07:41
@Gal-Zaidman
Copy link
Contributor

It's a reoccurring theme here that whenever you use the retry function,
the actual operation is inside the anonymous function which is sent to the retry function.
Like here[1]

I think you can refactor the code a bit and try to remove as many of those anonymous functions as you can, and splite it, especially the large once.
For example the waitForImageTransferReady is implemented completely within the anonymous function, I see a couple of problems with that:

  • Makes it much harder to understand and review especially for large functions which are most of them.
  • Could lead to code duplication since you will probably not look for what you need inside an anonymous function.
  • Less function documentation.

It can be split into "isImageTransferReady" which is the anonymous function and "waitForImageTransferReady" which is the retry part.

[1] https://github.com/oVirt/go-ovirt-client/pull/9/files?file-filters%5B%5D=.go#diff-d1c06aacde3ad46e36b19977671b2eb7945ec63ae785916f5b1d1ba911624f2bR179

Copy link
Contributor

@Gal-Zaidman Gal-Zaidman left a comment

Choose a reason for hiding this comment

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

I tried to do the best I can, please when you make changes commit them on a SEPARATE commit so I can understand what has change easily.

Other than that, great job 😃

@ghost ghost requested a review from Gal-Zaidman August 2, 2021 10:04
Fixes #9: Added ability to download images
Fixes #13: Disk no longer remains locked after failed image upload
Also fixed some retry bugs
@ghost ghost linked an issue Aug 2, 2021 that may be closed by this pull request
@ghost ghost merged commit 505d1f1 into oVirt:main Aug 2, 2021
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disk can remain locked after failed image upload Image download commands
1 participant