r\linux_virtual_machine,r\windows_virtual_machine: Add support for diff_disk_settings.placement
#14847
Conversation
…r `diff_disk_settings.placement`
internal/services/compute/linux_virtual_machine_resource_disk_os_test.go
Outdated
Show resolved
Hide resolved
internal/services/compute/windows_virtual_machine_resource_disk_os_test.go
Outdated
Show resolved
Hide resolved
|
Hi @magodo thanks for reviewing the change! I've updated to code to fix the review comments, please take a look. |
|
Updated test result |
|
@magodo do need to improve something in this PR? |
| @@ -285,7 +303,8 @@ func flattenVirtualMachineOSDisk(ctx context.Context, disksClient *compute.Disks | |||
| diffDiskSettings := make([]interface{}, 0) | |||
| if input.DiffDiskSettings != nil { | |||
| diffDiskSettings = append(diffDiskSettings, map[string]interface{}{ | |||
| "option": string(input.DiffDiskSettings.Option), | |||
| "option": string(input.DiffDiskSettings.Option), | |||
| "placement": string(input.DiffDiskSettings.Placement), | |||
There was a problem hiding this comment.
this'll be nil/empty in the response for existing resources, so we'd need to default this?
There was a problem hiding this comment.
The original version has it set, but I've asked not to: #14847 (comment) 😅.
My understanding is the original version is trying to check a low likely case that the API didn't return the placement, then set it to its default value. But that is not always right. Imagine users have set the placement to DiffDiskPlacementResourceDisk, then if the API didn't return it, it will still ends up with a diff as the default value is DiffDiskPlacementCacheDisk. So I prefer just set whatever API is returning.
There was a problem hiding this comment.
@tombuildsstuff @magodo thanks for sharing the thoughts. I took another look, this property is not introduced initially in service, and default value is CacheDisk, so when it's missing from the response, we could probably consider it as CacheDisk. If it's set to ResourceDisk, it should not be missing in the response.
|
This functionality has been released in v3.6.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you! |
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Implement Ephemeral OS Disk on Temp Resource Disk
Add new property
diff_disk_settings.placement. Default value isCacheDiskand can be set toResourceDiskto enable this feature.Also
cachingneeds to beReadOnlyto enablediff_disk_settings. Add this restriction as well.Test result:
