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.

Sign up for GitHub

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

Jump to bottom

Add xenorchestra_vdi resource #225

Merged
merged 3 commits into from
Dec 21, 2022
Merged

Add xenorchestra_vdi resource #225

merged 3 commits into from
Dec 21, 2022

Conversation

ddelnano
Copy link
Collaborator

@ddelnano ddelnano commented Dec 20, 2022 •
edited

This PR resolves #193. There is still more work to be done, but opening this up to review an early version of it and receive feedback on it.

This also provides the groundwork for the XO client package to work with the new XO rest api.

Todo

  • Allow using an iso from a remote URL
  • Determine whether to support vdh VDIs on initial iteration -- decided to skip for now
  • Write resource documentation
  • Implement and test that client tls insecure settings work for both websocket and rest API
    • Unable to test that tls insecure setting works due to my environment having an expired cert but InsecureSkipVerify should be properly set.
  • Move iso file to git lfs

Testing

  • New acceptance tests pass
  • make testacc passes
ddelnano@ddelnano-desktop:~/go/src/github.com/ddelnano/terraform-provider-xenorchestra$ make ci
TF_ACC=1 ~/code/gotestsum/gotestsum --rerun-fails=3 --rerun-fails-max-failures=1000 --packages='./...' -- ./... -parallel 5 -v -count=1 -timeout=40m -sweep=true
∅  .
✓  xoa/internal (32ms)
✓  xoa (36m29.102s)

=== Skipped
=== SKIP: xoa TestAccXenorchestraVm_createVmThatInstallsFromTheNetwork (0.00s)
    resource_xenorchestra_vm_test.go:461: For now this test is not implemented. See #156 for more details

DONE 88 tests, 1 skipped in 2192.009s

4censord
4censord reviewed Dec 20, 2022
View reviewed changes
Copy link
Contributor

@4censord 4censord left a comment •
edited

Choose a reason for hiding this comment

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

We should think about moving that binary file (alpine.iso) into lfs.

As for use cases for VHD, I personally don't use that at all.
Imho it would be more useful to support qcow2 images.

client/client.go Outdated Show resolved Hide resolved
client/storage_repository.go Show resolved Hide resolved
docs/resources/vdi.md Outdated Show resolved Hide resolved
docs/resources/vdi.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
@ddelnano
Copy link
Collaborator Author

As for use cases for VHD, I personally don't use that at all.
Imho it would be more useful to support qcow2 images.

@4censord are you suggesting some type of qemu-img conversion for using VHD? Citrix Hypervisor / XCP-ng don't support qcow2 do they?

@ddelnano
Copy link
Collaborator Author

At this point the PR is ready to be merged. The later remote file support will be followed up on as mentioned in more detail here.

@olivierlambert
Copy link
Member

Until SMAPIv3 is production ready, there's no support for qcow2. And when it will, it will depends on the SR type you are in (since SMAPIv3 will allow us to create any kind of SR driver supporting any kind of "way to store" VM disks)

@4censord
Copy link
Contributor

As for use cases for VHD, I personally don't use that at all.
Imho it would be more useful to support qcow2 images.

@4censord are you suggesting some type of qemu-img conversion for using
VHD?

I didn't articulate well enough what i meant.

Afaik xcp-ng supports import of raw and vhd type images. Many
cloud-ready images from upstream are provided in qcow2 format.
Conversion is rather easy, qemu-img convert img.qcow2 img.raw, but is
still a step to be done manually.

If either Xo or the provider would support converting from
qcow2 to raw, that would eliminate this manual step.

As for use cases for vhd images: Same story of "some images might be VHD"
so if we can support them that's great.

As far as I understood the provider just needs to set the parameters
correctly, and shouldn't need to parse the vhd at all.

@ddelnano ddelnano merged commit 0a81d65 into master Dec 21, 2022
@ddelnano ddelnano deleted the add-iso-resource branch December 21, 2022 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@4censord 4censord 4censord left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature] Upload isos via terraform
3 participants
@ddelnano @olivierlambert @4censord