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

Overhaul qemu disks merge #255

Merged
merged 122 commits into from
May 14, 2023

Conversation

Tinyblargon
Copy link
Collaborator

This is a new implementation of the disk structure discussed in #187 and deprecates the old implementation.

Code reworked:

  • Split the logic for converting between Proxmox API and our ConfigQemu data structure to its own function to improve test-ability.
  • Moved the default for ConfigQemu to its own function to improve test-ability.
  • Removed errors from functions that would always return nil.
  • Added TODOs to ConfigQemu
  • func NewConfigQemuFromApi( will no longer return the deprecated parts of type ConfigQemu

Features added:

  • Increased the CRUD logic to make it easier for projects to implement this library (func (newConfig ConfigQemu) setAdvanced).
  • Added support for all disk types:
    • cloudinit
    • cdrom
      • iso file
      • pass-through
      • none
    • virtual disk
    • disk pass-through
  • Disks from linked clones are also supported.
  • Added Validation function for disk func (storages QemuStorages) Validate()
  • Added func MoveQemuDisk to be a full implementation for disk migration, storage/disk-type change.
  • Added type ConfigQemu.Iso to keep the spirit of the old implementation. This feature feels out of place in this code and should be delegated to implementations like the Terraform provider.
  • Disk won't be updated when nothing has changed, this would previously makes an unnecessary pending change in Proxmox.
  • Add 6000 lines of test code.

Features deprecated:

  • func (config ConfigQemu) CreateVm
  • func (config ConfigQemu) UpdateConfig
  • func (c *Client) MoveQemuDisk
  • type ConfigQemu.QemuDisks
  • type ConfigQemu.QemuIso

All the deprecated functions still work and don't make use of the new disk implementation (bugs included).

Reasoning for moving the CRUD logic to this library:

  • Implementations like the Terraform provider will now require less CRUD logic.
  • This change makes more of the CRUD logic internal to the library.
  • Due to the logic being internal it makes it easier to test and make changes when bug are found.
  • Changes will have less impact due to things like input/output stayng the same.

Currently I am working on a Terraform implementation.

this allows for the user to manually provide the current config or let the library provide the config
@mleone87 mleone87 merged commit 8da8251 into Telmate:master May 14, 2023
mabeett added a commit to mabeett/packer-plugin-proxmox that referenced this pull request May 29, 2023
The latest main for the proxmox-api-go project includes RNG device
creation.
Return value for proxmox.ConfigQemu.CreateQemuEfiParams()
have changed ( Telmate/proxmox-api-go#255 )
lbajolet-hashicorp pushed a commit to hashicorp/packer-plugin-proxmox that referenced this pull request Jul 12, 2023
The latest main for the proxmox-api-go project includes RNG device
creation.
Return value for proxmox.ConfigQemu.CreateQemuEfiParams()
have changed ( Telmate/proxmox-api-go#255 )
@riverar
Copy link
Contributor

riverar commented Jan 2, 2024

func NewConfigQemuFromApi( will no longer return the deprecated parts of type ConfigQemu

Heads up: This was a fatal blow to the downstream terraform provider. In API terms, “deprecated” means discouraged for use and likely to be removed, while “obsolete” means already removed or replaced.

Perhaps this should be removed from QemuConfig or at least have its comment updated? 😅
https://github.com/Telmate/proxmox-api-go/blob/master/proxmox/config_qemu.go#L68

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants