From c72913dad8e750e58f6c1bc4a140d4901d1ffacf Mon Sep 17 00:00:00 2001 From: Tom Bamford Date: Tue, 21 Feb 2023 21:50:56 +0200 Subject: [PATCH 1/2] azuread_group: only set additional fields on create, when they are found in configuration --- internal/services/groups/group_resource.go | 16 ++++++++-------- .../services/groups/group_resource_test.go | 18 +++++++++--------- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/internal/services/groups/group_resource.go b/internal/services/groups/group_resource.go index 0b448cfd4e..51662f5b0a 100644 --- a/internal/services/groups/group_resource.go +++ b/internal/services/groups/group_resource.go @@ -700,12 +700,12 @@ func groupResourceCreate(ctx context.Context, d *schema.ResourceData, meta inter // See https://docs.microsoft.com/en-us/graph/known-issues#groups // AllowExternalSenders can only be set in its own PATCH request; including other properties returns a 400 - if allowExternalSenders := d.Get("external_senders_allowed").(bool); allowExternalSenders { + if allowExternalSenders, ok := d.GetOkExists("external_senders_allowed"); ok { //nolint:staticcheck if _, err := client.Update(ctx, msgraph.Group{ DirectoryObject: msgraph.DirectoryObject{ Id: group.ID(), }, - AllowExternalSenders: utils.Bool(allowExternalSenders), + AllowExternalSenders: utils.Bool(allowExternalSenders.(bool)), }); err != nil { return tf.ErrorDiagF(err, "Failed to set `external_senders_allowed` for group with object ID %q", *group.ID()) } @@ -724,12 +724,12 @@ func groupResourceCreate(ctx context.Context, d *schema.ResourceData, meta inter } // AutoSubscribeNewMembers can only be set in its own PATCH request; including other properties returns a 400 - if autoSubscribeNewMembers := d.Get("auto_subscribe_new_members").(bool); autoSubscribeNewMembers { + if autoSubscribeNewMembers, ok := d.GetOkExists("auto_subscribe_new_members"); ok { //nolint:staticcheck if _, err := client.Update(ctx, msgraph.Group{ DirectoryObject: msgraph.DirectoryObject{ Id: group.ID(), }, - AutoSubscribeNewMembers: utils.Bool(autoSubscribeNewMembers), + AutoSubscribeNewMembers: utils.Bool(autoSubscribeNewMembers.(bool)), }); err != nil { return tf.ErrorDiagF(err, "Failed to set `auto_subscribe_new_members` for group with object ID %q", *group.ID()) } @@ -748,12 +748,12 @@ func groupResourceCreate(ctx context.Context, d *schema.ResourceData, meta inter } // HideFromAddressLists can only be set in its own PATCH request; including other properties returns a 400 - if hideFromAddressList := d.Get("hide_from_address_lists").(bool); hideFromAddressList { + if hideFromAddressList, ok := d.GetOkExists("hide_from_address_lists"); ok { //nolint:staticcheck if _, err := client.Update(ctx, msgraph.Group{ DirectoryObject: msgraph.DirectoryObject{ Id: group.ID(), }, - HideFromAddressLists: utils.Bool(hideFromAddressList), + HideFromAddressLists: utils.Bool(hideFromAddressList.(bool)), }); err != nil { return tf.ErrorDiagF(err, "Failed to set `hide_from_address_lists` for group with object ID %q", *group.ID()) } @@ -772,12 +772,12 @@ func groupResourceCreate(ctx context.Context, d *schema.ResourceData, meta inter } // HideFromOutlookClients can only be set in its own PATCH request; including other properties returns a 400 - if hideFromOutlookClients := d.Get("hide_from_outlook_clients").(bool); hideFromOutlookClients { + if hideFromOutlookClients, ok := d.GetOkExists("hide_from_outlook_clients"); ok { //nolint:staticcheck if _, err := client.Update(ctx, msgraph.Group{ DirectoryObject: msgraph.DirectoryObject{ Id: group.ID(), }, - HideFromOutlookClients: utils.Bool(hideFromOutlookClients), + HideFromOutlookClients: utils.Bool(hideFromOutlookClients.(bool)), }); err != nil { return tf.ErrorDiagF(err, "Failed to set `hide_from_outlook_clients` for group with object ID %q", *group.ID()) } diff --git a/internal/services/groups/group_resource_test.go b/internal/services/groups/group_resource_test.go index dc94b77d34..b99cf01c19 100644 --- a/internal/services/groups/group_resource_test.go +++ b/internal/services/groups/group_resource_test.go @@ -961,17 +961,17 @@ resource "azuread_group" "test" { func (r GroupResource) administrativeUnits(data acceptance.TestData) string { return fmt.Sprintf(` resource "azuread_administrative_unit" "test" { - display_name = "acctestGroup-administrative-unit-%[1]d" + display_name = "acctestGroup-administrative-unit-%[1]d" } resource "azuread_administrative_unit" "test2" { - display_name = "acctestGroup-administrative-unit-%[1]d" + display_name = "acctestGroup-administrative-unit-%[1]d" } resource "azuread_group" "test" { - display_name = "acctestGroup-%[1]d" - security_enabled = true - administrative_unit_ids = [azuread_administrative_unit.test.id, azuread_administrative_unit.test2.id] + display_name = "acctestGroup-%[1]d" + security_enabled = true + administrative_unit_ids = [azuread_administrative_unit.test.id, azuread_administrative_unit.test2.id] } `, data.RandomInteger, data.RandomInteger) } @@ -979,16 +979,16 @@ resource "azuread_group" "test" { func (r GroupResource) administrativeUnitsWithoutAssociation(data acceptance.TestData) string { return fmt.Sprintf(` resource "azuread_administrative_unit" "test" { - display_name = "acctestGroup-administrative-unit-%[1]d" + display_name = "acctestGroup-administrative-unit-%[1]d" } resource "azuread_administrative_unit" "test2" { - display_name = "acctestGroup-administrative-unit-%[1]d" + display_name = "acctestGroup-administrative-unit-%[1]d" } resource "azuread_group" "test" { - display_name = "acctestGroup-%[1]d" - security_enabled = true + display_name = "acctestGroup-%[1]d" + security_enabled = true } `, data.RandomInteger, data.RandomInteger) } From 9e899d3f0b7f6c19e27729a980f3ef00b259bb8f Mon Sep 17 00:00:00 2001 From: Tom Bamford Date: Tue, 21 Feb 2023 22:17:01 +0200 Subject: [PATCH 2/2] azuread_group: only set additional fields on update, when the value differs --- internal/services/groups/group_resource.go | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/internal/services/groups/group_resource.go b/internal/services/groups/group_resource.go index 51662f5b0a..34a8f37fcd 100644 --- a/internal/services/groups/group_resource.go +++ b/internal/services/groups/group_resource.go @@ -916,11 +916,15 @@ func groupResourceUpdate(ctx context.Context, d *schema.ResourceData, meta inter if hasGroupType(groupTypes, msgraph.GroupTypeUnified) { // The unified group properties in this block only support delegated auth // Application-authenticated requests will return a 4xx error, so we only - // set these when explicitly configured + // set these when explicitly configured, and when the value differs. // See https://docs.microsoft.com/en-us/graph/known-issues#groups + extra, err := groupGetAdditional(ctx, client, *group.ID()) + if err != nil { + return tf.ErrorDiagF(err, "Retrieving extra fields for group with ID: %q", *group.ID()) + } // AllowExternalSenders can only be set in its own PATCH request; including other properties returns a 400 - if v, ok := d.GetOkExists("external_senders_allowed"); ok { //nolint:staticcheck + if v, ok := d.GetOkExists("external_senders_allowed"); ok && (extra.AllowExternalSenders == nil || *extra.AllowExternalSenders != v.(bool)) { //nolint:staticcheck if _, err := client.Update(ctx, msgraph.Group{ DirectoryObject: msgraph.DirectoryObject{ Id: group.ID(), @@ -944,7 +948,7 @@ func groupResourceUpdate(ctx context.Context, d *schema.ResourceData, meta inter } // AutoSubscribeNewMembers can only be set in its own PATCH request; including other properties returns a 400 - if v, ok := d.GetOkExists("auto_subscribe_new_members"); ok { //nolint:staticcheck + if v, ok := d.GetOkExists("auto_subscribe_new_members"); ok && (extra.AutoSubscribeNewMembers == nil || *extra.AutoSubscribeNewMembers != v.(bool)) { //nolint:staticcheck if _, err := client.Update(ctx, msgraph.Group{ DirectoryObject: msgraph.DirectoryObject{ Id: group.ID(), @@ -968,7 +972,7 @@ func groupResourceUpdate(ctx context.Context, d *schema.ResourceData, meta inter } // HideFromAddressLists can only be set in its own PATCH request; including other properties returns a 400 - if v, ok := d.GetOkExists("hide_from_address_lists"); ok { //nolint:staticcheck + if v, ok := d.GetOkExists("hide_from_address_lists"); ok && (extra.HideFromAddressLists == nil || *extra.HideFromAddressLists != v.(bool)) { //nolint:staticcheck if _, err := client.Update(ctx, msgraph.Group{ DirectoryObject: msgraph.DirectoryObject{ Id: group.ID(), @@ -992,7 +996,7 @@ func groupResourceUpdate(ctx context.Context, d *schema.ResourceData, meta inter } // HideFromOutlookClients can only be set in its own PATCH request; including other properties returns a 400 - if v, ok := d.GetOkExists("hide_from_outlook_clients"); ok { //nolint:staticcheck + if v, ok := d.GetOkExists("hide_from_outlook_clients"); ok && (extra.HideFromOutlookClients == nil || *extra.HideFromOutlookClients != v.(bool)) { //nolint:staticcheck if _, err := client.Update(ctx, msgraph.Group{ DirectoryObject: msgraph.DirectoryObject{ Id: group.ID(),