feat(vm): add support for pool update #505
Conversation
This commit removed the ForceNew flag from the VM resource's `pool_id`
argument and implements pool update:
* if the VM was part of a pool, it is removed from it,
* if the new `pool_id` value is non-empty, the VM is added to that new
pool.
* This commit fixes a linter error that somehow doesn't manifest
unless some other, unrelated changes trigger it (see bpg#501 and
bpg#505).
* In addition it fixes a similar issue that had so far gone undetected
by the linter.
* Refactored the code in question into a function, since it was mostly
duplicated.
* Simplified a pair of conditionals that had the same code in both
branches.
fix: linter error in ambush
* This commit fixes a linter error that somehow doesn't manifest
unless some other, unrelated changes trigger it (see #501 and
#505).
* In addition it fixes a similar issue that had so far gone undetected
by the linter.
* Refactored the code in question into a function, since it was mostly
duplicated.
* Simplified a pair of conditionals that had the same code in both
branches.
| @@ -6,6 +6,8 @@ | |||
|
|
|||
| package pools | |||
|
|
|||
| import "github.com/bpg/terraform-provider-proxmox/internal/types" | |||
There was a problem hiding this comment.
Oh... I didn't realize this mistake that I've made before -- the proxmox package (API client) is now dependent on the internal package types 😞
You're also adding new API types and more dependencies between API and FWK types in #498.
This all won't work long term, as I think it will make sense to extract the API client into a separate repo (#423)
But I'll leave it as is for now, the "separation" refactoring can be done later.
| @@ -4798,6 +4842,10 @@ func vmUpdate(ctx context.Context, d *schema.ResourceData, m interface{}) diag.D | |||
| return diag.FromErr(e) | |||
| } | |||
|
|
|||
| if vmUpdatePool(ctx, d, api.Pool(), vmID) != nil { | |||
| return diag.FromErr(e) | |||
There was a problem hiding this comment.
this swallows the returned error value and uses error e from the previous statement
There was a problem hiding this comment.
Woops, sorry. Thanks for the fix.
proxmox/pools/pool_types.go
Outdated
| // If this is set to 1, VMs and datastores will be removed from the pool instead of added. | ||
| Delete *types.CustomBool `json:"delete,omitempty" url:"delete,omitempty,int"` | ||
| // The list of virtual machines to add or delete. | ||
| VMs *[]string `json:"vms,omitempty" url:"vms,omitempty,comma"` |
There was a problem hiding this comment.
We'd need a custom JSON serialization to a comma-separated list, though it works now as there is only a single VM ID in the use case.
ghost
mentioned this pull request
bpg
changed the title
This commit removed the
ForceNewflag from the VM resource'spool_idargument and implements pool update:pool_idvalue is non-empty, the VM is added to that new pool.It triggers the same linter problem as my other pull request, the same solution would apply.
Contributor's Note
Please mark the following items with an [x] if they apply to your PR.
Leave the [ ] if they are not applicable, or if you have not completed the item.
/docsfor any user-facing features or additions./examplesfor any new or updated resources / data sources.make examplesto verify that the change works as expected.Community Note