[Enhancement] - Add os_disk.storage_type and data_disk.storage_type to azurerm_image
#26936
Conversation
bruceharrison1984
changed the title
os_disk.storage_type to azurerm_imageos_disk.storage_type and data_disk.storage_type to azurerm_image
There was a problem hiding this comment.
Hi @bruceharrison1984 - Thanks for the PR. Unfortunately this is a breaking change for users as the new property has a default value. I've left a little guidance on how to address this below, but reach out over slack to the team if you need further help.
|
@jackofallops I believe I've hidden it all behind the feature flag, though it looks like there are multiple ways to accomplish this. |
There was a problem hiding this comment.
@bruceharrison1984 Thanks for making that change! I've made some more comments inline below, if you can take a look at these then this should be good to merge 👍
| 1. `id` (String) Azure Resource Manager ID. No newline at end of file | ||
| 1. `id` (String) Azure Resource Manager ID. |
There was a problem hiding this comment.
Not sure what's happening here, a stealthy space/newline maybe? 😄
There was a problem hiding this comment.
Looks like a trailing whitespace was removed by running the linter.
Not sure if we want to revert it, or just leave it?
|
|
||
| if v.StorageAccountType != nil && features.FourPointOhBeta() { | ||
| output = append(output, map[string]interface{}{ | ||
| "storage_type": string(*v.StorageAccountType), | ||
| }) | ||
| } |
There was a problem hiding this comment.
I don't think this will work as expected, since the intended output of this helper func should be a single map[string]interface{} containing all properties (contained as the only element of a []interface{}
Due to GH limitations I can't suggest inline, but something like this should work:
properties := map[string]interface{}{
"blob_uri": blobUri,
"caching": caching,
"managed_disk_id": managedDiskId,
"os_type": string(v.OsType),
"os_state": string(v.OsState),
"size_gb": diskSizeGB,
"disk_encryption_set_id": diskEncryptionSetId,
}
if v.StorageAccountType != nil && features.FourPointOhBeta() {
properties["storage_type"] = string(*v.StorageAccountType)
}
output = append(output, properties)|
|
||
| output = append(output, map[string]interface{}{ | ||
| "blob_uri": blobUri, | ||
| "caching": caching, | ||
| "lun": int(disk.Lun), | ||
| "managed_disk_id": managedDiskId, | ||
| "size_gb": diskSizeGb, | ||
| }) | ||
|
|
||
| if disk.StorageAccountType != nil && features.FourPointOhBeta() { | ||
| output = append(output, map[string]interface{}{ | ||
| "storage_type": string(*disk.StorageAccountType), | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
As above, we should output a single map here
Co-authored-by: Tom Bamford <tom@bamford.io>
Co-authored-by: Tom Bamford <tom@bamford.io>
Co-authored-by: Tom Bamford <tom@bamford.io>
|
@manicminer Is there a way to run tests against Beta 4.0 features? Right now the tests pass, but only because none of the 4.0 code is under test. |
|
@bruceharrison1984 You can export the environment variable |
|
@manicminer Augmenting the tests with the new |
There was a problem hiding this comment.
Thanks @bruceharrison1984, this is mostly looking great! Could you also add an acceptance test for this property? You should be able to find examples of other tests that are gated behind the 4.0 feature flag (via the features.FourPointOh() function).
I've also made a couple more comments, if you could take a look at those as well as the test coverage, then this will be ready for merging 👍
Co-authored-by: Tom Bamford <tom@bamford.io>
Co-authored-by: Tom Bamford <tom@bamford.io>
Co-authored-by: Tom Bamford <tom@bamford.io>
Co-authored-by: Tom Bamford <tom@bamford.io>
Co-authored-by: Tom Bamford <tom@bamford.io>
Co-authored-by: Tom Bamford <tom@bamford.io>
|
@manicminer All existing tests have been updated and gated with the new 4.0 property. I'll have another PR for this same resource shortly, so this should help grease the skids for that one. |
|
Test results 
|
|
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. |
Community Note
Description
This just adds a two new properties that allows choosing the type of disk that will be utilized by downstream VMs created with a particular image.
PR Checklist
For example: “
resource_name_here- description of change e.g. adding propertynew_property_name_here”Changes to existing Resource / Data Source
Testing
Change Log
Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.
azurerm_image- support for theos_disk.storage_typeanddata_disk.storage_typepropertiesThis is a (please select all that apply):
Note
If this PR changes meaningfully during the course of review please update the title and description as required.