`azurerm_healthcare_dicom_service` resource & data source - support for new properties #27375

hqhqhqhqhqhqhqhqhqhqhq · 2024-09-12T17:23:16Z

Community Note

Please vote on this PR by adding a 👍 reaction to the original PR to help the community and maintainers prioritize for review
Please do not leave comments along the lines of "+1", "me too" or "any updates", they generate extra noise for PR followers and do not help prioritize for review

Description

For azurerm_healthcare_dicom_service resource, added properties:

cors_configuration
encryption_key_url
storage_configuration
data_partitions_enabled

For azurerm_healthcare_dicom_service data source, added properties:

cors_configuration
encryption_key_url
storage_configuration
data_partitions_enabled
provision_state
event_state

PR Checklist

I have followed the guidelines in our Contributing Documentation.
I have checked to ensure there aren't other open Pull Requests for the same update/change.
I have checked if my changes close any open issues. If so please include appropriate closing keywords below.
I have updated/added Documentation as required written in a helpful and kind way to assist users that may be unfamiliar with the resource / data source.
I have used a meaningful PR title to help maintainers and other users understand this change and help prevent duplicate work.
For example: “resource_name_here - description of change e.g. adding property new_property_name_here”

Changes to existing Resource / Data Source

I have added an explanation of what my changes do and why I'd like you to include them (This may be covered by linking to an issue above, but may benefit from additional explanation).
I have written new tests for my resource or datasource changes & updated any relevent documentation.
I have successfully run tests with my changes locally. If not, please provide details on testing challenges that prevented you running the tests.
(For changes that include a state migration only). I have manually tested the migration path between relevant versions of the provider.

Testing

My submission includes Test coverage as described in the Contribution Guide and the tests pass. (if this is not possible for any reason, please include details of why you did or could not add test coverage)

Change Log

Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.

azurerm_healthcare_dicom_service resource & data source - support for new properties

This is a (please select all that apply):

Bug Fix
New Feature (ie adding a service, resource, or data source)
Enhancement
Breaking Change

Note

If this PR changes meaningfully during the course of review please update the title and description as required.

katbyte · 2024-09-24T00:15:51Z

internal/services/healthcare/healthcare_dicom_data_source.go

