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

feat(vm): add support for boot_order argument for VM #219

Merged
merged 3 commits into from
Apr 7, 2023
Merged

Conversation

HenriAW
Copy link
Contributor

@HenriAW HenriAW commented Jan 24, 2023

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Closes #214

boot_order and boot_disk flags added to the VM resource. These are passed to the boot and bootdisk parameters in the Proxmox VE API described here.

@product-auto-label product-auto-label bot added size: s 📖 documentation Improvements or additions to documentation labels Jan 24, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jan 24, 2023

Codecov Report

Base: 11.20% // Head: 11.29% // Increases project coverage by +0.09% 🎉

Coverage data is based on head (e1cf589) compared to base (e0864c0).
Patch coverage: 80.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #219      +/-   ##
==========================================
+ Coverage   11.20%   11.29%   +0.09%     
==========================================
  Files          54       54              
  Lines       11726    11738      +12     
==========================================
+ Hits         1314     1326      +12     
  Misses      10386    10386              
  Partials       26       26              
Impacted Files Coverage Δ
proxmoxtf/resource_virtual_environment_vm.go 2.31% <80.00%> (+0.40%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@bpg
Copy link
Owner

bpg commented Jan 24, 2023

Hey @HenriAW, thanks for the PR!
I'll take a closer look later this week.

Copy link
Owner

@bpg bpg left a comment

Choose a reason for hiding this comment

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

Hey @HenriAW ! 👋🏻
Thanks again for the PR!
I'd like to make a few changes to align it better with the PVE API and the existing code, please see my comments.

If you have any questions, feel free to toss them in, I'll be happy to discuss 🙂

Comment on lines 251 to 254
Type: schema.TypeString,
Description: "Specify the guest boot order",
Optional: true,
Default: dvResourceVirtualEnvironmentVMBootOrder,
Copy link
Owner

Choose a reason for hiding this comment

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

Per documentation, the boot order is specified as a comma-separate list, so I'd like to reflect that in the schema as well, i.e. to have something like:

Suggested change
Type: schema.TypeString,
Description: "Specify the guest boot order",
Optional: true,
Default: dvResourceVirtualEnvironmentVMBootOrder,
Type: schema.TypeList,
Description: "Specify the guest boot order",
Optional: true,
Elem: &schema.Schema{Type: schema.TypeString},
Default: dvResourceVirtualEnvironmentVMBootOrder,

This would also require adding a logic to convert from a list to a string and back. VM tags can be taken as an example as they are processed in a similar way.

Copy link
Owner

Choose a reason for hiding this comment

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

You would also need to read the boot order back from PVE to populate the current state. It is usually done in resourceVirtualEnvironmentVMReadPrimitiveValues for values that referenced as primitives in the API model. Also keep in mind that we need to read from the "order" property of "boot" value returned by the PVE API.

@@ -120,6 +120,8 @@ output "ubuntu_vm_public_key" {
- `bios` - (Optional) The BIOS implementation (defaults to `seabios`).
- `ovmf` - OVMF (UEFI).
- `seabios` - SeaBIOS.
- `boot_order` - (Optional) Specify the guest boot order (defaults to `c`).
Copy link
Owner

Choose a reason for hiding this comment

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

I think you're referring to the "legacy" parameter of the boot order which specifies a disk by its letter. We should probably switch to the device names: https://pve.proxmox.com/pve-docs/chapter-qm.html#qm_options

@bpg bpg added size/S and removed size: s labels Apr 4, 2023
@pull-request-size pull-request-size bot added size/L and removed size/S labels Apr 7, 2023
@bpg bpg changed the title feat(vm): add support for 'boot_order and boot_disk flags for VM feat(vm): add support for boot_order ccccccjejlebcblviluhhcektiegvbhvgcldutugtrrdboot_disk flags for VM Apr 7, 2023
@bpg bpg changed the title feat(vm): add support for boot_order ccccccjejlebcblviluhhcektiegvbhvgcldutugtrrdboot_disk flags for VM feat(vm): add support for boot_order argument for VM Apr 7, 2023
@bpg bpg merged commit f9e263a into bpg:main Apr 7, 2023
@ghost ghost mentioned this pull request Apr 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📖 documentation Improvements or additions to documentation size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable boot order to be manually set
3 participants