-
Notifications
You must be signed in to change notification settings - Fork 675
defined a common interface for nativeimgutil & qemuimgutil #3650
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
base: master
Are you sure you want to change the base?
Conversation
fe7a833
to
6bc9e97
Compare
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.
Pull Request Overview
This PR refactors image utility handling by defining a common interface for both native and QEMU image utilities, allowing the code to use a unified API for disk image operations. Key changes include replacing direct calls to nativeimgutil and qemuimgutil with calls to the common imgutil package, introducing factory and proxy implementations, and updating tests and error handling accordingly.
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
pkg/vz/vm_darwin.go | Replaced nativeimgutil with common imgutil for raw disk conversion. |
pkg/vz/disk.go | Updated disk creation and resizing to use the new imgutil interface. |
pkg/qemu/qemu.go | Refactored disk info retrieval and validation to use the proxy interface. |
pkg/instance/start.go | Updated disk conversion logic to use the common image utility. |
pkg/imgutil/qemuimgutil/* | Renamed helper functions to unexported versions and updated tests. |
pkg/imgutil/nativeimgutil/* | Adjusted naming of internal functions and updated tests accordingly. |
pkg/imgutil/proxyimgutil/* | Introduced proxy implementation to abstract native/qemu differences. |
pkg/imgutil/factory.go | Added a factory function to return the correct image util based on format. |
cmd/limactl/disk.go | Updated command-line disk actions to use the new unified API. |
Comments suppressed due to low confidence (2)
pkg/qemu/qemu.go:95
- [nitpick] If multiple image info calls occur within this function, consider caching the result of NewImageUtil to avoid redundant initialization overhead.
_, infoProvider, err := imgutil.NewImageUtil("qcow2")
pkg/instance/start.go:429
- [nitpick] If prepareDiffDisk is called frequently, consider caching the image util instance to reduce the cost of repetitive initialization.
diskUtil, _, err := imgutil.NewImageUtil(format)
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.
Doesn't seem to follow #3518
bebc163
to
dabff97
Compare
The commits can be squashed |
d4972a3
to
590ae10
Compare
445ec48
to
e743134
Compare
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.
Thanks
cmd/limactl/disk.go
Outdated
} else { | ||
err = imgutil.ResizeDisk(dataDisk, disk.Format, int(diskSize)) | ||
} | ||
diskUtil := proxyimgutil.NewProxyImageUtil() |
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.
Can we improve the naming?
Currently, there are two issues:
- Duplication in
proxyimgutil
andNewProxyImageUtil
. NewProxyImageUtil
returns adiskUtil
, not animageUtil
, which can be confusing for someone reading the code.
diskUtil := proxyimgutil.NewProxyImageUtil() | |
diskUtil := proxyimgutil.NewDiskUtil() |
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.
Confusing to have both “imgutil” and “diskutil”
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.
Agree.
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.
Let's merge this PR and improve in the next PRs.
Signed-off-by: Praful Khanduri <[email protected]>
fixes : #3518
Define the common interface for qemuimgutil and nativeimgutil