+				Computed: true,
+			},
+
+			"cors_configuration": {


configuration is redundant,

Suggested change

"cors_configuration": {

"cors": {

Thanks, updated.

katbyte · 2024-09-24T00:16:20Z

internal/services/healthcare/healthcare_dicom_data_source.go

+				Computed: true,
+			},
+
+			"storage_configuration": {


ditto

Suggested change

"storage_configuration": {

"storage": {

Thanks, updated.

katbyte · 2024-09-24T00:16:30Z

internal/services/healthcare/healthcare_dicom_resource.go

@@ -115,6 +118,86 @@ func resourceHealthcareApisDicomService() *pluginsdk.Resource {
 				Default:  true,
 			},

+			"cors_configuration": {


Suggested change

"cors_configuration": {

"cors": {

Thanks, updated.

katbyte · 2024-09-24T00:16:48Z

internal/services/healthcare/healthcare_dicom_resource.go

+						},
+						"allowed_headers": {


Suggested change

},

"allowed_headers": {

},

"allowed_headers": {

please add blank lines between properties

Thanks, updated.

katbyte · 2024-09-24T00:17:09Z

internal/services/healthcare/healthcare_dicom_resource.go

+						},
+						"max_age_in_seconds": {
+							Type:     pluginsdk.TypeInt,
+							Optional: true,


can you validate this? i presume -3 is not a valid value? is 0? is 99999999999 ?

Trid it out, can support 0 to 99998 inclusive, added the validatefunc

katbyte · 2024-09-24T00:17:17Z

internal/services/healthcare/healthcare_dicom_resource.go

+				ValidateFunc: validation.IsURLWithHTTPS,
+			},
+
+			"storage_configuration": {


Suggested change

"storage_configuration": {

"storage": {

stephybun

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.

stephybun · 2024-09-30T07:51:12Z

internal/services/healthcare/healthcare_dicom_data_source.go

+			"provision_state": {
+				Type:     pluginsdk.TypeString,
+				Computed: true,
+			},
+
+			"event_state": {
+				Type:     pluginsdk.TypeString,
+				Computed: true,
+			},


These fields contain transient information that isn't meaningful to users, we don't usually expose these in resources or data sources

Suggested change

"provision_state": {

Type: pluginsdk.TypeString,

Computed: true,

},

"event_state": {

Type: pluginsdk.TypeString,

Computed: true,

},

Thanks, this has been removed from the data source and the docs

stephybun · 2024-09-30T07:54:45Z

internal/services/healthcare/healthcare_dicom_resource.go

+			enableDataPartitions := false
+			if props.EnableDataPartitions != nil {
+				enableDataPartitions = pointer.From(props.EnableDataPartitions)
+			}
+			d.Set("data_partitions_enabled", enableDataPartitions)


pointer.From can do the nil check for us, so this can be reduced to

Suggested change

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))

Thanks, updated in the resource & data source

stephybun · 2024-09-30T08:04:39Z

internal/services/healthcare/healthcare_dicom_resource.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{}))
+	}


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:

https://github.com/hashicorp/terraform-provider-azurerm/blob/main/contributing/topics/guide-new-resource.md

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

func resourcePublicIpUpdate(d *pluginsdk.ResourceData, meta interface{}) error {

client := meta.(*clients.Client).Network.PublicIPAddresses

ctx, cancel := timeouts.ForUpdate(meta.(*clients.Client).StopContext, d)

defer cancel()

log.Printf("[INFO] preparing arguments for AzureRM Public IP update.")

id, err := commonids.ParsePublicIPAddressID(d.Id())

if err != nil {

return err

}

existing, err := client.Get(ctx, *id, publicipaddresses.DefaultGetOperationOptions())

if err != nil {

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

}

if existing.Model == nil {

return fmt.Errorf("retrieving %s: `model` was nil", id)

}

if existing.Model.Properties == nil {

return fmt.Errorf("retrieving %s: `properties` was nil", id)

}

payload := existing.Model

if d.HasChange("allocation_method") {

payload.Properties.PublicIPAllocationMethod = pointer.To(publicipaddresses.IPAllocationMethod(d.Get("allocation_method").(string)))

}

if d.HasChange("ddos_protection_mode") {

if payload.Properties.DdosSettings == nil {

payload.Properties.DdosSettings = &publicipaddresses.DdosSettings{}

}

payload.Properties.DdosSettings.ProtectionMode = pointer.To(publicipaddresses.DdosSettingsProtectionMode(d.Get("ddos_protection_mode").(string)))

}

if d.HasChange("ddos_protection_plan_id") {

if !strings.EqualFold(string(*payload.Properties.DdosSettings.ProtectionMode), "enabled") {

return fmt.Errorf("ddos protection plan id can only be set when ddos protection is enabled")

}

if payload.Properties.DdosSettings == nil {

payload.Properties.DdosSettings = &publicipaddresses.DdosSettings{}

}

payload.Properties.DdosSettings.DdosProtectionPlan = &publicipaddresses.SubResource{

Id: pointer.To(d.Get("ddos_protection_plan_id").(string)),

}

}

if d.HasChange("idle_timeout_in_minutes") {

payload.Properties.IdleTimeoutInMinutes = utils.Int64(int64(d.Get("idle_timeout_in_minutes").(int)))

}

if d.HasChange("domain_name_label") {

if payload.Properties.DnsSettings == nil {

payload.Properties.DnsSettings = &publicipaddresses.PublicIPAddressDnsSettings{}

}

payload.Properties.DnsSettings.DomainNameLabel = utils.String(d.Get("domain_name_label").(string))

}

if d.HasChange("reverse_fqdn") {

if payload.Properties.DnsSettings == nil {

payload.Properties.DnsSettings = &publicipaddresses.PublicIPAddressDnsSettings{}

}

payload.Properties.DnsSettings.ReverseFqdn = utils.String(d.Get("reverse_fqdn").(string))

}

if d.HasChanges("tags") {

payload.Tags = tags.Expand(d.Get("tags").(map[string]interface{}))

}

if err = client.CreateOrUpdateThenPoll(ctx, *id, *payload); err != nil {

Got it, have updated the update function in the resource to this format.

stephybun · 2024-09-30T08:10:24Z

internal/services/healthcare/healthcare_dicom_resource.go

+	if configuration.FileSystemName != nil {
+		result["file_system_name"] = *configuration.FileSystemName
+	}


We could use pointer.From here for the nil check

Suggested change

if configuration.FileSystemName != nil {

result["file_system_name"] = *configuration.FileSystemName

}

result["file_system_name"] = pointer.From(configuration.FileSystemName)

Thanks, updated, the flatten function (used by the resource & data source)

stephybun · 2024-09-30T08:16:40Z

internal/services/healthcare/healthcare_dicom_resource.go

+	if configuration.StorageResourceId != nil {
+		result["storage_account_id"] = *configuration.StorageResourceId
+	}


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

Suggested change

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()

}

Thanks, updated the flatten function (used by the resource & data source), the function now also returns error to the calling function

stephybun · 2024-09-30T08:38:58Z

website/docs/d/healthcare_dicom.html.markdown

@@ -51,6 +63,29 @@ An `authentication` supports the following:

 * `audience` - The intended audience to receive authentication tokens for the service. The default value is <https://dicom.azurehealthcareapis.azure.com>

+---
+
+A `cors` supports the following:


For data sources we say a block exports rather than supports since these aren't arguments that can be supplied

Suggested change

A `cors` supports the following:

A `cors` exports the following:

stephybun · 2024-09-30T08:39:08Z

website/docs/d/healthcare_dicom.html.markdown

+
+---
+
+A `storage` block supports the following:


Suggested change

A `storage` block supports the following:

A `storage` block exports the following:

stephybun · 2024-09-30T08:40:13Z

website/docs/d/healthcare_dicom.html.markdown

+
+* `file_system_name` - The filesystem name of connected storage account.
+
+* `storage_account_id` - The resource id of connected storage account.


Suggested change

* `storage_account_id` - The resource id of connected storage account.

* `storage_account_id` - The resource ID of connected storage account.

Updated the doc with required changes & other parts where it is id rather than ID

stephybun · 2024-09-30T08:41:23Z

website/docs/r/healthcare_dicom.html.markdown

+
+* `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.


Suggested change

* `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.

stephybun · 2024-09-30T08:43:21Z

website/docs/r/healthcare_dicom.html.markdown

+
+* `storage_account_id` - (Required) The resource id of connected storage account. Changing this forces a new Healthcare DICOM Service to be created.
+
+~> **Note:** The `is_hns_enabled` needs to be set to `true` for the storage account to be used with the Healthcare DICOM Service.


If this is the case then shouldn't this be set in the complete test config?

This property is currently set for azurerm_storage_account resource in the complete test

hqhqhqhqhqhqhqhqhqhqhq · 2024-09-30T14:23:07Z

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.

@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

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.

…s-properties

hqhqhqhqhqhqhqhqhqhqhq added 2 commits September 13, 2024 02:19

add several properties for healthcareapi dicomservice

89385bf

Update healthcare_dicom_data_source.go

a7230a0

github-actions bot added documentation service/healthcare size/XL labels Sep 12, 2024

hqhqhqhqhqhqhqhqhqhqhq marked this pull request as ready for review September 23, 2024 23:16

katbyte requested changes Sep 24, 2024

View reviewed changes

github-actions bot added the waiting-response label Sep 24, 2024

fix issues addressed in review

8605ae6

github-actions bot removed the waiting-response label Sep 24, 2024

stephybun requested changes Sep 30, 2024

View reviewed changes

github-actions bot added the waiting-response label Sep 30, 2024

hqhqhqhqhqhqhqhqhqhqhq added 2 commits September 30, 2024 23:04

Update healthcare_dicom_data_source.go

dbc59ff

Update docs

3abe99f

github-actions bot removed the waiting-response label Sep 30, 2024

hqhqhqhqhqhqhqhqhqhqhq added 3 commits September 30, 2024 23:22

Update resource & data source & test

b99e007

fix: extend error message

4506f14

Update healthcare_dicom_resource_test.go

fff7a9e

rcskosir added the enhancement label Oct 14, 2024

stephybun requested changes Oct 31, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/main' into feat/healthcare_api…

005ae2a

…s-properties

hqhqhqhqhqhqhqhqhqhqhq requested a review from a team as a code owner November 6, 2024 04:11

Update healthcare_dicom_data_source.go

06a0e6c

github-actions bot added the waiting-response label Nov 6, 2024

Update healthcare_dicom_data_source.go

e43efe8

github-actions bot added waiting-response and removed waiting-response labels Nov 6, 2024

fix: update url to URL for API Response

c967bc5

github-actions bot removed the waiting-response label Nov 8, 2024

Update healthcare_dicom_resource_test.go

a6b433a

	func resourcePublicIpUpdate(d *pluginsdk.ResourceData, meta interface{}) error {
	client := meta.(*clients.Client).Network.PublicIPAddresses
	ctx, cancel := timeouts.ForUpdate(meta.(*clients.Client).StopContext, d)
	defer cancel()

	log.Printf("[INFO] preparing arguments for AzureRM Public IP update.")

	id, err := commonids.ParsePublicIPAddressID(d.Id())
	if err != nil {
	return err
	}

	existing, err := client.Get(ctx, *id, publicipaddresses.DefaultGetOperationOptions())
	if err != nil {
	return fmt.Errorf("retrieving %s: %+v", id, err)
	}

	if existing.Model == nil {
	return fmt.Errorf("retrieving %s: `model` was nil", id)
	}
	if existing.Model.Properties == nil {
	return fmt.Errorf("retrieving %s: `properties` was nil", id)
	}

	payload := existing.Model

	if d.HasChange("allocation_method") {
	payload.Properties.PublicIPAllocationMethod = pointer.To(publicipaddresses.IPAllocationMethod(d.Get("allocation_method").(string)))
	}

	if d.HasChange("ddos_protection_mode") {
	if payload.Properties.DdosSettings == nil {
	payload.Properties.DdosSettings = &publicipaddresses.DdosSettings{}
	}
	payload.Properties.DdosSettings.ProtectionMode = pointer.To(publicipaddresses.DdosSettingsProtectionMode(d.Get("ddos_protection_mode").(string)))
	}

	if d.HasChange("ddos_protection_plan_id") {
	if !strings.EqualFold(string(*payload.Properties.DdosSettings.ProtectionMode), "enabled") {
	return fmt.Errorf("ddos protection plan id can only be set when ddos protection is enabled")
	}
	if payload.Properties.DdosSettings == nil {
	payload.Properties.DdosSettings = &publicipaddresses.DdosSettings{}
	}
	payload.Properties.DdosSettings.DdosProtectionPlan = &publicipaddresses.SubResource{
	Id: pointer.To(d.Get("ddos_protection_plan_id").(string)),
	}
	}

	if d.HasChange("idle_timeout_in_minutes") {
	payload.Properties.IdleTimeoutInMinutes = utils.Int64(int64(d.Get("idle_timeout_in_minutes").(int)))
	}

	if d.HasChange("domain_name_label") {
	if payload.Properties.DnsSettings == nil {
	payload.Properties.DnsSettings = &publicipaddresses.PublicIPAddressDnsSettings{}
	}
	payload.Properties.DnsSettings.DomainNameLabel = utils.String(d.Get("domain_name_label").(string))
	}

	if d.HasChange("reverse_fqdn") {
	if payload.Properties.DnsSettings == nil {
	payload.Properties.DnsSettings = &publicipaddresses.PublicIPAddressDnsSettings{}
	}
	payload.Properties.DnsSettings.ReverseFqdn = utils.String(d.Get("reverse_fqdn").(string))
	}

	if d.HasChanges("tags") {
	payload.Tags = tags.Expand(d.Get("tags").(map[string]interface{}))
	}

	if err = client.CreateOrUpdateThenPoll(ctx, id, payload); err != nil {

-	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()
+	}

	A `cors` supports the following:
	A `cors` exports the following:

	A `storage` block supports the following:
	A `storage` block exports the following:


		* `file_system_name` - The filesystem name of connected storage account.

		* `storage_account_id` - The resource id of connected storage account.


		* `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.


		* `storage_account_id` - (Required) The resource id of connected storage account. Changing this forces a new Healthcare DICOM Service to be created.

		~> Note: The `is_hns_enabled` needs to be set to `true` for the storage account to be used with the Healthcare DICOM Service.

azurerm_healthcare_dicom_service resource & data source - support for new properties #27375

Are you sure you want to change the base?

azurerm_healthcare_dicom_service resource & data source - support for new properties #27375

Conversation

hqhqhqhqhqhqhqhqhqhqhq commented Sep 12, 2024

Community Note

Description

PR Checklist

Changes to existing Resource / Data Source

Testing

Change Log

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephybun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hqhqhqhqhqhqhqhqhqhqhq commented Sep 30, 2024

stephybun left a comment

Choose a reason for hiding this comment

`azurerm_healthcare_dicom_service` resource & data source - support for new properties #27375

`azurerm_healthcare_dicom_service` resource & data source - support for new properties #27375