Skip to content

Commit

Permalink
azurerm_management_group - deprecated and rename group_id to… (#6276)
Browse files Browse the repository at this point in the history 
The term group_id in management group is sometimes confusing with the UUID of the mgmt group or a full ID of a mgmt group. And a mgmt group can have a human-readable string as its group_id. Therefore group_id is not quite a good term for this.

I introduced a new attribute name for the resource and data source azurerm_management_group to provide more friendly usage of management group and reduce the confusion.
  • Loading branch information
@ArcturusZhang
ArcturusZhang authored Mar 30, 2020
1 parent 223a5ea commit 41f8fdc
Show file tree
Hide file tree
Showing 6 changed files with 85 additions and 51 deletions.
30 changes: 24 additions & 6 deletions azurerm/internal/services/managementgroup/data_source_management_group.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},

Expand Down Expand Up @@ -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)
Expand Down
80 changes: 48 additions & 32 deletions azurerm/internal/services/managementgroup/resource_arm_management_group.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,9 @@ 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"
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"
)
Expand All @@ -27,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),
Expand All @@ -40,11 +42,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": {
Expand Down Expand Up @@ -77,9 +90,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)
Expand All @@ -88,11 +104,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)
}
}

Expand All @@ -101,10 +117,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{
Expand All @@ -119,18 +135,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)
Expand All @@ -139,32 +155,31 @@ 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)
}
}
}
}
}

// 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)
}
}

Expand Down Expand Up @@ -193,6 +208,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 {
Expand Down
2 changes: 1 addition & 1 deletion azurerm/internal/services/managementgroup/tests/data_source_management_group_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ resource "azurerm_management_group" "test" {
}
data "azurerm_management_group" "test" {
group_id = azurerm_management_group.test.group_id
name = azurerm_management_group.test.name
}
`, data.RandomInteger)
}
8 changes: 1 addition & 7 deletions azurerm/internal/services/managementgroup/tests/resource_arm_management_group_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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{
Expand Down Expand Up @@ -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
}
`
}
Expand Down
8 changes: 6 additions & 2 deletions website/docs/d/management_group.html.markdown
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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" {
Expand All @@ -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

Expand Down
8 changes: 5 additions & 3 deletions website/docs/r/management_group.html.markdown
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,11 @@ 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.

* `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.

Expand All @@ -67,5 +69,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
```

0 comments on commit 41f8fdc

Please sign in to comment.