azurerm_shared_image_version - target_region.x.storage_account_type is now defaulted and multiple target_regions can be added/removed

[neil-yechenwei]

fixes #5569

Existing test case already covers this scenario. So no more test case needs to be added.

[mbfrahry]

Hey @neil-yechenwei, this fixes part of your linked issue but we are still missing a piece. We'll want to remove the ForceNew: true and add a CustomizeDiff that checks to see if the old/new key being passed in aren't empty and aren't the same. If they aren't the same then we have to ForceNew the resource.

[neil-yechenwei]

@mbfrahry , "ValidateFunc" would check if the input value is empty. So I think we don't need to add additional logic to validate, right? And "ForceNew: true" would check if old value and new value are different. If it's different, it would recreate the resource. So may I know why I have to remove "ForceNew: true" and add "CustomizeDiff" to do the same thing?

[mbfrahry]

Hey @neil-yechenwei, no problem with the extra explanation. The reason I call out using a separate CustomizeDiff is because the current iteration will always force a new resource to be created whenever a new target_region is added. This is due to the ForceNew: true tag being associated with storage_account_type. In practice, we should be able to add and remove a target_region as we please but this isn't the case with the current version. By adding a CustomizeDiff we can have a finer granularity on when we ForceNew the resource. We can check the old key and the new key to determine what we do with the resource. If either is empty, we know that we're removing or adding a target_region and can proceed without recreating the resource. If both are not empty, we now need to recreate the resource since that is not allowed by the api. The best approach may actually be removing the target_region when the previous use case occurs and adding a new target_region with the new storage_account_type.

[neil-yechenwei]

Hi @mbfrahry ,

I collated the rules.

1. The resource should be updated when Target_region is added from none at first creation
2. The resource should be updated when already exists multiple target_region and then added or removed target_region.
3. The resource should be updated when name or regional_replica_count is updated
4. The resource should be recreated when storage_account_type is updated.
5. We cannot remove all target_region since it’s required property. So we don’t need to take care about this case.

Based on above rules, then I tried to remove forcenew attribute and use CustomizeDiff with below code but "old value" and "new value" are same for TypeSet while updated value of storage_account_type in tfconfig and ran "tf apply" at second time so that I cannot compare the value of "storage_account_type". Maybe I missed something. Could you give me some suggestion? Thanks.

"target_region": {
				Type:     schema.TypeSet,
				Required: true,
				Elem: &schema.Resource{
					Schema: map[string]*schema.Schema{
						"name": {
							Type:             schema.TypeString,
							Required:         true,
							StateFunc:        location.StateFunc,
							DiffSuppressFunc: location.DiffSuppressFunc,
						},

						"regional_replica_count": {
							Type:     schema.TypeInt,
							Required: true,
						},

						"storage_account_type": {
							Type:     schema.TypeString,
							Optional: true,
							ValidateFunc: validation.StringInSlice([]string{
								string(compute.StorageAccountTypeStandardLRS),
								string(compute.StorageAccountTypeStandardZRS),
							}, false),
							Default: string(compute.StorageAccountTypeStandardLRS),
						},
					},
				},
			},

CustomizeDiff: customdiff.Sequence(
			customdiff.ForceNewIfChange("target_region", func(old, new, meta interface{}) bool {
				oldTargetRegions := old.(*schema.Set).List()
				newTargetRegions := new.(*schema.Set).List()

				// As `target_region` is required property, so `new` cannot be set as empty. So we don't need to check here.
				if len(oldTargetRegions) == 0 {
					return true
				}

				for _, oldTargetRegion := range oldTargetRegions {
					oldTargetRegionBlock := oldTargetRegion.(map[string]interface{})
					for _, newTargetRegion := range newTargetRegions {
						newTargetRegionBlock := newTargetRegion.(map[string]interface{})
						if azure.NormalizeLocation(oldTargetRegionBlock["name"]) == azure.NormalizeLocation(newTargetRegionBlock["name"]) {
							if oldTargetRegionBlock["storage_account_type"] != newTargetRegionBlock["storage_account_type"] {
								return true
							}
						}
					}
				}

				return true
			}),
		),

[mbfrahry]

Ok, I've just spent quite a bit of time playing with this and you're right that you won't get the correct behavior with CustomizeDiff.

I will note that your implementation will always ForceNew the resource if the number of target_regions change. We'd want to return false at the end of the function to show that we don't need to ForceNew the resource if things haven't changed.

CustomizeDiff won't work in a Set where a specific attribute needs to be ForceNew because we can't get to the key of the Set where the changed storage_account_type lives. The implementation you provided only triggers when we add or remove a target_region but we only care about when a storage_account_type changes.

So we have two options: We can just remove the ForceNew entirely from storage_account_type and rely on the api error that you can't update that field or we can do some complicated logic in the provider to detect when storage_account_type is changing and update the resource by removing the affected target_region and then re-adding it with the new storage_account_type

[neil-yechenwei]

@mbfrahry , thanks for your suggestion. I think using CustomizeDiff in a Set should be a common requirement in terraform. So I prefer your first option since I assume it should be supported/implemented in the future.

[mbfrahry]

LGTM!

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!