chore(tests): allow different node / storage names in "example tests" #2042
Conversation
Co-authored-by: MacherelR <[email protected]> Signed-off-by: Pavel Boldyrev <[email protected]> Signed-off-by: MacherelR <[email protected]> Signed-off-by: Pavel Boldyrev <[email protected]>
## Walkthrough
The changes introduce two new variables for Proxmox node name and storage, replacing hardcoded values in multiple Terraform resources to use these variables instead. Additionally, a resource for downloading a Debian ISO image is removed, and references to image resources are updated for consistency.
## Changes
| Files/Paths | Change Summary |
|----------------------------------------------------------------------------|-----------------------------------------------------------------------------------------------------------------|
| example/resource_virtual_environment_container.tf, example/resource_virtual_environment_vm.tf | Replaced hardcoded "local-lvm" storage references with `var.virtual_environment_storage`; updated disk/image references. |
| example/resource_virtual_environment_download_file.tf | Replaced hardcoded node name "pve" with `var.virtual_environment_node_name`; removed Debian ISO image resource. |
| example/variables.tf | Added variables: `virtual_environment_node_name` and `virtual_environment_storage` with defaults and descriptions. |
## Possibly related PRs
- bpg/terraform-provider-proxmox#1883: Simplifies example datastore references by consolidating outputs and replacing dynamic indexing with static strings; related through datastore reference modifications.
## Suggested labels
`size/S`✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
example/variables.tf (1)
16-26: Add validation blocks for the newly-introduced variablesThe defaults work, but nothing prevents a caller from passing an empty / malformed node or storage name. A tiny validation guard would fail fast and surface the problem during
terraform validate, instead of at apply-time on the Proxmox side.variable "virtual_environment_node_name" { description = "Name of the Proxmox node" type = string default = "pve" + + validation { + condition = can(regex("^[a-zA-Z0-9._-]+$", var.virtual_environment_node_name)) + error_message = "Node name may only contain letters, numbers, dots, underscores and dashes." + } } variable "virtual_environment_storage" { description = "Name of the Proxmox storage" type = string default = "local-lvm" + + validation { + condition = can(regex("^[a-zA-Z0-9._-]+$", var.virtual_environment_storage)) + error_message = "Storage name may only contain letters, numbers, dots, underscores and dashes." + } }example/resource_virtual_environment_container.tf (1)
68-70: Minor: mirror the template changes in the clone block?
proxmox_virtual_environment_container.example’smount_pointblock still hard-codes the bind-mount path, which is fine, but if you ever add more disks here you might want to reusevar.virtual_environment_storagefor consistency the same way you did in the template resource.example/resource_virtual_environment_download_file.tf (1)
5-11: Node name made configurable – great; storage still hard-codedThe switch to
var.virtual_environment_node_nameremoves the last reference to"pve"– nice.
For parity with other changes, you might also exposedatastore_idas a variable (many installations don’t have the defaultlocaldirectory storage enabled).A minimal tweak:
- datastore_id = "local" + datastore_id = var.virtual_environment_download_datastore(where the new variable could default to
"local").
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
example/resource_virtual_environment_container.tf(2 hunks)example/resource_virtual_environment_download_file.tf(2 hunks)example/resource_virtual_environment_vm.tf(2 hunks)example/variables.tf(1 hunks)
🔇 Additional comments (3)
example/resource_virtual_environment_container.tf (1)
6-15: Good move to parameterise storage-specific fieldsSwitching both
disk.datastore_idandmount_point.volumetovar.virtual_environment_storageremoves the last hard-coded"local-lvm"dependency and keeps the example self-contained.example/resource_virtual_environment_vm.tf (2)
2-3: Storage abstraction applied consistently – niceReplacing the literal
"local-lvm"inlocals { datastore_id = … }aligns the VM stack with the new variable.
48-56: Cross-datastore import may surprise usersThe QCOW2 image is downloaded to the fixed
datastore_id = "local"(see download_file.tf), yet the disks that import it are created onvar.virtual_environment_storage(default"local-lvm"). Proxmox can copy between stores, but on clusters without alocaldir storage or whenvirtual_environment_storageis alsolocal, the import path might break.Consider parameterising the download datastore as well, or at least document the expectation:
- datastore_id = "local" + datastore_id = var.virtual_environment_download_datastore # new variable with default "local"or add a note in README/examples.
Also applies to: 232-236
Contributor's Note
Split up #1995 into smaller pieces.
/docsfor any user-facing features or additions./fwprovider/testsfor any new or updated resources / data sources.make exampleto verify that the change works as expected.Proof of Work
Community Note
Relates #817
Summary by CodeRabbit