Bug Fix - Edit Volume ID inside a VolumeGroup results the VG gets destoried

Author: Terraform Team Automation

internal/integrationtest/core_volume_group_test.go

@@ -50,6 +50,7 @@ var (
 		"backup_policy_id":    acctest.Representation{RepType: acctest.Optional, Create: `${data.oci_core_volume_backup_policies.test_volume_user_defined_backup_policies.volume_backup_policies.0.id}`},
 		"defined_tags":        acctest.Representation{RepType: acctest.Optional, Create: `${map("${oci_identity_tag_namespace.tag-namespace1.name}.${oci_identity_tag.tag1.name}", "value")}`, Update: `${map("${oci_identity_tag_namespace.tag-namespace1.name}.${oci_identity_tag.tag1.name}", "updatedValue")}`},
 		"display_name":        acctest.Representation{RepType: acctest.Optional, Create: `displayName`, Update: `displayName2`},
+		"volume_ids":          acctest.Representation{RepType: acctest.Optional, Create: nil, Update: []string{`${oci_core_volume.source_volume_list.*.id[0]}`}},
 		"freeform_tags":       acctest.Representation{RepType: acctest.Optional, Create: map[string]string{"Department": "Finance"}, Update: map[string]string{"Department": "Accounting"}},
 	}
 	CoreVolumeGroupSourceDetailsRepresentation = map[string]interface{}{
@@ -227,7 +228,7 @@ func TestCoreVolumeGroupResource_basic(t *testing.T) {
 				resource.TestCheckResourceAttr(resourceName, "source_details.#", "1"),
 				resource.TestCheckResourceAttr(resourceName, "source_details.0.type", "volumeIds"),
 				resource.TestCheckResourceAttrSet(resourceName, "time_created"),
-				resource.TestCheckResourceAttr(resourceName, "volume_ids.#", "2"),
+				resource.TestCheckResourceAttr(resourceName, "volume_ids.#", "1"),
 
 				func(s *terraform.State) (err error) {
 					resId2, err = acctest.FromInstanceState(s, resourceName, "id")

internal/service/core/core_volume_group_resource.go

@@ -179,8 +179,12 @@ func CoreVolumeGroupResource() *schema.Resource {
 				Computed: true,
 			},
 			"volume_ids": {
-				Type:     schema.TypeList,
-				Computed: true,
+				Type:             schema.TypeList,
+				Optional:         true,
+				Computed:         true,
+				MaxItems:         64,
+				MinItems:         0,
+				DiffSuppressFunc: tfresource.ListEqualIgnoreOrderSuppressDiff,
 				Elem: &schema.Schema{
 					Type: schema.TypeString,
 				},
@@ -303,6 +307,12 @@ func (s *CoreVolumeGroupResourceCrud) Create() error {
 		}
 	}
 
+	if volumeIds, ok := s.D.GetOkExists("volume_ids"); ok {
+		if tmp := volumeIds.([]interface{}); len(tmp) > 0 {
+			return fmt.Errorf("volume_ids under resource is not supported during creation")
+		}
+	}
+
 	if volumeGroupReplicas, ok := s.D.GetOkExists("volume_group_replicas"); ok {
 		interfaces := volumeGroupReplicas.([]interface{})
 		tmp := make([]oci_core.VolumeGroupReplicaDetails, len(interfaces))
@@ -403,14 +413,17 @@ func (s *CoreVolumeGroupResourceCrud) Update() error {
 		}
 	}
 
+	// Indicate if current update is disabling volume group replica
 	disableReplication := false
 	if volumeGroupReplicasDeletion, ok := s.D.GetOkExists("volume_group_replicas_deletion"); ok {
 		disableReplication = volumeGroupReplicasDeletion.(bool)
 		if disableReplication == true {
 			request.VolumeGroupReplicas = []oci_core.VolumeGroupReplicaDetails{}
 		}
+		disableReplication = s.D.HasChange("volume_group_replicas_deletion") && disableReplication
 	}
 
+	// We don't want customers to disable volume group replica and update volume list at the same time
 	if !disableReplication {
 		if volumeIds, ok := s.D.GetOkExists("volume_ids"); ok {
 			interfaces := volumeIds.([]interface{})

internal/tfresource/crud_helpers.go

@@ -527,6 +527,38 @@ func loadBalancersSuppressDiff(d schemaResourceData) bool {
 func EqualIgnoreCaseSuppressDiff(key string, old string, new string, d *schema.ResourceData) bool {
 	return strings.EqualFold(old, new)
 }
+func ListEqualIgnoreOrderSuppressDiff(key string, old string, new string, d *schema.ResourceData) bool {
+	return listEqualIgnoreOrderSuppressDiff(key, d)
+}
+
+func listEqualIgnoreOrderSuppressDiff(key string, d schemaResourceData) bool {
+	// Take only the field name, key might be field.#
+	oldRaw, newRaw := d.GetChange(strings.Split(key, ".")[0])
+	if newRaw == nil || oldRaw == nil {
+		return false
+	}
+	oldList := oldRaw.([]interface{})
+	newList := newRaw.([]interface{})
+
+	if len(oldList) != len(newList) {
+		return false
+	}
+	tmp1 := make([]string, len(oldList))
+	tmp2 := make([]string, len(newList))
+
+	for i := range oldList {
+		tmp1[i] = oldList[i].(string)
+		tmp2[i] = newList[i].(string)
+	}
+	sort.Strings(tmp1)
+	sort.Strings(tmp2)
+	for i := range oldList {
+		if tmp1[i] != tmp2[i] {
+			return false
+		}
+	}
+	return true
+}
 
 func FieldDeprecatedAndAvoidReferences(deprecatedFieldName string) string {
 	return fmt.Sprintf("The '%s' field has been deprecated and may be removed in a future version. Do not use this field.", deprecatedFieldName)

internal/tfresource/crud_helpers_test.go

@@ -248,6 +248,15 @@ func (d *mockResourceData) GetChange(_ string) (interface{}, interface{}) {
 	if d.state == "3" {
 		return []interface{}{map[string]interface{}{"load_balancer_id": "1"}}, []interface{}{map[string]interface{}{"load_balancer_id": "2"}}
 	}
+	if d.state == "4" {
+		return []interface{}{"foo", "bar"}, []interface{}{"bar", "foo"}
+	}
+	if d.state == "5" {
+		return []interface{}{"foo", "foo", "bar"}, []interface{}{"bar", "bar", "foo"}
+	}
+	if d.state == "6" {
+		return []interface{}{"foo", "bar"}, []interface{}{"foo"}
+	}
 	return []interface{}{map[string]interface{}{"load_balancer_id": "1"}}, []interface{}{map[string]interface{}{"load_balancer_id": "1"}}
 }
 
@@ -613,6 +622,47 @@ func TestUnitGetTimeoutDuration(t *testing.T) {
 	}
 }
 
+func TestListEqualIgnoreOrderSuppressDiff(t *testing.T) {
+	type args struct {
+		d *mockResourceData
+	}
+	type testFormat struct {
+		name   string
+		args   args
+		output bool
+	}
+	changeReqResourceData := func(k string) *mockResourceData {
+		reqResourceData := &mockResourceData{
+			state: k,
+		}
+		return reqResourceData
+	}
+	tests := []testFormat{
+		{
+			name:   "Test same lists with diff order",
+			args:   args{d: changeReqResourceData("4")},
+			output: true,
+		},
+		{
+			name:   "Test diff lists with duplicated elements",
+			args:   args{d: changeReqResourceData("5")},
+			output: false,
+		},
+		{
+			name:   "Test diff lists with diff length",
+			args:   args{d: changeReqResourceData("6")},
+			output: false,
+		},
+	}
+	for _, test := range tests {
+		t.Logf("Running %s", test.name)
+		t.Logf("Running %s", fmt.Sprint(test.args.d))
+		if res := listEqualIgnoreOrderSuppressDiff("volume_id", test.args.d); res != test.output {
+			t.Errorf("Output %t not equal to expected %t", res, test.output)
+		}
+	}
+}
+
 func TestUnitGenericMapToJsonMap(t *testing.T) {
 	type args struct {
 		genericMap map[string]interface{}