Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use TypeSet for notifications so that a change in order won't cause an update #311

Merged
merged 2 commits into from
Apr 22, 2024
Merged

Use TypeSet for notifications so that a change in order won't cause an update #311

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
867a17e
Use TypeSet for notifications so that a change in order won't cause a…
fformica Apr 19, 2024
3bcca07
Change index to append
fformica Apr 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 14 additions & 11 deletions ns1/resource_notifylist.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down Expand Up @@ -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())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if you set this to

ns := make([]*monitor.Notification, 0, rawNotifications.Len())

then you don't need to define the index separately from the for loop. Then change the additions to the slice to appends, e.g.

ns[index] = monitor.NewEmailNotification(email.(string))
// change to:
ns = append(ns, monitor.NewEmailNotification(email.(string)))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Going to test it next week

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")
}
Expand All @@ -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
}
Expand Down
62 changes: 28 additions & 34 deletions ns1/resource_notifylist_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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]
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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 = {
Expand Down
Loading