-
Notifications
You must be signed in to change notification settings - Fork 46
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
fix: isoPath as absolute path #247
Conversation
072f289
to
1c9a3d8
Compare
This modification ensures that both `isoPath` is an absolute path. Ref: #25 Signed-off-by: Ryan Johnson <[email protected]>
1c9a3d8
to
ea0000a
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 PR @tenthirtyam, looks straightforward enough to me, I'll merge this ASAP.
I do have one question/suggestion: I wonder if we can do this upstream, when registering iso_path
. If we register the iso_path as an absolute path in the first place, we can fail fast (I'm assuming this is done during Prepare
?), so the path can be corrected quickly and we don't need to call filepath.Abs
at this step.
But fundamentally this doesn't need to be done in this PR, it can also be rolled later, let me know how you want to proceed with this suggestion, and if you think it's irrelevant or you want to address it later, we can merge this today.
@lbajolet-hashicorp - Yes, indeed! I'll raise this with the desktop hypervisor engineering team and reference this pull request. Likely something that should be considered for out-of-the-box support. |
I realise my intent with |
Roger that, Lucas. I got you now and will make a note in my backlog list as the tech-debt is addressed. If we can move on with merging this one here to resolve the issue and address how it in the state next. |
Yeah that makes sense, we can proceed with the current fix and revisit later, no objection on my part! Merging now! |
Summary
This modification ensures that both
isoPath
is an absolute path.Testing
Basic:
E2E:
Notice:
==> vmware-iso.vm: https://cdimage.ubuntu.com/releases/noble/release/ubuntu-24.04.1-live-server-arm64.iso?checksum=sha256%3A5ceecb7ef5f976e8ab3fffee7871518c8e9927ec221a3bb548ee1193989e1773 => ./Downloads/a26d6549df299d1b04dcda5352d588f8b034ee1a.iso
And the VMX includes the following during the build:
Reference
Closes #25