-
Notifications
You must be signed in to change notification settings - Fork 43
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
feat: check ovftool version #201
Conversation
e2599bb
to
843025f
Compare
843025f
to
dabfdf6
Compare
I don't enough about this change yet. But the fact that we are not enforcing a minimum version feels like it will break user workflows. Should this be a minor bump instead? |
A minor bump would make sense, yes. |
dabfdf6
to
76fe0fc
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 for the reroll @tenthirtyam!
Looking at the CheckOVFToolVersion function, I think we can iterate on it a bit more to make it more robust/simple, but aside from those comments, the code looks good!
I'll come back for another round of review when you've had a chance to address my comments.
939a252
to
082fb4b
Compare
I've refactored. Let me know what you think. I still need to run some additional tests prior to a merge and will report back. |
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.
Left a couple more suggestions regarding the check function, in the end, given the usage for both the bool
and the error
, it seems that error
alone would make more sense as the two point to the same thing: we cannot proceed as either we couldn't detect the version, or the installed one is too old.
0b49707
to
e6aec7a
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.
Left a few more suggestions on the code, having a warning regarding the version of OVFTool sounds good to me, we can continue with this approach, no problem here!
Checks the minimum recommended version of VMware Open Virtualization Format Tool ('ovftool'). Ref: #135 Signed-off-by: Ryan Johnson <[email protected]>
e6aec7a
to
6f50269
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.
LGTM! Thanks for the many rerolls @tenthirtyam, will merge ASAP
Description
Checks the minimum recommended version of VMware Open Virtualization Format Tool ('ovftool').
Sets the minimum recommended to 4.6.0 to support vSphere 8.0 and earlier.
Also improves the documentation for Export Configuration that uses
ovftool
.Testing
✅ Basic Tests
✅ Unit Tests
Reference
Closes #135