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

New Resource: azurerm_virtual_machine_implicit_data_disk_from_source #25537

Merged
merged 12 commits into from
May 29, 2024
1 change: 1 addition & 0 deletions internal/services/compute/registration.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ func (r Registration) DataSources() []sdk.DataSource {

func (r Registration) Resources() []sdk.Resource {
return []sdk.Resource{
VirtualMachineImplicitDataDiskFromSourceResource{},
VirtualMachineRunCommandResource{},
GalleryApplicationResource{},
GalleryApplicationVersionResource{},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
package compute

import (
"context"
"fmt"
"log"
"time"
Expand All @@ -29,9 +30,51 @@ func resourceVirtualMachineDataDiskAttachment() *pluginsdk.Resource {
Read: resourceVirtualMachineDataDiskAttachmentRead,
Update: resourceVirtualMachineDataDiskAttachmentCreateUpdate,
Delete: resourceVirtualMachineDataDiskAttachmentDelete,
Importer: pluginsdk.ImporterValidatingResourceId(func(id string) error {
Importer: pluginsdk.ImporterValidatingResourceIdThen(func(id string) error {
_, err := parse.DataDiskID(id)
return err
}, func(ctx context.Context, d *pluginsdk.ResourceData, meta interface{}) ([]*pluginsdk.ResourceData, error) {
client := meta.(*clients.Client).Compute.VirtualMachinesClient
id, err := parse.DataDiskID(d.Id())
if err != nil {
return nil, err
}

virtualMachineId := virtualmachines.NewVirtualMachineID(id.SubscriptionId, id.ResourceGroup, id.VirtualMachineName)
virtualMachine, err := client.Get(ctx, virtualMachineId, virtualmachines.DefaultGetOperationOptions())
if err != nil {
if response.WasNotFound(virtualMachine.HttpResponse) {
return nil, fmt.Errorf("%s was not found therefore Data Disk Attachment cannot be imported", virtualMachineId)
}

return nil, fmt.Errorf("retrieving %s: %+v", id, err)
}

var disk *virtualmachines.DataDisk
if model := virtualMachine.Model; model != nil {
if props := model.Properties; props != nil {
if profile := props.StorageProfile; profile != nil {
if dataDisks := profile.DataDisks; dataDisks != nil {
for _, dataDisk := range *dataDisks {
if *dataDisk.Name == id.Name {
disk = &dataDisk
break
}
}
}
}
}
}

if disk == nil {
return nil, fmt.Errorf("Data Disk %s was not found", *id)
}

if disk.CreateOption != virtualmachines.DiskCreateOptionTypesAttach && disk.CreateOption != virtualmachines.DiskCreateOptionTypesEmpty {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to make a test for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, added a new testcase:

=== RUN TestAccVirtualMachineDataDiskAttachment_importImplicitDataDisk
=== PAUSE TestAccVirtualMachineDataDiskAttachment_importImplicitDataDisk
=== CONT TestAccVirtualMachineDataDiskAttachment_importImplicitDataDisk
--- PASS: TestAccVirtualMachineDataDiskAttachment_importImplicitDataDisk (594.13s)
PASS

return nil, fmt.Errorf("the value of `create_option` for the imported `azurerm_virtual_machine_data_disk_attachment` instance must be `Attach` or `Empty`, whereas now is %s", disk.CreateOption)
}

return []*pluginsdk.ResourceData{d}, nil
}),

Timeouts: &pluginsdk.ResourceTimeout{
Expand Down Expand Up @@ -154,6 +197,15 @@ func resourceVirtualMachineDataDiskAttachmentCreateUpdate(d *pluginsdk.ResourceD
WriteAcceleratorEnabled: pointer.To(writeAcceleratorEnabled),
}

// there are ways to provision a VM without a StorageProfile and/or DataDisks
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should also include a test to confirm that the existing VM won't have any diffs when a disk is attached this way

Copy link
Contributor Author

@ms-zhenhua ms-zhenhua May 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I have re-run all existing testcases which are passed and attached in the description.
  2. Though this comment said there are ways to provision a VM without a StorageProfile and/or DataDisks with certain types of images, I do not really know which image will trigger such scenario. Kindly let me know if you know the image type and I will add the testcase. However, this part of code will not cause any diff because this code only helps initialize the storage and disk when they are nil, which is a protection for the existing code.

if virtualMachine.Model.Properties.StorageProfile == nil {
virtualMachine.Model.Properties.StorageProfile = &virtualmachines.StorageProfile{}
}

if virtualMachine.Model.Properties.StorageProfile.DataDisks == nil {
virtualMachine.Model.Properties.StorageProfile.DataDisks = pointer.To(make([]virtualmachines.DataDisk, 0))
}

disks := *virtualMachine.Model.Properties.StorageProfile.DataDisks

existingIndex := -1
Expand Down
Loading
Loading