From 7d2554db7db524db497e92ed2ea5b5a83666e375 Mon Sep 17 00:00:00 2001 From: Pavel Boldyrev <627562+bpg@users.noreply.github.com> Date: Fri, 16 Feb 2024 23:09:55 -0500 Subject: [PATCH] acceptance tests! Signed-off-by: Pavel Boldyrev <627562+bpg@users.noreply.github.com> --- .golangci.yml | 1 + fwprovider/tests/resource_vm_test.go | 270 ++++++++++++++++++++------- proxmoxtf/resource/file.go | 4 +- proxmoxtf/resource/vm/disk.go | 38 ++-- proxmoxtf/resource/vm/schema.go | 14 +- proxmoxtf/resource/vm/vm.go | 43 ++--- 6 files changed, 254 insertions(+), 116 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index d176bdb8..2578d2ad 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -73,6 +73,7 @@ linters: - ireturn - maintidx - nlreturn + - perfsprint - tagliatelle - testpackage - varnamelen diff --git a/fwprovider/tests/resource_vm_test.go b/fwprovider/tests/resource_vm_test.go index 15e3aa92..2892f828 100644 --- a/fwprovider/tests/resource_vm_test.go +++ b/fwprovider/tests/resource_vm_test.go @@ -9,93 +9,221 @@ package tests import ( "context" "fmt" + "regexp" "testing" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" "github.com/hashicorp/terraform-plugin-sdk/v2/terraform" - "github.com/stretchr/testify/require" -) - -const ( - accTestVMName = "proxmox_virtual_environment_vm.test_vm" - accTestVMCloneName = "proxmox_virtual_environment_vm.test_vm_clone" ) func TestAccResourceVM(t *testing.T) { t.Parallel() + tests := []struct { + name string + step resource.TestStep + }{ + {"multiline description", resource.TestStep{ + Config: ` + resource "proxmox_virtual_environment_vm" "test_vm1" { + node_name = "pve" + started = false + + description = <<-EOT + my + description + value + EOT + }`, + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("proxmox_virtual_environment_vm.test_vm1", "description", "my\ndescription\nvalue"), + ), + }}, + {"single line description", resource.TestStep{ + Config: ` + resource "proxmox_virtual_environment_vm" "test_vm2" { + node_name = "pve" + started = false + + description = "my description value" + }`, + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("proxmox_virtual_environment_vm.test_vm2", "description", "my description value"), + ), + }}, + {"no description", resource.TestStep{ + Config: ` + resource "proxmox_virtual_environment_vm" "test_vm3" { + node_name = "pve" + started = false + + description = "" + }`, + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("proxmox_virtual_environment_vm.test_vm3", "description", ""), + ), + }}, + } + accProviders := testAccMuxProviders(context.Background(), t) - resource.Test(t, resource.TestCase{ - ProtoV6ProviderFactories: accProviders, - Steps: []resource.TestStep{ + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + resource.Test(t, resource.TestCase{ + ProtoV6ProviderFactories: accProviders, + Steps: []resource.TestStep{tt.step}, + }) + }) + } +} + +func TestAccResourceVMDisks(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + steps []resource.TestStep + }{ + {"create disk with default parameters", []resource.TestStep{{ + Config: ` + resource "proxmox_virtual_environment_vm" "test_disk1" { + node_name = "pve" + started = false + name = "test-disk1" + + disk { + // note: default qcow2 is not supported by lvm (?) + file_format = "raw" + datastore_id = "local-lvm" + interface = "virtio0" + size = 8 + } + }`, + Check: resource.ComposeTestCheckFunc( + testResourceAttributes("proxmox_virtual_environment_vm.test_disk1", map[string]string{ + "disk.0.cache": "none", + "disk.0.datastore_id": "local-lvm", + "disk.0.discard": "ignore", + "disk.0.file_format": "raw", + // "disk.0.file_id": "", // is empty by default, but we can't check for that + "disk.0.interface": "virtio0", + "disk.0.iothread": "false", + "disk.0.path_in_datastore": `vm-\d+-disk-\d+`, + "disk.0.size": "8", + "disk.0.ssd": "false", + }), + ), + }}}, + {"create disk from an image", []resource.TestStep{{ + Config: ` + resource "proxmox_virtual_environment_download_file" "test_disk2_image" { + content_type = "iso" + datastore_id = "local" + node_name = "pve" + url = "https://cloud-images.ubuntu.com/jammy/current/jammy-server-cloudimg-amd64.img" + } + resource "proxmox_virtual_environment_vm" "test_disk2" { + node_name = "pve" + started = false + name = "test-disk2" + disk { + datastore_id = "local-lvm" + file_id = proxmox_virtual_environment_download_file.test_disk2_image.id + interface = "virtio0" + iothread = true + discard = "on" + size = 20 + } + }`, + Check: resource.ComposeTestCheckFunc( + testResourceAttributes("proxmox_virtual_environment_vm.test_disk2", map[string]string{ + "disk.0.cache": "none", + "disk.0.datastore_id": "local-lvm", + "disk.0.discard": "on", + "disk.0.file_format": "raw", + // "disk.0.file_id": "", // is empty by default, but we can't check for that + "disk.0.interface": "virtio0", + "disk.0.iothread": "true", + "disk.0.path_in_datastore": `vm-\d+-disk-\d+`, + "disk.0.size": "20", + "disk.0.ssd": "false", + }), + ), + }}}, + {"clone default disk", []resource.TestStep{ { - Config: testAccResourceVMCreateConfig(false), - Check: testAccResourceVMCreateCheck(t), + Config: ` + resource "proxmox_virtual_environment_vm" "test_disk3_template" { + node_name = "pve" + started = false + name = "test-disk3-template" + template = "true" + + disk { + file_format = "raw" + datastore_id = "local-lvm" + interface = "virtio0" + size = 8 + } + } + resource "proxmox_virtual_environment_vm" "test_disk3" { + node_name = "pve" + started = false + name = "test-disk3" + + clone { + vm_id = proxmox_virtual_environment_vm.test_disk3_template.id + } + } + `, + Check: resource.ComposeTestCheckFunc( + // fully cloned disk, does not have any attributes in state + resource.TestCheckNoResourceAttr("proxmox_virtual_environment_vm.test_disk3", "disk.0"), + ), }, { - Config: testAccResourceVMCreateConfig(true) + testAccResourceVMCreateCloneConfig(), - Check: testAccResourceVMCreateCloneCheck(t), + RefreshState: true, }, - }, - }) + }}, + //{"default disk parameters", resource.TestStep{}}, + //{"default disk parameters", resource.TestStep{}}, + } + + accProviders := testAccMuxProviders(context.Background(), t) + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + resource.Test(t, resource.TestCase{ + ProtoV6ProviderFactories: accProviders, + Steps: tt.steps, + }) + }) + } } -func testAccResourceVMCreateConfig(isTemplate bool) string { - return fmt.Sprintf(` -resource "proxmox_virtual_environment_vm" "test_vm" { - node_name = "%s" - vm_id = 2100 - template = %t - started = false +func testResourceAttributes(res string, attrs map[string]string) resource.TestCheckFunc { + return func(s *terraform.State) error { + for k, v := range attrs { + if err := resource.TestCheckResourceAttrWith(res, k, func(got string) error { + match, err := regexp.Match(v, []byte(got)) //nolint:mirror + if err != nil { + return fmt.Errorf("error matching %s: %w", v, err) + } + if !match { + return fmt.Errorf("expected %s to match %s", got, v) + } + return nil + })(s); err != nil { + return err + } + } - disk { - file_format= "raw" - datastore_id = "local-lvm" - interface = "virtio0" - size = 8 - } - -} -`, accTestNodeName, isTemplate) -} - -func testAccResourceVMCreateCheck(t *testing.T) resource.TestCheckFunc { - t.Helper() - - return resource.ComposeTestCheckFunc( - func(*terraform.State) error { - err := getNodesClient().VM(2100).WaitForVMStatus(context.Background(), "stopped", 10, 1) - require.NoError(t, err, "vm did not start") - return nil - }, - ) -} - -func testAccResourceVMCreateCloneConfig() string { - return fmt.Sprintf(` -resource "proxmox_virtual_environment_vm" "test_vm_clone" { - depends_on = [proxmox_virtual_environment_vm.test_vm] - - node_name = "%s" - vm_id = 2101 - started = false - - clone { - vm_id = 2100 - } -} -`, accTestNodeName) -} - -func testAccResourceVMCreateCloneCheck(t *testing.T) resource.TestCheckFunc { - t.Helper() - - return resource.ComposeTestCheckFunc( - func(*terraform.State) error { - err := getNodesClient().VM(2101).WaitForVMStatus(context.Background(), "stopped", 20, 1) - require.NoError(t, err, "vm did not start") - return nil - }, - ) + return nil + } } diff --git a/proxmoxtf/resource/file.go b/proxmoxtf/resource/file.go index 3fe5955c..51a64417 100644 --- a/proxmoxtf/resource/file.go +++ b/proxmoxtf/resource/file.go @@ -22,8 +22,6 @@ import ( "strings" "time" - "github.com/bpg/terraform-provider-proxmox/proxmoxtf/resource/ssh" - "github.com/google/uuid" "github.com/hashicorp/go-cty/cty" "github.com/hashicorp/terraform-plugin-log/tflog" @@ -227,7 +225,7 @@ func File() *schema.Resource { DeleteContext: fileDelete, UpdateContext: fileUpdate, Importer: &schema.ResourceImporter{ - StateContext: func(ctx context.Context, d *schema.ResourceData, i interface{}) ([]*schema.ResourceData, error) { + StateContext: func(_ context.Context, d *schema.ResourceData, _ interface{}) ([]*schema.ResourceData, error) { node, volID, err := fileParseImportID(d.Id()) if err != nil { return nil, err diff --git a/proxmoxtf/resource/vm/disk.go b/proxmoxtf/resource/vm/disk.go index c07f39e9..95e7ac38 100644 --- a/proxmoxtf/resource/vm/disk.go +++ b/proxmoxtf/resource/vm/disk.go @@ -3,21 +3,22 @@ package vm import ( "context" "fmt" - "github.com/bpg/terraform-provider-proxmox/proxmoxtf/resource/ssh" "regexp" "strconv" "strings" - "github.com/bpg/terraform-provider-proxmox/proxmox" - "github.com/bpg/terraform-provider-proxmox/proxmox/nodes/vms" - "github.com/bpg/terraform-provider-proxmox/proxmox/types" - "github.com/bpg/terraform-provider-proxmox/proxmoxtf" - "github.com/bpg/terraform-provider-proxmox/proxmoxtf/resource/validator" - "github.com/bpg/terraform-provider-proxmox/proxmoxtf/structure" "github.com/hashicorp/terraform-plugin-log/tflog" "github.com/hashicorp/terraform-plugin-sdk/v2/diag" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation" + + "github.com/bpg/terraform-provider-proxmox/proxmox" + "github.com/bpg/terraform-provider-proxmox/proxmox/nodes/vms" + "github.com/bpg/terraform-provider-proxmox/proxmox/ssh" + "github.com/bpg/terraform-provider-proxmox/proxmox/types" + "github.com/bpg/terraform-provider-proxmox/proxmoxtf" + "github.com/bpg/terraform-provider-proxmox/proxmoxtf/resource/validator" + "github.com/bpg/terraform-provider-proxmox/proxmoxtf/structure" ) const ( @@ -199,7 +200,7 @@ func diskSchema() *schema.Schema { } } -// called from vmCreateClone +// called from vmCreateClone. func createDisks( ctx context.Context, vmConfig *vms.GetResponseData, d *schema.ResourceData, vmAPI *vms.Client, ) (vms.CustomStorageDevices, error) { @@ -226,7 +227,6 @@ func createDisks( } // disk is present, i.e. when cloning a template, but we need to check if it needs to be moved or resized - timeoutSec := d.Get(mkTimeoutMoveDisk).(int) err := resizeDiskIfRequired(ctx, currentDisk, planDisk, vmAPI, timeoutSec) @@ -263,7 +263,7 @@ func resizeDiskIfRequired( err := vmAPI.ResizeVMDisk(ctx, diskResizeBody, timeoutSec) if err != nil { - return err + return fmt.Errorf("failed to resize disk: %w", err) } } @@ -291,7 +291,7 @@ func moveDiskIfRequired( err := vmAPI.MoveVMDisk(ctx, diskMoveBody, timeoutSec) if err != nil { - return err + return fmt.Errorf("failed to move disk: %w", err) } } @@ -322,7 +322,7 @@ func createDisk(ctx context.Context, disk *vms.CustomStorageDevice, vmAPI *vms.C err := vmAPI.UpdateVM(ctx, diskUpdateBody) if err != nil { - return err + return fmt.Errorf("failed to create disk: %w", err) } return nil @@ -331,7 +331,7 @@ func createDisk(ctx context.Context, disk *vms.CustomStorageDevice, vmAPI *vms.C func vmImportCustomDisks(ctx context.Context, d *schema.ResourceData, m interface{}) error { vmID, err := strconv.Atoi(d.Id()) if err != nil { - return err + return fmt.Errorf("failed to convert vm id to int: %w", err) } planDisks, err := getStorageDevicesFromResource(d) @@ -398,7 +398,7 @@ func vmImportCustomDisks(ctx context.Context, d *schema.ResourceData, m interfac api, err := config.GetClient() if err != nil { - return err + return fmt.Errorf("failed to get client: %w", err) } nodeName := d.Get(mkNodeName).(string) @@ -406,10 +406,10 @@ func vmImportCustomDisks(ctx context.Context, d *schema.ResourceData, m interfac out, err := api.SSH().ExecuteNodeCommands(ctx, nodeName, commands) if err != nil { if matches, e := regexp.Match(`pvesm: .* not found`, out); e == nil && matches { - return ssh.NewErrSSHUserNoPermission(api.SSH().Username()) + return ssh.NewErrUserHasNoPermission(api.SSH().Username()) //nolint:wrapcheck } - return err + return fmt.Errorf("failed to import disks: %w", err) } tflog.Debug(ctx, "vmCreateCustomDisks", map[string]interface{}{ @@ -457,7 +457,7 @@ func getDiskDeviceObjects1(d *schema.ResourceData, disks []interface{}) (vms.Cus false, ) if err != nil { - return diskDeviceObjects, err + return diskDeviceObjects, fmt.Errorf("failed to read disk speed: %w", err) } if fileFormat == "" { @@ -519,7 +519,7 @@ func getDiskDeviceObjects1(d *schema.ResourceData, disks []interface{}) (vms.Cus if storageInterface != "virtio" && storageInterface != "scsi" && storageInterface != "sata" { return diskDeviceObjects, fmt.Errorf( - "The disk interface '%s' is not supported, should be one of 'virtioN', 'sataN', or 'scsiN'", + "the disk interface '%s' is not supported, should be one of 'virtioN', 'sataN', or 'scsiN'", diskInterface, ) } @@ -657,7 +657,7 @@ func readDisk1(ctx context.Context, d *schema.ResourceData, return diags } -func updateDisk(d *schema.ResourceData, vmConfig *vms.GetResponseData, updateBody *vms.UpdateRequestBody) error { +func updateDisk(d *schema.ResourceData, updateBody *vms.UpdateRequestBody) error { // Prepare the new disk device configuration. if !d.HasChange(mkDisk) { return nil diff --git a/proxmoxtf/resource/vm/schema.go b/proxmoxtf/resource/vm/schema.go index c14fc8f5..d66a370c 100644 --- a/proxmoxtf/resource/vm/schema.go +++ b/proxmoxtf/resource/vm/schema.go @@ -6,11 +6,12 @@ import ( "strconv" "strings" - "github.com/bpg/terraform-provider-proxmox/proxmoxtf/resource/validator" - "github.com/bpg/terraform-provider-proxmox/proxmoxtf/structure" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/customdiff" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation" + + "github.com/bpg/terraform-provider-proxmox/proxmoxtf/resource/validator" + "github.com/bpg/terraform-provider-proxmox/proxmoxtf/structure" ) const ( @@ -568,6 +569,15 @@ func VM() *schema.Resource { Description: "The description", Optional: true, Default: dvDescription, + StateFunc: func(i interface{}) string { + // PVE always adds a newline to the description, so we have to do the same, + // also taking in account the CLRF case (Windows) + if i.(string) != "" { + return strings.ReplaceAll(strings.TrimSpace(i.(string)), "\r\n", "\n") + } + + return "" + }, }, mkDisk: diskSchema(), mkEFIDisk: { diff --git a/proxmoxtf/resource/vm/vm.go b/proxmoxtf/resource/vm/vm.go index 32e739d3..7cdb3ee7 100644 --- a/proxmoxtf/resource/vm/vm.go +++ b/proxmoxtf/resource/vm/vm.go @@ -183,7 +183,6 @@ func vmCreateClone(ctx context.Context, d *schema.ResourceData, m interface{}) d vmID = *vmIDNew err = d.Set(mkVMID, vmID) - if err != nil { return diag.FromErr(err) } @@ -1471,8 +1470,8 @@ func vmGetHostPCIDeviceObjects(d *schema.ResourceData) vms.CustomPCIDevices { } if ids != "" { - dIds := strings.Split(ids, ";") - device.DeviceIDs = &dIds + dIDs := strings.Split(ids, ";") + device.DeviceIDs = &dIDs } if mdev != "" { @@ -1741,7 +1740,7 @@ func vmGetVGADeviceObject(d *schema.ResourceData) (*vms.CustomVGADevice, error) true, ) if err != nil { - return nil, err + return nil, fmt.Errorf("error reading VGA block: %w", err) } vgaEnabled := types.CustomBool(vgaBlock[mkVGAEnabled].(bool)) @@ -2458,7 +2457,8 @@ func vmReadCustom( currentInitialization := d.Get(mkInitialization).([]interface{}) - if len(clone) > 0 { + switch { + case len(clone) > 0: if len(currentInitialization) > 0 { if len(initialization) > 0 { err := d.Set( @@ -2471,10 +2471,10 @@ func vmReadCustom( diags = append(diags, diag.FromErr(err)...) } } - } else if len(initialization) > 0 { + case len(initialization) > 0: err := d.Set(mkInitialization, []interface{}{initialization}) diags = append(diags, diag.FromErr(err)...) - } else { + default: err := d.Set(mkInitialization, []interface{}{}) diags = append(diags, diag.FromErr(err)...) } @@ -2637,11 +2637,9 @@ func vmReadCustom( if len(clone) > 0 { err = d.Set(mkNetworkDevice, networkDeviceList[:networkDeviceLast+1]) diags = append(diags, diag.FromErr(err)...) - } else { - if len(currentNetworkDeviceList) > 0 || networkDeviceLast > -1 { - err := d.Set(mkNetworkDevice, networkDeviceList[:networkDeviceLast+1]) - diags = append(diags, diag.FromErr(err)...) - } + } else if len(currentNetworkDeviceList) > 0 || networkDeviceLast > -1 { + err := d.Set(mkNetworkDevice, networkDeviceList[:networkDeviceLast+1]) + diags = append(diags, diag.FromErr(err)...) } } @@ -2656,7 +2654,8 @@ func vmReadCustom( currentOperatingSystem := d.Get(mkOperatingSystem).([]interface{}) - if len(clone) > 0 { + switch { + case len(clone) > 0: if len(currentOperatingSystem) > 0 { err := d.Set( mkOperatingSystem, @@ -2664,11 +2663,11 @@ func vmReadCustom( ) diags = append(diags, diag.FromErr(err)...) } - } else if len(currentOperatingSystem) > 0 || - operatingSystem[mkOperatingSystemType] != dvOperatingSystemType { + case len(currentOperatingSystem) > 0 || + operatingSystem[mkOperatingSystemType] != dvOperatingSystemType: err := d.Set(mkOperatingSystem, []interface{}{operatingSystem}) diags = append(diags, diag.FromErr(err)...) - } else { + default: err := d.Set(mkOperatingSystem, []interface{}{}) diags = append(diags, diag.FromErr(err)...) } @@ -2875,18 +2874,19 @@ func vmReadCustom( currentVGA := d.Get(mkVGA).([]interface{}) - if len(clone) > 0 { + switch { + case len(clone) > 0: if len(currentVGA) > 0 { err := d.Set(mkVGA, []interface{}{vga}) diags = append(diags, diag.FromErr(err)...) } - } else if len(currentVGA) > 0 || + case len(currentVGA) > 0 || vga[mkVGAEnabled] != dvVGAEnabled || vga[mkVGAMemory] != dvVGAMemory || - vga[mkVGAType] != dvVGAType { + vga[mkVGAType] != dvVGAType: err := d.Set(mkVGA, []interface{}{vga}) diags = append(diags, diag.FromErr(err)...) - } else { + default: err := d.Set(mkVGA, []interface{}{}) diags = append(diags, diag.FromErr(err)...) } @@ -3524,7 +3524,7 @@ func vmUpdate(ctx context.Context, d *schema.ResourceData, m interface{}) diag.D rebootRequired = true } - err := updateDisk(d, vmConfig, updateBody) + err := updateDisk(d, updateBody) if err != nil { return diag.FromErr(err) } @@ -3777,6 +3777,7 @@ func vmUpdate(ctx context.Context, d *schema.ResourceData, m interface{}) diag.D if e := vmShutdown(ctx, vmAPI, d); e != nil { return e } + rebootRequired = false } }