From e150c122d592a50cf991f4956012b3db7ed1c059 Mon Sep 17 00:00:00 2001 From: Dapeng Zhang Date: Fri, 27 Mar 2020 13:02:49 +0800 Subject: [PATCH 1/4] Add new field group_name and deprecate group_id --- .../data_source_management_group.go | 30 ++++++-- .../resource_arm_management_group.go | 71 +++++++++++-------- .../data_source_management_group_test.go | 2 +- .../resource_arm_management_group_test.go | 8 +-- website/docs/d/management_group.html.markdown | 8 ++- website/docs/r/management_group.html.markdown | 10 ++- 6 files changed, 81 insertions(+), 48 deletions(-) diff --git a/azurerm/internal/services/managementgroup/data_source_management_group.go b/azurerm/internal/services/managementgroup/data_source_management_group.go index 8a0deb7d05bf..2f5ada30dd0d 100644 --- a/azurerm/internal/services/managementgroup/data_source_management_group.go +++ b/azurerm/internal/services/managementgroup/data_source_management_group.go @@ -23,7 +23,18 @@ func dataSourceArmManagementGroup() *schema.Resource { Schema: map[string]*schema.Schema{ "group_id": { Type: schema.TypeString, - Required: true, + Optional: true, + Computed: true, + ExactlyOneOf: []string{"name", "group_id"}, + Deprecated: "Deprecated in favor of `name`", + ValidateFunc: validate.ManagementGroupName, + }, + + "name": { + 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"}, ValidateFunc: validate.ManagementGroupName, }, @@ -52,24 +63,31 @@ func dataSourceArmManagementGroupRead(d *schema.ResourceData, meta interface{}) ctx, cancel := timeouts.ForRead(meta.(*clients.Client).StopContext, d) defer cancel() - groupId := d.Get("group_id").(string) + groupName := "" + if v, ok := d.GetOk("name"); ok { + groupName = v.(string) + } + if v, ok := d.GetOk("group_id"); ok { + groupName = v.(string) + } recurse := true - resp, err := client.Get(ctx, groupId, "children", &recurse, "", managementGroupCacheControl) + resp, err := client.Get(ctx, groupName, "children", &recurse, "", managementGroupCacheControl) if err != nil { if utils.ResponseWasNotFound(resp.Response) { - return fmt.Errorf("Management Group %q was not found", groupId) + return fmt.Errorf("Management Group %q was not found", groupName) } return fmt.Errorf("Error reading Management Group %q: %+v", d.Id(), err) } if resp.ID == nil { - return fmt.Errorf("Client returned an nil ID for Management Group %q", groupId) + return fmt.Errorf("Client returned an nil ID for Management Group %q", groupName) } d.SetId(*resp.ID) - d.Set("group_id", groupId) + d.Set("name", groupName) + d.Set("group_id", groupName) if props := resp.Properties; props != nil { d.Set("display_name", props.DisplayName) diff --git a/azurerm/internal/services/managementgroup/resource_arm_management_group.go b/azurerm/internal/services/managementgroup/resource_arm_management_group.go index e09e9ac2f2dc..cb72ef8279fe 100644 --- a/azurerm/internal/services/managementgroup/resource_arm_management_group.go +++ b/azurerm/internal/services/managementgroup/resource_arm_management_group.go @@ -12,7 +12,6 @@ import ( "github.com/hashicorp/terraform-plugin-sdk/helper/schema" "github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/tf" "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/clients" - "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/features" "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/managementgroup/parse" "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/managementgroup/validate" "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/timeouts" @@ -40,11 +39,22 @@ func resourceArmManagementGroup() *schema.Resource { Schema: map[string]*schema.Schema{ "group_id": { - Type: schema.TypeString, - Optional: true, - Computed: true, - ForceNew: true, - ValidateFunc: validate.ManagementGroupName, + Type: schema.TypeString, + Optional: true, + Computed: true, + ForceNew: true, + ConflictsWith: []string{"name"}, + Deprecated: "Deprecated in favor of `name`", + ValidateFunc: validate.ManagementGroupName, + }, + + "name": { + Type: schema.TypeString, + Optional: true, + Computed: true, + ForceNew: true, + ConflictsWith: []string{"group_id"}, + ValidateFunc: validate.ManagementGroupName, }, "display_name": { @@ -77,9 +87,12 @@ func resourceArmManagementGroupCreateUpdate(d *schema.ResourceData, meta interfa defer cancel() armTenantID := meta.(*clients.Client).Account.TenantId - groupId := d.Get("group_id").(string) - if groupId == "" { - groupId = uuid.New().String() + groupName := uuid.New().String() + if v, ok := d.GetOk("group_name"); ok { + groupName = v.(string) + } + if v, ok := d.GetOk("group_id"); ok { + groupName = v.(string) } parentManagementGroupId := d.Get("parent_management_group_id").(string) @@ -88,11 +101,11 @@ func resourceArmManagementGroupCreateUpdate(d *schema.ResourceData, meta interfa } recurse := false - if features.ShouldResourcesBeImported() && d.IsNewResource() { - existing, err := client.Get(ctx, groupId, "children", &recurse, "", managementGroupCacheControl) + if d.IsNewResource() { + existing, err := client.Get(ctx, groupName, "children", &recurse, "", managementGroupCacheControl) if err != nil { if !utils.ResponseWasNotFound(existing.Response) { - return fmt.Errorf("unable to check for presence of existing Management Group %q: %s", groupId, err) + return fmt.Errorf("unable to check for presence of existing Management Group %q: %s", groupName, err) } } @@ -101,10 +114,10 @@ func resourceArmManagementGroupCreateUpdate(d *schema.ResourceData, meta interfa } } - log.Printf("[INFO] Creating Management Group %q", groupId) + log.Printf("[INFO] Creating Management Group %q", groupName) properties := managementgroups.CreateManagementGroupRequest{ - Name: utils.String(groupId), + Name: utils.String(groupName), CreateManagementGroupProperties: &managementgroups.CreateManagementGroupProperties{ TenantID: utils.String(armTenantID), Details: &managementgroups.CreateManagementGroupDetails{ @@ -119,18 +132,18 @@ func resourceArmManagementGroupCreateUpdate(d *schema.ResourceData, meta interfa properties.CreateManagementGroupProperties.DisplayName = utils.String(v.(string)) } - future, err := client.CreateOrUpdate(ctx, groupId, properties, managementGroupCacheControl) + future, err := client.CreateOrUpdate(ctx, groupName, properties, managementGroupCacheControl) if err != nil { - return fmt.Errorf("unable to create Management Group %q: %+v", groupId, err) + return fmt.Errorf("unable to create Management Group %q: %+v", groupName, err) } if err = future.WaitForCompletionRef(ctx, client.Client); err != nil { - return fmt.Errorf("failed when waiting for creation of Management Group %q: %+v", groupId, err) + return fmt.Errorf("failed when waiting for creation of Management Group %q: %+v", groupName, err) } - resp, err := client.Get(ctx, groupId, "children", &recurse, "", managementGroupCacheControl) + resp, err := client.Get(ctx, groupName, "children", &recurse, "", managementGroupCacheControl) if err != nil { - return fmt.Errorf("unable to retrieve Management Group %q: %+v", groupId, err) + return fmt.Errorf("unable to retrieve Management Group %q: %+v", groupName, err) } d.SetId(*resp.ID) @@ -139,19 +152,19 @@ func resourceArmManagementGroupCreateUpdate(d *schema.ResourceData, meta interfa // first remove any which need to be removed if !d.IsNewResource() { - log.Printf("[DEBUG] Determine which Subscriptions should be removed from Management Group %q", groupId) + log.Printf("[DEBUG] Determine which Subscriptions should be removed from Management Group %q", groupName) if props := resp.Properties; props != nil { subscriptionIdsToRemove, err2 := determineManagementGroupSubscriptionsIdsToRemove(props.Children, subscriptionIds) if err2 != nil { - return fmt.Errorf("unable to determine which subscriptions should be removed from Management Group %q: %+v", groupId, err2) + return fmt.Errorf("unable to determine which subscriptions should be removed from Management Group %q: %+v", groupName, err2) } for _, subscriptionId := range *subscriptionIdsToRemove { - log.Printf("[DEBUG] De-associating Subscription ID %q from Management Group %q", subscriptionId, groupId) - deleteResp, err2 := subscriptionsClient.Delete(ctx, groupId, subscriptionId, managementGroupCacheControl) + log.Printf("[DEBUG] De-associating Subscription ID %q from Management Group %q", subscriptionId, groupName) + deleteResp, err2 := subscriptionsClient.Delete(ctx, groupName, subscriptionId, managementGroupCacheControl) if err2 != nil { if !response.WasNotFound(deleteResp.Response) { - return fmt.Errorf("unable to de-associate Subscription %q from Management Group %q: %+v", subscriptionId, groupId, err2) + return fmt.Errorf("unable to de-associate Subscription %q from Management Group %q: %+v", subscriptionId, groupName, err2) } } } @@ -159,12 +172,11 @@ func resourceArmManagementGroupCreateUpdate(d *schema.ResourceData, meta interfa } // then add the new ones - log.Printf("[DEBUG] Preparing to assign Subscriptions to Management Group %q", groupId) + log.Printf("[DEBUG] Preparing to assign Subscriptions to Management Group %q", groupName) for _, subscriptionId := range subscriptionIds { - log.Printf("[DEBUG] Assigning Subscription ID %q to management group %q", subscriptionId, groupId) - _, err = subscriptionsClient.Create(ctx, groupId, subscriptionId, managementGroupCacheControl) - if err != nil { - return fmt.Errorf("[DEBUG] Error assigning Subscription ID %q to Management Group %q: %+v", subscriptionId, groupId, err) + log.Printf("[DEBUG] Assigning Subscription ID %q to management group %q", subscriptionId, groupName) + if _, err := subscriptionsClient.Create(ctx, groupName, subscriptionId, managementGroupCacheControl); err != nil { + return fmt.Errorf("[DEBUG] Error assigning Subscription ID %q to Management Group %q: %+v", subscriptionId, groupName, err) } } @@ -193,6 +205,7 @@ func resourceArmManagementGroupRead(d *schema.ResourceData, meta interface{}) er return fmt.Errorf("unable to read Management Group %q: %+v", d.Id(), err) } + d.Set("name", id.GroupId) d.Set("group_id", id.GroupId) if props := resp.Properties; props != nil { diff --git a/azurerm/internal/services/managementgroup/tests/data_source_management_group_test.go b/azurerm/internal/services/managementgroup/tests/data_source_management_group_test.go index dd27c1c2aa82..d540113b1b56 100644 --- a/azurerm/internal/services/managementgroup/tests/data_source_management_group_test.go +++ b/azurerm/internal/services/managementgroup/tests/data_source_management_group_test.go @@ -37,7 +37,7 @@ resource "azurerm_management_group" "test" { } data "azurerm_management_group" "test" { - group_id = azurerm_management_group.test.group_id + group_name = azurerm_management_group.test.group_name } `, data.RandomInteger) } diff --git a/azurerm/internal/services/managementgroup/tests/resource_arm_management_group_test.go b/azurerm/internal/services/managementgroup/tests/resource_arm_management_group_test.go index d764fed56d5d..254aeb15f4d9 100644 --- a/azurerm/internal/services/managementgroup/tests/resource_arm_management_group_test.go +++ b/azurerm/internal/services/managementgroup/tests/resource_arm_management_group_test.go @@ -10,7 +10,6 @@ import ( "github.com/hashicorp/terraform-plugin-sdk/terraform" "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/acceptance" "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/clients" - "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/features" ) func TestAccAzureRMManagementGroup_basic(t *testing.T) { @@ -33,11 +32,6 @@ func TestAccAzureRMManagementGroup_basic(t *testing.T) { } func TestAccAzureRMManagementGroup_requiresImport(t *testing.T) { - if !features.ShouldResourcesBeImported() { - t.Skip("Skipping since resources aren't required to be imported") - return - } - data := acceptance.BuildTestData(t, "azurerm_management_group", "test") resource.ParallelTest(t, resource.TestCase{ @@ -278,7 +272,7 @@ resource "azurerm_management_group" "test" { } resource "azurerm_management_group" "import" { - group_id = azurerm_management_group.test.group_id + name = azurerm_management_group.test.name } ` } diff --git a/website/docs/d/management_group.html.markdown b/website/docs/d/management_group.html.markdown index 2690d3c90e94..cbec61c906cb 100644 --- a/website/docs/d/management_group.html.markdown +++ b/website/docs/d/management_group.html.markdown @@ -14,7 +14,7 @@ Use this data source to access information about an existing Management Group. ```hcl data "azurerm_management_group" "example" { - group_id = "00000000-0000-0000-0000-000000000000" + name = "00000000-0000-0000-0000-000000000000" } output "display_name" { @@ -26,7 +26,11 @@ output "display_name" { The following arguments are supported: -* `group_id` - Specifies the UUID of this Management Group. +* `name` - Specifies the name or UUID of this Management Group. + +* `group_id` - Specifies the name or UUID of this Management Group. + +~> **NOTE:** The field `group_id` has been deprecated in favor of `name`. ## Attributes Reference diff --git a/website/docs/r/management_group.html.markdown b/website/docs/r/management_group.html.markdown index 7e00736d7dc2..c3384c333500 100644 --- a/website/docs/r/management_group.html.markdown +++ b/website/docs/r/management_group.html.markdown @@ -39,9 +39,13 @@ resource "azurerm_management_group" "example_child" { The following arguments are supported: -* `group_id` - (Optional) The UUID for this Management Group, which needs to be unique across your tenant - which will be generated if not provided. Changing this forces a new resource to be created. +* `name` - (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. -* `display_name` - (Optional) A friendly name for this Management Group. If not specified, this'll be the same as the `group_id`. +* `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:** The field `group_id` has been deprecated in favor of `name`. + +* `display_name` - (Optional) A friendly name for this Management Group. If not specified, this'll be the same as the `name`. * `parent_management_group_id` - (Optional) The ID of the Parent Management Group. Changing this forces a new resource to be created. @@ -67,5 +71,5 @@ The `timeouts` block allows you to specify [timeouts](https://www.terraform.io/d Management Groups can be imported using the `management group resource id`, e.g. ```shell -terraform import azurerm_management_group.example /providers/Microsoft.Management/ManagementGroups/group1 +terraform import azurerm_management_group.example /providers/Microsoft.Management/managementGroups/group1 ``` From 97d8a3143eafc7cde1e78fb9580b4ff6ca99577e Mon Sep 17 00:00:00 2001 From: Dapeng Zhang Date: Fri, 27 Mar 2020 14:25:24 +0800 Subject: [PATCH 2/4] Validate ID first before import --- .../managementgroup/resource_arm_management_group.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/azurerm/internal/services/managementgroup/resource_arm_management_group.go b/azurerm/internal/services/managementgroup/resource_arm_management_group.go index cb72ef8279fe..4d27a364052b 100644 --- a/azurerm/internal/services/managementgroup/resource_arm_management_group.go +++ b/azurerm/internal/services/managementgroup/resource_arm_management_group.go @@ -2,6 +2,7 @@ package managementgroup import ( "fmt" + azSchema "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/tf/schema" "log" "strings" "time" @@ -26,9 +27,11 @@ func resourceArmManagementGroup() *schema.Resource { Update: resourceArmManagementGroupCreateUpdate, Read: resourceArmManagementGroupRead, Delete: resourceArmManagementGroupDelete, - Importer: &schema.ResourceImporter{ - State: schema.ImportStatePassthrough, - }, + + Importer: azSchema.ValidateResourceIDPriorToImport(func(id string) error { + _, err := parse.ManagementGroupID(id) + return err + }), Timeouts: &schema.ResourceTimeout{ Create: schema.DefaultTimeout(30 * time.Minute), From fa0b1b1d787bfbbe0ab5db424f648ee0f8b9a7ba Mon Sep 17 00:00:00 2001 From: Dapeng Zhang Date: Fri, 27 Mar 2020 14:40:37 +0800 Subject: [PATCH 3/4] Goimports --- .../services/managementgroup/resource_arm_management_group.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/azurerm/internal/services/managementgroup/resource_arm_management_group.go b/azurerm/internal/services/managementgroup/resource_arm_management_group.go index 4d27a364052b..6ab888dd4d52 100644 --- a/azurerm/internal/services/managementgroup/resource_arm_management_group.go +++ b/azurerm/internal/services/managementgroup/resource_arm_management_group.go @@ -2,7 +2,6 @@ package managementgroup import ( "fmt" - azSchema "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/tf/schema" "log" "strings" "time" @@ -15,6 +14,7 @@ import ( "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/clients" "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/managementgroup/parse" "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/managementgroup/validate" + azSchema "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/tf/schema" "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/timeouts" "github.com/terraform-providers/terraform-provider-azurerm/azurerm/utils" ) From 30fe4b235ea38f0018fecf27f9f4e92b9dac36f5 Mon Sep 17 00:00:00 2001 From: Dapeng Zhang Date: Mon, 30 Mar 2020 10:58:30 +0800 Subject: [PATCH 4/4] Resolve comments --- .../managementgroup/tests/data_source_management_group_test.go | 2 +- website/docs/r/management_group.html.markdown | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/azurerm/internal/services/managementgroup/tests/data_source_management_group_test.go b/azurerm/internal/services/managementgroup/tests/data_source_management_group_test.go index d540113b1b56..405cb9cd4ff2 100644 --- a/azurerm/internal/services/managementgroup/tests/data_source_management_group_test.go +++ b/azurerm/internal/services/managementgroup/tests/data_source_management_group_test.go @@ -37,7 +37,7 @@ resource "azurerm_management_group" "test" { } data "azurerm_management_group" "test" { - group_name = azurerm_management_group.test.group_name + name = azurerm_management_group.test.name } `, data.RandomInteger) } diff --git a/website/docs/r/management_group.html.markdown b/website/docs/r/management_group.html.markdown index c3384c333500..63f5936eb659 100644 --- a/website/docs/r/management_group.html.markdown +++ b/website/docs/r/management_group.html.markdown @@ -43,8 +43,6 @@ 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:** The field `group_id` has been deprecated in favor of `name`. - * `display_name` - (Optional) A friendly name for this Management Group. If not specified, this'll be the same as the `name`. * `parent_management_group_id` - (Optional) The ID of the Parent Management Group. Changing this forces a new resource to be created.