New Resource: azurerm_management_group
#1788
Conversation
Documentation for arm_management_group resource
There was a problem hiding this comment.
hey @justinbarias
Thanks for this PR :)
I've taken a look through and left some comments in-line, but this is off to a good start. My main 🤔 at the moment is about the resource design e.g. would the association between a Subscription and a Management Group be better as an independent resource, rather than being defined within this resource?
Thanks!
| Type: schema.TypeList, | ||
| Optional: true, | ||
| Required: false, | ||
| Computed: false, |
There was a problem hiding this comment.
we can remove Required and Computed here (since they both default to false)
|
|
||
| createManagementGroupRequest := managementgroups.CreateManagementGroupRequest{ | ||
| ID: nil, | ||
| Type: utils.String("/providers/Microsoft.Management/managementGroups"), |
There was a problem hiding this comment.
we don't need to set ID or Type here - can we remove these?
| createManagementGroupRequest := managementgroups.CreateManagementGroupRequest{ | ||
| ID: nil, | ||
| Type: utils.String("/providers/Microsoft.Management/managementGroups"), | ||
| Name: &name, |
There was a problem hiding this comment.
we shouldn't need to set the name field here since it's specified in the CreateOrUpdate method below (which forms the URI as part of the request) - so I think we can remove this?
There was a problem hiding this comment.
tried it out.. gave me a Original Error: autorest/azure: Service returned an error. Status=400 Code="BadRequest" Message="Name of the group doesn't match the group id." error..
| TenantID: &armTenantID, | ||
| DisplayName: &name, | ||
| Details: &details, | ||
| } |
There was a problem hiding this comment.
minor could we in-line the CreateManagementGroupProperties, CreateManagementGroupDetails and CreateParentGroupInfo objects below and dereference the strings using utils.String() eg
parentId := fmt.Sprintf("/providers/Microsoft.Management/managementGroups/%s", armTenantID)
properties := managementgroups.CreateManagementGroupRequest{
CreateManagementGroupProperties: &managementgroups.CreateManagementGroupProperties{
TenantID: utils.String(armTenantID),
DisplayName: utils.String(name),
Details: &managementgroups.CreateManagementGroupDetails{
Parent: &managementgroups.CreateParentGroupInfo{
ID: utils.String(parentId),
}
}
}
}
|
|
||
| managementGroupProperties := managementgroups.CreateManagementGroupProperties{ | ||
| TenantID: &armTenantID, | ||
| DisplayName: &name, |
There was a problem hiding this comment.
given Name and Display Name could be different (and have different validation requirements) - should we make this a separate field?
| Management groups can be imported using the `management group name`, e.g. | ||
|
|
||
| ```shell | ||
| terraform import azurerm_management_group.testManagementGroup /providers/Microsoft.Management/ManagementGroups/<MANAGEMENT_GROUP_NAME> |
There was a problem hiding this comment.
minor can we make this:
terraform import azurerm_management_group.test /providers/Microsoft.Management/ManagementGroups/group1
to match the other resources
|
|
||
| ## Import | ||
|
|
||
| Management groups can be imported using the `management group name`, e.g. |
There was a problem hiding this comment.
management group name -> resource id
vendor/vendor.json
Outdated
| "checksumSHA1": "dOTq3QXVgEkhOV23gEhom7VQOgk=", | ||
| "path": "github.com/Azure/azure-sdk-for-go/services/preview/resources/mgmt/2018-03-01-preview/management", | ||
| "revision": "4e8cbbfb1aeab140cd0fa97fd16b64ee18c3ca6a", | ||
| "revisionTime": "2018-07-27T22:05:59Z" |
There was a problem hiding this comment.
Lastly, as this is my first time contributing publicly with Go, I wasn't sure if I did the right thing with govendor (pulled in a specific package from the Azure GO SDK).
this looks close, but we need to pin it to the version of the SDK we're using (v18.0.0) - but I'd suggest we do this once the other comments are fixed up :)
tombuildsstuff
changed the title
azurerm_management_group
Remove required, computed on subscription_ids Made CreateManagementGroupRequest properties lean Remove unnecessary evaluation of future result Modified test case with subscription to manipulate subscriptions Fixed up documentation page label Removed unnecessary examples
|
@tombuildsstuff some updates - tried assigning a policy definition to the management group.. error. Seems changes are needed to the azurerm_policy_definition resource. The policyDefinitions package has a separate method for creating policies to be assigned on the management group level (https://docs.microsoft.com/en-us/rest/api/resources/policydefinitions/createorupdateatmanagementgroup). I'm thinking adding a parameter for the azurerm_policy_definition resource of "policy_scope" which accepts either "subscription", or "managementgroup". Thoughts? |
- Allowing users to specify the Group ID - Renaming `name` to `display_name` / making it optional - Generating a Group ID / Display Name if not specified - Support for Parent Management Groups - Additional tests Tests pass: ``` $ acctests azurerm TestAccAzureRMManagementGroup_ === RUN TestAccAzureRMManagementGroup_basic --- PASS: TestAccAzureRMManagementGroup_basic (56.52s) === RUN TestAccAzureRMManagementGroup_nested --- PASS: TestAccAzureRMManagementGroup_nested (86.19s) === RUN TestAccAzureRMManagementGroup_multiLevel --- PASS: TestAccAzureRMManagementGroup_multiLevel (122.92s) === RUN TestAccAzureRMManagementGroup_multiLevelUpdated --- PASS: TestAccAzureRMManagementGroup_multiLevelUpdated (200.95s) === RUN TestAccAzureRMManagementGroup_withName --- PASS: TestAccAzureRMManagementGroup_withName (68.17s) === RUN TestAccAzureRMManagementGroup_updateName --- PASS: TestAccAzureRMManagementGroup_updateName (98.42s) === RUN TestAccAzureRMManagementGroup_withSubscriptions --- PASS: TestAccAzureRMManagementGroup_withSubscriptions (189.71s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 823.213s ```
|
hey @justinbarias Thanks for pushing those changes - I hope you don't mind but I've taken a look through and have updated the schema/added some extra tests so that we can get this merged :)
Rather than hold this PR on support for that - would you mind opening a separate issue to track this? The idea sounds sensible enough - but I'm thinking this'd be better to do conditionally on a new Thanks! |
There was a problem hiding this comment.
Dismissing since I've pushed a commit here, asking @mbfrahry to re-review
tombuildsstuff
dismissed
their stale review
Dismissing since I've pushed a commit here, asking @mbfrahry to re-review
|
Tests pass: |
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks! |
ghost
locked and limited conversation to collaborators
I don't see the need for a data source, as the most you need to interpolate is the ID (which you can do from the resource node).
Nested management groups aren't supported for now.
Also, i was trying to do some validation for the subscription ids (a subscription can only be contained in a single management group) - wasn't sure how to extend the validation engine.. Is it as simple as returning a function callback with type schema.SchemaValidateFunc?
Lastly, as this is my first time contributing publicly with Go, I wasn't sure if I did the right thing with govendor (pulled in a specific package from the Azure GO SDK).