-
-
Notifications
You must be signed in to change notification settings - Fork 138
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(cluster): Extend the available attributes for the proxmox_virtual_environment_cluster_options
resource
#1241
Conversation
proxmox_virtual_environment_cluster_options
resourceproxmox_virtual_environment_cluster_options
resource
167e5ec
to
908b9a7
Compare
Thanks for the PR! 👍🏼 I'll review it over the weekend. Sorry about the failed Qodana checks, I'm still experimenting with different static code analysis tools.
Just fyi, it is documented in https://pve.proxmox.com/pve-docs/api-viewer/#/nodes/{node}/qemu (POST), and also in many references in the admin guide https://pve.proxmox.com/pve-docs/pve-admin-guide.html#_strong_qm_strong_qemu_kvm_virtual_machine_manager |
@all-contributors please add @svengreb for code |
I've put up a pull request to add @svengreb! 🎉 |
Sure, take your time 😃
Oh, I haven't checked other endpoints, but it makes sense that these numbers must be documented somewhere related to VMs itself! |
This commit implements the `next-id` and `notify` PVE API cluster options. The `next-id` attribute allows to control the range for the next free VM ID. It is implemented as object and can be used in the `proxmox_virtual_environment_cluster_options` resource and can be used like this: ```terraform resource "proxmox_virtual_environment_cluster_options" "options" { next_id = { lower = 200 upper = 299 } } ``` Note that the minimum and maximum values are unfortunately not documented in the PVE API explorer but can be found in the web UI where the form fields have validations! The `notify` PVE API attribute is also an object that has all the PVE API fields: ```terraform resource "proxmox_virtual_environment_cluster_options" "options" { notify = { ha_fencing_mode = "never" ha_fencing_target = "default-matcher" package_updates = "always" package_updates_target = "default-matcher" package_replication = "always" package_replication_target = "default-matcher" } } ```terraform Note that the "fencing" attribute names have been adjusted to better reflect their meaning since they are scoped to the Proxmox VE HA fencing feature [1]. All attributes with the `_target` suffix are names for the Proxmox VE notifications matchers [2]. [1]: https://pve.proxmox.com/wiki/Fencing [2]: https://pve.proxmox.com/pve-docs/chapter-notifications.html#notification_matchers Signed-off-by: Sven Greb <[email protected]>
908b9a7
to
6bd10c0
Compare
Signed-off-by: Sven Greb <[email protected]>
Validators: []validator.String{ | ||
stringvalidator.UTF8LengthAtLeast(1), | ||
stringvalidator.RegexMatches(regexp.MustCompile(`^\S|^$`), "must not start with whitespace"), | ||
stringvalidator.RegexMatches(regexp.MustCompile(`\S$|^$`), "must not end with whitespace"), | ||
}, |
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.
This validation is duplicated few times in the schema, so could extracted in a shared validator.
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.
Definitely, it's basically a validator that could be applied to (almost) all strings, even when the Proxmox VE API does not check or enforce it.
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 so much for adding this feature!
There are few minor comments that we can address later. I'll probably incorporate them into my ongoing work on the VM resource.
LGTM! 🚀
@@ -285,6 +403,24 @@ func (m *clusterOptionsModel) importFromOptionsAPI( | |||
m.MigrationNetwork = types.StringNull() | |||
} | |||
|
|||
if opts.NextID != nil { | |||
m.NextID = &clusterOptionsNextIDModel{} | |||
lower := int64(*opts.NextID.Lower) |
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.
we can add something like .PointerInt64()
to CustomInt64
type to simplify this conversion.
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.
Yep, I also thought about it, but decided to solve it this way to get the implementation done.
Contributor's Note
/docs
for any user-facing features or additions./fwprovider/tests
for any new or updated resources / data sources.make example
to verify that the change works as expected.Summary
This pull request implements the
next-id
andnotify
PVE API cluster options.The
next-id
attribute allows to control the range for the next free VM ID. It is implemented as object and can be used in theproxmox_virtual_environment_cluster_options
resource like this:Note that the minimum and maximum values are unfortunately not documented in the PVE API explorer but can be found in the web UI where the form fields have validations!
The
notify
PVE API attribute is also an object that has all the PVE API fields:Note that the "fencing" attribute names have been adjusted to better reflect their meaning since they are scoped to the Proxmox VE HA fencing feature. All attributes with the
_target
suffix are names for the Proxmox VE notifications matchers.Community Note