Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New Resource:
azurerm_management_group
#1788New Resource:
azurerm_management_group
#1788Changes from 4 commits
96e7730
06ce278
6e91482
632f8ee
3ddd88e
3c3d080
039373e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can remove
Required
andComputed
here (since they both default tofalse
)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
given Name and Display Name could be different (and have different validation requirements) - should we make this a separate field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor could we in-line the
CreateManagementGroupProperties
,CreateManagementGroupDetails
andCreateParentGroupInfo
objects below and dereference the strings usingutils.String()
egThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't need to set ID or Type here - can we remove these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we don't need the result of the Future here we should be able to just wait for the completion of the Future instead (also we can reuse the
err
object in the Get/wrap the errors) e.g.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
given this is an entirely different API call - I'm wondering if this'd make sense as a separate resource?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
based on the SDK package /mgmt/2018-03-01-preview/management, there are two different clients for managementgroup (client) and subscriptions (subscriptionclient) - in this case though, subscriptionclient really only gets used in the managementgroup context, and purely to associate a subscription to a management group (and not for creating subscriptions).. i think there's merit creating a dedicated subscription object, but for the creation/deletion of subscriptions (which is a different SDK package). My two cents..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use the
.Properties
object available in the SDK (which is where this actually sits in the response) - and nil-check it in-case the API Response / Swagger changes? e.g.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we wrap and return this error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we also set the
name
anddisplay_name
field here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
policy
->Management Group
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is missing a closing
]
bracket - can we add this in?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed with latest commit.