From d8ccc4b005b06b1d8034cc0a5479023d870ec2d6 Mon Sep 17 00:00:00 2001 From: Dapeng Zhang Date: Thu, 7 May 2020 15:29:08 +0800 Subject: [PATCH 1/5] Fix 6757 --- .../management_group_data_source.go | 57 +++++++++++++++++-- .../management_group_resource.go | 2 +- .../management_group_data_source_test.go | 40 ++++++++++++- website/docs/d/management_group.html.markdown | 12 +++- website/docs/r/management_group.html.markdown | 4 ++ 5 files changed, 103 insertions(+), 12 deletions(-) diff --git a/azurerm/internal/services/managementgroup/management_group_data_source.go b/azurerm/internal/services/managementgroup/management_group_data_source.go index fdc49353e7b7..78244b2d4f6f 100644 --- a/azurerm/internal/services/managementgroup/management_group_data_source.go +++ b/azurerm/internal/services/managementgroup/management_group_data_source.go @@ -1,6 +1,7 @@ package managementgroup import ( + "context" "fmt" "time" @@ -25,7 +26,7 @@ func dataSourceArmManagementGroup() *schema.Resource { Type: schema.TypeString, Optional: true, Computed: true, - ExactlyOneOf: []string{"name", "group_id"}, + ExactlyOneOf: []string{"name", "group_id", "display_name"}, Deprecated: "Deprecated in favour of `name`", ValidateFunc: validate.ManagementGroupName, }, @@ -34,13 +35,14 @@ func dataSourceArmManagementGroup() *schema.Resource { Type: schema.TypeString, Optional: true, // TODO -- change back to required after the deprecation Computed: true, // TODO -- remove computed after the deprecation - ExactlyOneOf: []string{"name", "group_id"}, + ExactlyOneOf: []string{"name", "group_id", "display_name"}, ValidateFunc: validate.ManagementGroupName, }, "display_name": { - Type: schema.TypeString, - Computed: true, + Type: schema.TypeString, + Optional: true, + ExactlyOneOf: []string{"name", "group_id", "display_name"}, }, "parent_management_group_id": { @@ -58,6 +60,41 @@ func dataSourceArmManagementGroup() *schema.Resource { } } +func getManagementGroupNameByDisplayName(ctx context.Context, client *managementgroups.Client, displayName string) (string, error) { + iterator, err := client.ListComplete(ctx, managementGroupCacheControl, "") + if err != nil { + return "", fmt.Errorf("Error listing Management Groups: %+v", err) + } + + var results []string + for iterator.NotDone() { + group := iterator.Value() + if group.DisplayName != nil && *group.DisplayName == displayName && group.Name != nil { + results = append(results, *group.Name) + } + + if err := iterator.NextWithContext(ctx); err != nil { + return "", fmt.Errorf("Error listing Management Groups: %+v", err) + } + } + + // we found none + if len(results) == 0 { + return "", fmt.Errorf("do not find Management Group (Display Name %q)", displayName) + } + + // we found more than one + if len(results) > 1 { + return "", fmt.Errorf("found more than one Management Group with display name %q", displayName) + } + + if results[0] == "" { + return "", fmt.Errorf("cannot find the Management Group with display name %q", displayName) + } + + return results[0], nil +} + func dataSourceArmManagementGroupRead(d *schema.ResourceData, meta interface{}) error { client := meta.(*clients.Client).ManagementGroups.GroupsClient ctx, cancel := timeouts.ForRead(meta.(*clients.Client).StopContext, d) @@ -70,7 +107,17 @@ func dataSourceArmManagementGroupRead(d *schema.ResourceData, meta interface{}) if v, ok := d.GetOk("group_id"); ok { groupName = v.(string) } + displayName := d.Get("display_name").(string) + // one of displayName and groupName must be non-empty, this is guaranteed by schema + // if the user is retrieving the mgmt group by display name, use the list api to get the group name first + var err error + if displayName != "" { + groupName, err = getManagementGroupNameByDisplayName(ctx, client, displayName) + if err != nil { + return fmt.Errorf("Error reading Management Group (Display Name %q): %+v", displayName, err) + } + } recurse := true resp, err := client.Get(ctx, groupName, "children", &recurse, "", managementGroupCacheControl) if err != nil { @@ -78,7 +125,7 @@ func dataSourceArmManagementGroupRead(d *schema.ResourceData, meta interface{}) return fmt.Errorf("Management Group %q was not found", groupName) } - return fmt.Errorf("Error reading Management Group %q: %+v", d.Id(), err) + return fmt.Errorf("Error reading Management Group %q: %+v", groupName, err) } if resp.ID == nil { diff --git a/azurerm/internal/services/managementgroup/management_group_resource.go b/azurerm/internal/services/managementgroup/management_group_resource.go index 661036045290..d24c64cb6573 100644 --- a/azurerm/internal/services/managementgroup/management_group_resource.go +++ b/azurerm/internal/services/managementgroup/management_group_resource.go @@ -91,7 +91,7 @@ func resourceArmManagementGroupCreateUpdate(d *schema.ResourceData, meta interfa armTenantID := meta.(*clients.Client).Account.TenantId groupName := uuid.New().String() - if v, ok := d.GetOk("group_name"); ok { + if v, ok := d.GetOk("name"); ok { groupName = v.(string) } if v, ok := d.GetOk("group_id"); ok { diff --git a/azurerm/internal/services/managementgroup/tests/management_group_data_source_test.go b/azurerm/internal/services/managementgroup/tests/management_group_data_source_test.go index 405cb9cd4ff2..41be4b998400 100644 --- a/azurerm/internal/services/managementgroup/tests/management_group_data_source_test.go +++ b/azurerm/internal/services/managementgroup/tests/management_group_data_source_test.go @@ -8,7 +8,7 @@ import ( "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/acceptance" ) -func TestAccDataSourceArmManagementGroup_basic(t *testing.T) { +func TestAccDataSourceArmManagementGroup_basicByName(t *testing.T) { data := acceptance.BuildTestData(t, "data.azurerm_management_group", "test") resource.ParallelTest(t, resource.TestCase{ @@ -16,7 +16,7 @@ func TestAccDataSourceArmManagementGroup_basic(t *testing.T) { Providers: acceptance.SupportedProviders, Steps: []resource.TestStep{ { - Config: testAccDataSourceArmManagementGroup_basic(data), + Config: testAccDataSourceArmManagementGroup_basicByName(data), Check: resource.ComposeTestCheckFunc( resource.TestCheckResourceAttr(data.ResourceName, "display_name", fmt.Sprintf("acctestmg-%d", data.RandomInteger)), resource.TestCheckResourceAttr(data.ResourceName, "subscription_ids.#", "0"), @@ -26,7 +26,25 @@ func TestAccDataSourceArmManagementGroup_basic(t *testing.T) { }) } -func testAccDataSourceArmManagementGroup_basic(data acceptance.TestData) string { +func TestAccDataSourceArmManagementGroup_basicByDisplayName(t *testing.T) { + data := acceptance.BuildTestData(t, "data.azurerm_management_group", "test") + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { acceptance.PreCheck(t) }, + Providers: acceptance.SupportedProviders, + Steps: []resource.TestStep{ + { + Config: testAccDataSourceArmManagementGroup_basicByDisplayName(data), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr(data.ResourceName, "display_name", fmt.Sprintf("acctestmg-%d", data.RandomInteger)), + resource.TestCheckResourceAttr(data.ResourceName, "subscription_ids.#", "0"), + ), + }, + }, + }) +} + +func testAccDataSourceArmManagementGroup_basicByName(data acceptance.TestData) string { return fmt.Sprintf(` provider "azurerm" { features {} @@ -41,3 +59,19 @@ data "azurerm_management_group" "test" { } `, data.RandomInteger) } + +func testAccDataSourceArmManagementGroup_basicByDisplayName(data acceptance.TestData) string { + return fmt.Sprintf(` +provider "azurerm" { + features {} +} + +resource "azurerm_management_group" "test" { + display_name = "acctestmg-%d" +} + +data "azurerm_management_group" "test" { + display_name = azurerm_management_group.test.display_name +} +`, data.RandomInteger) +} diff --git a/website/docs/d/management_group.html.markdown b/website/docs/d/management_group.html.markdown index 668836809ad3..3b8da58916b9 100644 --- a/website/docs/d/management_group.html.markdown +++ b/website/docs/d/management_group.html.markdown @@ -13,6 +13,10 @@ Use this data source to access information about an existing Management Group. ## Example Usage ```hcl +provider "azurerm" { + features {} +} + data "azurerm_management_group" "example" { name = "00000000-0000-0000-0000-000000000000" } @@ -32,17 +36,19 @@ The following arguments are supported: ~> **NOTE:** The field `group_id` has been deprecated in favour of `name`. +* `display_name` - Specifies the display name of this Management Group. + +~> **NOTE** As `display_name` is not unique errors may occur when there are multiple Management Groups with same display name. + ## Attributes Reference The following attributes are exported: * `id` - The ID of the Management Group. -* `display_name` - A friendly name for the Management Group. - * `parent_management_group_id` - The ID of any Parent Management Group. -* `subscription_ids` - A list of Subscription ID's which are assigned to the Management Group. +* `subscription_ids` - A list of Subscription IDs which are assigned to the Management Group. ## Timeouts diff --git a/website/docs/r/management_group.html.markdown b/website/docs/r/management_group.html.markdown index 63f5936eb659..6ffb72534661 100644 --- a/website/docs/r/management_group.html.markdown +++ b/website/docs/r/management_group.html.markdown @@ -43,8 +43,12 @@ The following arguments are supported: * `group_id` - (Optional) The name or UUID for this Management Group, which needs to be unique across your tenant. A new UUID will be generated if not provided. Changing this forces a new resource to be created. +~> **Note** `name` and `group_id` here is called as `ID` on Azure Portal. + * `display_name` - (Optional) A friendly name for this Management Group. If not specified, this'll be the same as the `name`. +~> **Note** `display_name` here is called as `Name` on Azure Portal. + * `parent_management_group_id` - (Optional) The ID of the Parent Management Group. Changing this forces a new resource to be created. * `subscription_ids` - (Optional) A list of Subscription GUIDs which should be assigned to the Management Group. From 0f728edf052f416170c0bf884c5dda1268f2edf6 Mon Sep 17 00:00:00 2001 From: Dapeng Zhang Date: Thu, 7 May 2020 15:32:59 +0800 Subject: [PATCH 2/5] Fix schema flaus --- .../services/managementgroup/management_group_data_source.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/azurerm/internal/services/managementgroup/management_group_data_source.go b/azurerm/internal/services/managementgroup/management_group_data_source.go index 78244b2d4f6f..b61315c5d829 100644 --- a/azurerm/internal/services/managementgroup/management_group_data_source.go +++ b/azurerm/internal/services/managementgroup/management_group_data_source.go @@ -33,8 +33,8 @@ func dataSourceArmManagementGroup() *schema.Resource { "name": { Type: schema.TypeString, - Optional: true, // TODO -- change back to required after the deprecation - Computed: true, // TODO -- remove computed after the deprecation + Optional: true, + Computed: true, ExactlyOneOf: []string{"name", "group_id", "display_name"}, ValidateFunc: validate.ManagementGroupName, }, @@ -42,6 +42,7 @@ func dataSourceArmManagementGroup() *schema.Resource { "display_name": { Type: schema.TypeString, Optional: true, + Computed: true, ExactlyOneOf: []string{"name", "group_id", "display_name"}, }, From 04289ea92ca275632c624de62978bab0f15e0691 Mon Sep 17 00:00:00 2001 From: Dapeng Zhang Date: Thu, 7 May 2020 15:34:53 +0800 Subject: [PATCH 3/5] Move helper functions down --- .../management_group_data_source.go | 70 +++++++++---------- 1 file changed, 35 insertions(+), 35 deletions(-) diff --git a/azurerm/internal/services/managementgroup/management_group_data_source.go b/azurerm/internal/services/managementgroup/management_group_data_source.go index b61315c5d829..a34141d75e64 100644 --- a/azurerm/internal/services/managementgroup/management_group_data_source.go +++ b/azurerm/internal/services/managementgroup/management_group_data_source.go @@ -61,41 +61,6 @@ func dataSourceArmManagementGroup() *schema.Resource { } } -func getManagementGroupNameByDisplayName(ctx context.Context, client *managementgroups.Client, displayName string) (string, error) { - iterator, err := client.ListComplete(ctx, managementGroupCacheControl, "") - if err != nil { - return "", fmt.Errorf("Error listing Management Groups: %+v", err) - } - - var results []string - for iterator.NotDone() { - group := iterator.Value() - if group.DisplayName != nil && *group.DisplayName == displayName && group.Name != nil { - results = append(results, *group.Name) - } - - if err := iterator.NextWithContext(ctx); err != nil { - return "", fmt.Errorf("Error listing Management Groups: %+v", err) - } - } - - // we found none - if len(results) == 0 { - return "", fmt.Errorf("do not find Management Group (Display Name %q)", displayName) - } - - // we found more than one - if len(results) > 1 { - return "", fmt.Errorf("found more than one Management Group with display name %q", displayName) - } - - if results[0] == "" { - return "", fmt.Errorf("cannot find the Management Group with display name %q", displayName) - } - - return results[0], nil -} - func dataSourceArmManagementGroupRead(d *schema.ResourceData, meta interface{}) error { client := meta.(*clients.Client).ManagementGroups.GroupsClient ctx, cancel := timeouts.ForRead(meta.(*clients.Client).StopContext, d) @@ -160,6 +125,41 @@ func dataSourceArmManagementGroupRead(d *schema.ResourceData, meta interface{}) return nil } +func getManagementGroupNameByDisplayName(ctx context.Context, client *managementgroups.Client, displayName string) (string, error) { + iterator, err := client.ListComplete(ctx, managementGroupCacheControl, "") + if err != nil { + return "", fmt.Errorf("Error listing Management Groups: %+v", err) + } + + var results []string + for iterator.NotDone() { + group := iterator.Value() + if group.DisplayName != nil && *group.DisplayName == displayName && group.Name != nil { + results = append(results, *group.Name) + } + + if err := iterator.NextWithContext(ctx); err != nil { + return "", fmt.Errorf("Error listing Management Groups: %+v", err) + } + } + + // we found none + if len(results) == 0 { + return "", fmt.Errorf("do not find Management Group (Display Name %q)", displayName) + } + + // we found more than one + if len(results) > 1 { + return "", fmt.Errorf("found more than one Management Group with display name %q", displayName) + } + + if results[0] == "" { + return "", fmt.Errorf("cannot find the Management Group with display name %q", displayName) + } + + return results[0], nil +} + func flattenArmManagementGroupDataSourceSubscriptionIds(input *[]managementgroups.ChildInfo) (*schema.Set, error) { subscriptionIds := &schema.Set{F: schema.HashString} if input == nil { From 35e07ea183624c4ad1399ebb0765ff0e7846cf33 Mon Sep 17 00:00:00 2001 From: Dapeng Zhang Date: Sat, 9 May 2020 11:57:00 +0800 Subject: [PATCH 4/5] Resolve comments --- .../managementgroup/management_group_data_source.go | 6 +----- website/docs/r/management_group.html.markdown | 4 ++-- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/azurerm/internal/services/managementgroup/management_group_data_source.go b/azurerm/internal/services/managementgroup/management_group_data_source.go index a34141d75e64..64f2329ac0c5 100644 --- a/azurerm/internal/services/managementgroup/management_group_data_source.go +++ b/azurerm/internal/services/managementgroup/management_group_data_source.go @@ -134,7 +134,7 @@ func getManagementGroupNameByDisplayName(ctx context.Context, client *management var results []string for iterator.NotDone() { group := iterator.Value() - if group.DisplayName != nil && *group.DisplayName == displayName && group.Name != nil { + if group.DisplayName != nil && *group.DisplayName == displayName && group.Name != nil && *group.Name != "" { results = append(results, *group.Name) } @@ -153,10 +153,6 @@ func getManagementGroupNameByDisplayName(ctx context.Context, client *management return "", fmt.Errorf("found more than one Management Group with display name %q", displayName) } - if results[0] == "" { - return "", fmt.Errorf("cannot find the Management Group with display name %q", displayName) - } - return results[0], nil } diff --git a/website/docs/r/management_group.html.markdown b/website/docs/r/management_group.html.markdown index 6ffb72534661..580ca31d6dc6 100644 --- a/website/docs/r/management_group.html.markdown +++ b/website/docs/r/management_group.html.markdown @@ -43,11 +43,11 @@ The following arguments are supported: * `group_id` - (Optional) The name or UUID for this Management Group, which needs to be unique across your tenant. A new UUID will be generated if not provided. Changing this forces a new resource to be created. -~> **Note** `name` and `group_id` here is called as `ID` on Azure Portal. +~> **Note** `name` and `group_id` here corresponds to `ID` on Azure Portal. * `display_name` - (Optional) A friendly name for this Management Group. If not specified, this'll be the same as the `name`. -~> **Note** `display_name` here is called as `Name` on Azure Portal. +~> **Note** `display_name` here corresponds to `Name` on Azure Portal. * `parent_management_group_id` - (Optional) The ID of the Parent Management Group. Changing this forces a new resource to be created. From 8f9a2b92fdfd0d70a87d96bbe7ff366d8dccd52a Mon Sep 17 00:00:00 2001 From: Dapeng Zhang Date: Tue, 12 May 2020 12:34:57 +0800 Subject: [PATCH 5/5] Resolve comments --- .../managementgroup/management_group_data_source.go | 4 ++-- .../tests/management_group_data_source_test.go | 4 ++-- website/docs/d/management_group.html.markdown | 6 +----- website/docs/r/management_group.html.markdown | 4 ---- 4 files changed, 5 insertions(+), 13 deletions(-) diff --git a/azurerm/internal/services/managementgroup/management_group_data_source.go b/azurerm/internal/services/managementgroup/management_group_data_source.go index 64f2329ac0c5..a8f9a26f0471 100644 --- a/azurerm/internal/services/managementgroup/management_group_data_source.go +++ b/azurerm/internal/services/managementgroup/management_group_data_source.go @@ -145,12 +145,12 @@ func getManagementGroupNameByDisplayName(ctx context.Context, client *management // we found none if len(results) == 0 { - return "", fmt.Errorf("do not find Management Group (Display Name %q)", displayName) + return "", fmt.Errorf("Management Group (Display Name %q) was not found", displayName) } // we found more than one if len(results) > 1 { - return "", fmt.Errorf("found more than one Management Group with display name %q", displayName) + return "", fmt.Errorf("expected a single Management Group with the Display Name %q but expected one", displayName) } return results[0], nil diff --git a/azurerm/internal/services/managementgroup/tests/management_group_data_source_test.go b/azurerm/internal/services/managementgroup/tests/management_group_data_source_test.go index 41be4b998400..809014b8a49a 100644 --- a/azurerm/internal/services/managementgroup/tests/management_group_data_source_test.go +++ b/azurerm/internal/services/managementgroup/tests/management_group_data_source_test.go @@ -36,7 +36,7 @@ func TestAccDataSourceArmManagementGroup_basicByDisplayName(t *testing.T) { { Config: testAccDataSourceArmManagementGroup_basicByDisplayName(data), Check: resource.ComposeTestCheckFunc( - resource.TestCheckResourceAttr(data.ResourceName, "display_name", fmt.Sprintf("acctestmg-%d", data.RandomInteger)), + resource.TestCheckResourceAttr(data.ResourceName, "display_name", fmt.Sprintf("acctest Management Group %d", data.RandomInteger)), resource.TestCheckResourceAttr(data.ResourceName, "subscription_ids.#", "0"), ), }, @@ -67,7 +67,7 @@ provider "azurerm" { } resource "azurerm_management_group" "test" { - display_name = "acctestmg-%d" + display_name = "acctest Management Group %d" } data "azurerm_management_group" "test" { diff --git a/website/docs/d/management_group.html.markdown b/website/docs/d/management_group.html.markdown index 3b8da58916b9..b5ad6c03d3f1 100644 --- a/website/docs/d/management_group.html.markdown +++ b/website/docs/d/management_group.html.markdown @@ -13,10 +13,6 @@ Use this data source to access information about an existing Management Group. ## Example Usage ```hcl -provider "azurerm" { - features {} -} - data "azurerm_management_group" "example" { name = "00000000-0000-0000-0000-000000000000" } @@ -38,7 +34,7 @@ The following arguments are supported: * `display_name` - Specifies the display name of this Management Group. -~> **NOTE** As `display_name` is not unique errors may occur when there are multiple Management Groups with same display name. +~> **NOTE** Whilst multiple management groups may share the same display name, when filtering Terraform expects a single management group to be found with this name. ## Attributes Reference diff --git a/website/docs/r/management_group.html.markdown b/website/docs/r/management_group.html.markdown index 580ca31d6dc6..63f5936eb659 100644 --- a/website/docs/r/management_group.html.markdown +++ b/website/docs/r/management_group.html.markdown @@ -43,12 +43,8 @@ The following arguments are supported: * `group_id` - (Optional) The name or UUID for this Management Group, which needs to be unique across your tenant. A new UUID will be generated if not provided. Changing this forces a new resource to be created. -~> **Note** `name` and `group_id` here corresponds to `ID` on Azure Portal. - * `display_name` - (Optional) A friendly name for this Management Group. If not specified, this'll be the same as the `name`. -~> **Note** `display_name` here corresponds to `Name` on Azure Portal. - * `parent_management_group_id` - (Optional) The ID of the Parent Management Group. Changing this forces a new resource to be created. * `subscription_ids` - (Optional) A list of Subscription GUIDs which should be assigned to the Management Group.