azurerm_healthcare_dicom_service resource & data source - support for new properties#27375
Conversation
| Computed: true, | ||
| }, | ||
|
|
||
| "cors_configuration": { |
There was a problem hiding this comment.
configuration is redundant,
| "cors_configuration": { | |
| "cors": { |
There was a problem hiding this comment.
Thanks, updated.
| Computed: true, | ||
| }, | ||
|
|
||
| "storage_configuration": { |
There was a problem hiding this comment.
ditto
| "storage_configuration": { | |
| "storage": { |
There was a problem hiding this comment.
Thanks, updated.
| Default: true, | ||
| }, | ||
|
|
||
| "cors_configuration": { |
There was a problem hiding this comment.
| "cors_configuration": { | |
| "cors": { |
There was a problem hiding this comment.
Thanks, updated.
| }, | ||
| "allowed_headers": { |
There was a problem hiding this comment.
| }, | |
| "allowed_headers": { | |
| }, | |
| "allowed_headers": { |
please add blank lines between properties
There was a problem hiding this comment.
Thanks, updated.
| }, | ||
| "max_age_in_seconds": { | ||
| Type: pluginsdk.TypeInt, | ||
| Optional: true, |
There was a problem hiding this comment.
can you validate this? i presume -3 is not a valid value? is 0? is 99999999999 ?
There was a problem hiding this comment.
Trid it out, can support 0 to 99998 inclusive, added the validatefunc
| ValidateFunc: validation.IsURLWithHTTPS, | ||
| }, | ||
|
|
||
| "storage_configuration": { |
There was a problem hiding this comment.
| "storage_configuration": { | |
| "storage": { |
There was a problem hiding this comment.
Updated
stephybun
left a comment
There was a problem hiding this comment.
Thanks @hqhqhqhqhqhqhqhqhqhqhq. Could you take a look through and fix up the. comments left in-line? Once that's done we can take another look through.
| "provision_state": { | ||
| Type: pluginsdk.TypeString, | ||
| Computed: true, | ||
| }, | ||
|
|
||
| "event_state": { | ||
| Type: pluginsdk.TypeString, | ||
| Computed: true, | ||
| }, |
There was a problem hiding this comment.
These fields contain transient information that isn't meaningful to users, we don't usually expose these in resources or data sources
| "provision_state": { | |
| Type: pluginsdk.TypeString, | |
| Computed: true, | |
| }, | |
| "event_state": { | |
| Type: pluginsdk.TypeString, | |
| Computed: true, | |
| }, |
There was a problem hiding this comment.
Thanks, this has been removed from the data source and the docs
| enableDataPartitions := false | ||
| if props.EnableDataPartitions != nil { | ||
| enableDataPartitions = pointer.From(props.EnableDataPartitions) | ||
| } | ||
| d.Set("data_partitions_enabled", enableDataPartitions) |
There was a problem hiding this comment.
pointer.From can do the nil check for us, so this can be reduced to
| enableDataPartitions := false | |
| if props.EnableDataPartitions != nil { | |
| enableDataPartitions = pointer.From(props.EnableDataPartitions) | |
| } | |
| d.Set("data_partitions_enabled", enableDataPartitions) | |
| d.Set("data_partitions_enabled", pointer.From(props.EnableDataPartitions)) |
There was a problem hiding this comment.
Thanks, updated in the resource & data source
| var enableDataPartitions *bool | ||
| if v, ok := d.GetOk("data_partitions_enabled"); ok { | ||
| enableDataPartitions = pointer.To(v.(bool)) | ||
| } | ||
|
|
||
| var corsConfiguration *dicomservices.CorsConfiguration | ||
| if v, ok := d.GetOk("cors"); ok { | ||
| corsConfiguration = expandDicomServiceCorsConfiguration(v.([]interface{})) | ||
| } | ||
|
|
||
| var encryption *dicomservices.Encryption | ||
| if v, ok := d.GetOk("encryption_key_url"); ok { | ||
| encryption = &dicomservices.Encryption{ | ||
| CustomerManagedKeyEncryption: &dicomservices.EncryptionCustomerManagedKeyEncryption{ | ||
| KeyEncryptionKeyUrl: pointer.To(v.(string)), | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| var storageConfiguration *dicomservices.StorageConfiguration | ||
| if v, ok := d.GetOk("storage"); ok { | ||
| storageConfiguration = expandStorageConfiguration(v.([]interface{})) | ||
| } |
There was a problem hiding this comment.
We should be using d.HasChanges here so we can support the use of ignore_changes, you may need to rewrite this to retrieve the existing resource and patch in changes with d.HasChanges. The update method in our guide on adding new resources has an explanation on how this should be done:
Here is also a practical example from within the provider:
terraform-provider-azurerm/internal/services/network/public_ip_resource.go
Lines 297 to 368 in 8477e14
There was a problem hiding this comment.
Got it, have updated the update function in the resource to this format.
| if configuration.FileSystemName != nil { | ||
| result["file_system_name"] = *configuration.FileSystemName | ||
| } |
There was a problem hiding this comment.
We could use pointer.From here for the nil check
| if configuration.FileSystemName != nil { | |
| result["file_system_name"] = *configuration.FileSystemName | |
| } | |
| result["file_system_name"] = pointer.From(configuration.FileSystemName) |
There was a problem hiding this comment.
Thanks, updated, the flatten function (used by the resource & data source)
| if configuration.StorageResourceId != nil { | ||
| result["storage_account_id"] = *configuration.StorageResourceId | ||
| } |
There was a problem hiding this comment.
We should parse the Storage account ID that we're getting back from the API so that we can catch any casing inconsistencies and potentially prevent them from creeping into user's state
| if configuration.StorageResourceId != nil { | |
| result["storage_account_id"] = *configuration.StorageResourceId | |
| } | |
| if v := pointer.From(configuration.StorageResourceId); v != "" { | |
| id, err := commonids.ParseStorageAccountID(v) | |
| if err != nil { | |
| return nil, err | |
| } | |
| result["storage_account_id"] = id.ID() | |
| } |
There was a problem hiding this comment.
Thanks, updated the flatten function (used by the resource & data source), the function now also returns error to the calling function
|
|
||
| --- | ||
|
|
||
| A `cors` supports the following: |
There was a problem hiding this comment.
For data sources we say a block exports rather than supports since these aren't arguments that can be supplied
| A `cors` supports the following: | |
| A `cors` exports the following: |
There was a problem hiding this comment.
Updated
|
|
||
| --- | ||
|
|
||
| A `storage` block supports the following: |
There was a problem hiding this comment.
| A `storage` block supports the following: | |
| A `storage` block exports the following: |
There was a problem hiding this comment.
Updated
|
|
||
| * `file_system_name` - The filesystem name of connected storage account. | ||
|
|
||
| * `storage_account_id` - The resource id of connected storage account. |
There was a problem hiding this comment.
| * `storage_account_id` - The resource id of connected storage account. | |
| * `storage_account_id` - The resource ID of connected storage account. |
There was a problem hiding this comment.
Updated the doc with required changes & other parts where it is id rather than ID
|
|
||
| * `file_system_name` - (Required) The filesystem name of connected storage account. Changing this forces a new Healthcare DICOM Service to be created. | ||
|
|
||
| * `storage_account_id` - (Required) The resource id of connected storage account. Changing this forces a new Healthcare DICOM Service to be created. |
There was a problem hiding this comment.
| * `storage_account_id` - (Required) The resource id of connected storage account. Changing this forces a new Healthcare DICOM Service to be created. | |
| * `storage_account_id` - (Required) The resource ID of connected storage account. Changing this forces a new Healthcare DICOM Service to be created. |
There was a problem hiding this comment.
Updated
@stephybun Thanks for all the reviews🙏, I have addressed all of them and reran the tests: Please help give another review when available, thank you! |
stephybun
left a comment
There was a problem hiding this comment.
Sorry for the delayed re-review @hqhqhqhqhqhqhqhqhqhqhq. This looks mostly fine, I posted a response to one of the comments regarding the default values for the cors and storage block, please take a look at the detail in the response.
stephybun
left a comment
There was a problem hiding this comment.
One final comment @hqhqhqhqhqhqhqhqhqhqhq then this is probably good to go.
| var enableDataPartitions *bool | ||
| if v, ok := d.GetOk("data_partitions_enabled"); ok { | ||
| enableDataPartitions = pointer.To(v.(bool)) | ||
| } | ||
|
|
||
| var corsConfiguration *dicomservices.CorsConfiguration | ||
| if v, ok := d.GetOk("cors"); ok { | ||
| corsConfiguration = expandDicomServiceCorsConfiguration(v.([]interface{})) | ||
| } | ||
|
|
||
| var encryption *dicomservices.Encryption | ||
| if v, ok := d.GetOk("encryption_key_url"); ok { | ||
| encryption = &dicomservices.Encryption{ | ||
| CustomerManagedKeyEncryption: &dicomservices.EncryptionCustomerManagedKeyEncryption{ | ||
| KeyEncryptionKeyURL: pointer.To(v.(string)), | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| var storageConfiguration *dicomservices.StorageConfiguration | ||
| if v, ok := d.GetOk("storage"); ok { | ||
| storageConfiguration = expandStorageConfiguration(v.([]interface{})) | ||
| } | ||
|
|
||
| parameters := dicomservices.DicomService{ | ||
| Identity: i, | ||
| Properties: &dicomservices.DicomServiceProperties{ | ||
| PublicNetworkAccess: pointer.To(dicomservices.PublicNetworkAccessEnabled), | ||
| CorsConfiguration: corsConfiguration, | ||
| EnableDataPartitions: enableDataPartitions, | ||
| Encryption: encryption, | ||
| PublicNetworkAccess: pointer.To(dicomservices.PublicNetworkAccessEnabled), | ||
| StorageConfiguration: storageConfiguration, | ||
| }, | ||
| Location: pointer.To(location.Normalize(d.Get("location").(string))), | ||
| Tags: tags.Expand(t), |
There was a problem hiding this comment.
We typically don't set optional properties in the create payload if they user hasn't specified them, so this would look more like:
parameters := dicomservices.DicomService{
Identity: i,
Properties: &dicomservices.DicomServiceProperties{
PublicNetworkAccess: pointer.To(dicomservices.PublicNetworkAccessEnabled),
},
Location: pointer.To(location.Normalize(d.Get("location").(string))),
Tags: tags.Expand(t),
}
if v, ok := d.GetOk("data_partitions_enabled"); ok {
parameters.Properties.EnableDataPartitions = pointer.To(v.(bool))
}
cors := expandDicomServiceCorsConfiguration(d.Get("cors").([]interface{}))
if cors != nil {
parameters.Properties.CorsConfiguration = cors
}
if v, ok := d.GetOk("encryption_key_url"); ok && v != "" {
parameters.Properties.Encryption = &dicomservices.Encryption{
CustomerManagedKeyEncryption: &dicomservices.EncryptionCustomerManagedKeyEncryption{
KeyEncryptionKeyURL: pointer.To(v.(string)),
},
}
}
storage := expandStorageConfiguration(d.Get("storage").([]interface{}))
if storage != nil {
parameters.Properties.StorageConfiguration = storage
}
|
Hi @stephybun, updated as requested, please help give this another review when you have time, thanks! |
stephybun
left a comment
There was a problem hiding this comment.
Thanks @hqhqhqhqhqhqhqhqhqhqhq LGTM 👍
* Update CHANGELOG.md for #28233 * Update for #28215 * Update CHANGELOG.md for #28279 * Update CHANGELOG.md #28269 * Update CHANGELOG.md #27876 * Update CHANGELOG.md #28069 * Update CHANGELOG.md for #28312 * Update CHANGELOG.md for #28278 * Update CHANGELOG.md #28311 * Update CHANGELOG.md undo 28311 * Update CHANGELOG.md #27874 * Update CHANGELOG.md * Update CHANGELOG for #28352 * Update CHANGELOG.md for #28390 * Update CHANGELOG.md for #28398 * Update CHANGELOG.md for #28425 * Update CHANGELOG.md #28427 * Update CHANGELOG.md #28280 * Update CHANGELOG.md for #28319 * Update CHANGELOG.md #24801 * Update for #28360 #28216 #27830 #28404 #27401 #27122 #27931 #28442 * Update for #28379 * Update CHANGELOG.md for #28281 * Update for #28380 * Update for #27375 * Update for #25695 * Update CHANGELOG.md #27985 * Update CHANGELOG.md - update release date manually until can be scripted * Update CHANGELOG.md revert date change as script available * pre-release script updates --------- Co-authored-by: stephybun <steph@hashicorp.com> Co-authored-by: catriona-m <86247157+catriona-m@users.noreply.github.com> Co-authored-by: Wyatt Fry <wyattfry@gmail.com> Co-authored-by: sreallymatt <106555974+sreallymatt@users.noreply.github.com> Co-authored-by: Matthew Frahry <mbfrahry@gmail.com> Co-authored-by: kt <kt@katbyte.me>
…or new properties (hashicorp#27375) * add several properties for healthcareapi dicomservice * Update healthcare_dicom_data_source.go * fix issues addressed in review * Update healthcare_dicom_data_source.go * Update docs * Update resource & data source & test * fix: extend error message * Update healthcare_dicom_resource_test.go * Update healthcare_dicom_data_source.go * Update healthcare_dicom_data_source.go * fix: update url to URL for API Response * Update healthcare_dicom_resource_test.go * Update healthcare_dicom_resource.go * Update healthcare_dicom_resource.go
* Update CHANGELOG.md for hashicorp#28233 * Update for hashicorp#28215 * Update CHANGELOG.md for hashicorp#28279 * Update CHANGELOG.md hashicorp#28269 * Update CHANGELOG.md hashicorp#27876 * Update CHANGELOG.md hashicorp#28069 * Update CHANGELOG.md for hashicorp#28312 * Update CHANGELOG.md for hashicorp#28278 * Update CHANGELOG.md hashicorp#28311 * Update CHANGELOG.md undo 28311 * Update CHANGELOG.md hashicorp#27874 * Update CHANGELOG.md * Update CHANGELOG for hashicorp#28352 * Update CHANGELOG.md for hashicorp#28390 * Update CHANGELOG.md for hashicorp#28398 * Update CHANGELOG.md for hashicorp#28425 * Update CHANGELOG.md hashicorp#28427 * Update CHANGELOG.md hashicorp#28280 * Update CHANGELOG.md for hashicorp#28319 * Update CHANGELOG.md hashicorp#24801 * Update for hashicorp#28360 hashicorp#28216 hashicorp#27830 hashicorp#28404 hashicorp#27401 hashicorp#27122 hashicorp#27931 hashicorp#28442 * Update for hashicorp#28379 * Update CHANGELOG.md for hashicorp#28281 * Update for hashicorp#28380 * Update for hashicorp#27375 * Update for hashicorp#25695 * Update CHANGELOG.md hashicorp#27985 * Update CHANGELOG.md - update release date manually until can be scripted * Update CHANGELOG.md revert date change as script available * pre-release script updates --------- Co-authored-by: stephybun <steph@hashicorp.com> Co-authored-by: catriona-m <86247157+catriona-m@users.noreply.github.com> Co-authored-by: Wyatt Fry <wyattfry@gmail.com> Co-authored-by: sreallymatt <106555974+sreallymatt@users.noreply.github.com> Co-authored-by: Matthew Frahry <mbfrahry@gmail.com> Co-authored-by: kt <kt@katbyte.me>
|
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
For
azurerm_healthcare_dicom_serviceresource, added properties:cors_configurationencryption_key_urlstorage_configurationdata_partitions_enabledFor
azurerm_healthcare_dicom_servicedata source, added properties:cors_configurationencryption_key_urlstorage_configurationdata_partitions_enabledprovision_stateevent_statePR 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_healthcare_dicom_serviceresource & data source - support for new propertiesThis 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.