From 867a17e581238f37e9f542ad87ca0b96f1721abd Mon Sep 17 00:00:00 2001 From: Ferdinando Formica Date: Fri, 19 Apr 2024 15:20:42 +0100 Subject: [PATCH 1/2] Use TypeSet for notifications so that a change in order won't cause an update --- ns1/resource_notifylist.go | 25 +++++++------ ns1/resource_notifylist_test.go | 62 +++++++++++++++------------------ 2 files changed, 42 insertions(+), 45 deletions(-) diff --git a/ns1/resource_notifylist.go b/ns1/resource_notifylist.go index e4f17111..754be33e 100644 --- a/ns1/resource_notifylist.go +++ b/ns1/resource_notifylist.go @@ -18,7 +18,7 @@ func notifyListResource() *schema.Resource { Required: true, }, "notifications": { - Type: schema.TypeList, + Type: schema.TypeSet, Optional: true, Elem: &schema.Resource{ Schema: map[string]*schema.Schema{ @@ -66,39 +66,40 @@ func notifyListToResourceData(d *schema.ResourceData, nl *monitor.NotifyList) er func resourceDataToNotifyList(nl *monitor.NotifyList, d *schema.ResourceData) error { nl.ID = d.Id() - if rawNotifications := d.Get("notifications").([]interface{}); len(rawNotifications) > 0 { - ns := make([]*monitor.Notification, len(rawNotifications)) - for i, notificationRaw := range rawNotifications { + index := 0 + if rawNotifications := d.Get("notifications").(*schema.Set); rawNotifications.Len() > 0 { + ns := make([]*monitor.Notification, rawNotifications.Len()) + for _, notificationRaw := range rawNotifications.List() { ni := notificationRaw.(map[string]interface{}) config := ni["config"].(map[string]interface{}) - if config != nil { + if len(config) > 0 { switch ni["type"].(string) { case "email": email := config["email"] if email != nil { - ns[i] = monitor.NewEmailNotification(email.(string)) + ns[index] = monitor.NewEmailNotification(email.(string)) } else { return fmt.Errorf("wrong config for email expected email field into config") } case "datafeed": sourceId := config["sourceid"] if sourceId != nil { - ns[i] = monitor.NewFeedNotification(sourceId.(string)) + ns[index] = monitor.NewFeedNotification(sourceId.(string)) } else { return fmt.Errorf("wrong config for datafeed expected sourceid field into config") } case "webhook": url := config["url"] if url != nil { - ns[i] = monitor.NewWebNotification(url.(string), nil) + ns[index] = monitor.NewWebNotification(url.(string), nil) } else { return fmt.Errorf("wrong config for webhook expected url field into config") } case "pagerduty": serviceKey := config["service_key"] if serviceKey != nil { - ns[i] = monitor.NewPagerDutyNotification(serviceKey.(string)) + ns[index] = monitor.NewPagerDutyNotification(serviceKey.(string)) } else { return fmt.Errorf("wrong config for pagerduty expected serviceKey field into config") } @@ -107,16 +108,18 @@ func resourceDataToNotifyList(nl *monitor.NotifyList, d *schema.ResourceData) er username := config["username"] channel := config["channel"] if url != nil && username != nil && channel != nil { - ns[i] = monitor.NewSlackNotification(url.(string), username.(string), channel.(string)) + ns[index] = monitor.NewSlackNotification(url.(string), username.(string), channel.(string)) } else { return fmt.Errorf("wrong config for slack expected url, username and channel fields into config") } default: return fmt.Errorf("%s is not a valid notifier type", ni["type"]) } + // Only increase if not empty + index++ } } - nl.Notifications = ns + nl.Notifications = ns[:index] } return nil } diff --git a/ns1/resource_notifylist_test.go b/ns1/resource_notifylist_test.go index f4f1687a..feb15b9e 100644 --- a/ns1/resource_notifylist_test.go +++ b/ns1/resource_notifylist_test.go @@ -72,19 +72,19 @@ func TestAccNotifyList_multiple(t *testing.T) { Check: resource.ComposeTestCheckFunc( testAccCheckNotifyListExists("ns1_notifylist.test_multiple", &nl), testAccCheckNotifyListName(&nl, "terraform test multiple"), + testAccCheckNotifyTypeOrder(&nl, "pagerduty", "webhook"), + ), + }, + // Despite the change in order the object is the same: we want to make sure + // that it's not actually modified and the order stays the same + { + Config: testAccNotifyListMultipleDifferentOrder, + Check: resource.ComposeTestCheckFunc( + testAccCheckNotifyListExists("ns1_notifylist.test_multiple", &nl), + testAccCheckNotifyListName(&nl, "terraform test multiple different order"), + testAccCheckNotifyTypeOrder(&nl, "pagerduty", "webhook"), ), }, - // This fails because the schema.TypeList is ordered. We want to switch - // this to schema.TypeSet but cannot due to SDK issue #652 / #895, fix for - // which is waiting for review, see - // https://github.com/hashicorp/terraform-plugin-sdk/pull/1042 - // { - // Config: testAccNotifyListMultipleDifferentOrder , - // Check: resource.ComposeTestCheckFunc( - // testAccCheckNotifyListExists("ns1_notifylist.test_multiple2", &nl), - // testAccCheckNotifyListName(&nl, "terraform test multiple2"), - // ), - // }, { ResourceName: "ns1_notifylist.test_multiple", ImportState: true, @@ -157,27 +157,6 @@ func TestAccNotifyList_ManualDelete(t *testing.T) { }) } -func testAccCheckNotifyListState(key, value string) resource.TestCheckFunc { - return func(s *terraform.State) error { - rs, ok := s.RootModule().Resources["ns1_notifylist.test"] - if !ok { - return fmt.Errorf("not found: %s", "ns1_notifylist.test") - } - - if rs.Primary.ID == "" { - return fmt.Errorf("no ID is set") - } - - p := rs.Primary - if p.Attributes[key] != value { - return fmt.Errorf( - "%s != %s (actual: %s)", key, value, p.Attributes[key]) - } - - return nil - } -} - func testAccCheckNotifyListExists(n string, nl *monitor.NotifyList) resource.TestCheckFunc { return func(s *terraform.State) error { rs, ok := s.RootModule().Resources[n] @@ -236,6 +215,21 @@ func testAccCheckNotifyListName(nl *monitor.NotifyList, expected string) resourc } } +func testAccCheckNotifyTypeOrder(nl *monitor.NotifyList, type1, type2 string) resource.TestCheckFunc { + return func(s *terraform.State) error { + if len(nl.Notifications) < 2 { + return fmt.Errorf("nl.Name: got: %#v want: 2 elements", nl.Notifications) + } + if nl.Notifications[0].Type != type1 { + return fmt.Errorf("nl.Name: got: %#v want: %#v", nl.Notifications[0].Type, type1) + } + if nl.Notifications[1].Type != type2 { + return fmt.Errorf("nl.Name: got: %#v want: %#v", nl.Notifications[1].Type, type2) + } + return nil + } +} + // Simulate a manual deletion of a notify list. func testAccManualDeleteNotifyList(nl *monitor.NotifyList) func() { return func() { @@ -315,8 +309,8 @@ resource "ns1_notifylist" "test_multiple" { ` const testAccNotifyListMultipleDifferentOrder = ` -resource "ns1_notifylist" "test_multiple2" { - name = "terraform test multiple2" +resource "ns1_notifylist" "test_multiple" { + name = "terraform test multiple different order" notifications { type = "webhook" config = { From 3bcca07b25a8c8174cd693d4dc8795ce19083ba6 Mon Sep 17 00:00:00 2001 From: Ferdinando Formica Date: Mon, 22 Apr 2024 10:20:01 +0100 Subject: [PATCH 2/2] Change index to append --- ns1/resource_notifylist.go | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/ns1/resource_notifylist.go b/ns1/resource_notifylist.go index 754be33e..e493db27 100644 --- a/ns1/resource_notifylist.go +++ b/ns1/resource_notifylist.go @@ -66,9 +66,8 @@ func notifyListToResourceData(d *schema.ResourceData, nl *monitor.NotifyList) er func resourceDataToNotifyList(nl *monitor.NotifyList, d *schema.ResourceData) error { nl.ID = d.Id() - index := 0 if rawNotifications := d.Get("notifications").(*schema.Set); rawNotifications.Len() > 0 { - ns := make([]*monitor.Notification, rawNotifications.Len()) + ns := make([]*monitor.Notification, 0, rawNotifications.Len()) for _, notificationRaw := range rawNotifications.List() { ni := notificationRaw.(map[string]interface{}) config := ni["config"].(map[string]interface{}) @@ -78,28 +77,28 @@ func resourceDataToNotifyList(nl *monitor.NotifyList, d *schema.ResourceData) er case "email": email := config["email"] if email != nil { - ns[index] = monitor.NewEmailNotification(email.(string)) + ns = append(ns, monitor.NewEmailNotification(email.(string))) } else { return fmt.Errorf("wrong config for email expected email field into config") } case "datafeed": sourceId := config["sourceid"] if sourceId != nil { - ns[index] = monitor.NewFeedNotification(sourceId.(string)) + ns = append(ns, monitor.NewFeedNotification(sourceId.(string))) } else { return fmt.Errorf("wrong config for datafeed expected sourceid field into config") } case "webhook": url := config["url"] if url != nil { - ns[index] = monitor.NewWebNotification(url.(string), nil) + ns = append(ns, monitor.NewWebNotification(url.(string), nil)) } else { return fmt.Errorf("wrong config for webhook expected url field into config") } case "pagerduty": serviceKey := config["service_key"] if serviceKey != nil { - ns[index] = monitor.NewPagerDutyNotification(serviceKey.(string)) + ns = append(ns, monitor.NewPagerDutyNotification(serviceKey.(string))) } else { return fmt.Errorf("wrong config for pagerduty expected serviceKey field into config") } @@ -108,18 +107,16 @@ func resourceDataToNotifyList(nl *monitor.NotifyList, d *schema.ResourceData) er username := config["username"] channel := config["channel"] if url != nil && username != nil && channel != nil { - ns[index] = monitor.NewSlackNotification(url.(string), username.(string), channel.(string)) + ns = append(ns, monitor.NewSlackNotification(url.(string), username.(string), channel.(string))) } else { return fmt.Errorf("wrong config for slack expected url, username and channel fields into config") } default: return fmt.Errorf("%s is not a valid notifier type", ni["type"]) } - // Only increase if not empty - index++ } } - nl.Notifications = ns[:index] + nl.Notifications = ns } return nil }