-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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_policy_remediation
#5746
New resource azurerm_policy_remediation
#5746
Conversation
1201796
to
4120edb
Compare
496c912
to
68a5414
Compare
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.
Thanks for the new resource @ArcturusZhang, overall this looks good but i do have some concerns about the multi-purpose ID struct you created. As mentioned in the comments i'm not sure that is ideal given not all properties are valid for all types. Additionally we can pull the switch for the create/read/delete calls out into the resource, they won't be used anywhere else?
Additional what would the use case for the data source be? will the ID be used elsewhere?
azurerm/internal/services/policyinsights/data_source_policy_remediation.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/policyinsights/data_source_policy_remediation.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/policyinsights/data_source_policy_remediation.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/policyinsights/resource_arm_policy_remediation.go
Outdated
Show resolved
Hide resolved
68a5414
to
48dc397
Compare
Well, in general this PR should not have conflicts with them, because I did not change anything else in the policy service (including the policy_definition resource) other than the addition of policy remediation. Therefore I suppose my PR should not have conflicts with them. But I think the changes for management group IDs in this PR would benefit those two PR. They are trying to handle the management group in their own resources, which I think is not a very good idea to introduce too much copy and paste work. |
I just noticed that one of them has been approved, then maybe a refinement of how to deal the management group in the policy service should be introduced after that and this PR get merged |
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.
Thanks for the changes @ArcturusZhang, just a couple more minor comments i've left inline and this should be good to merge 👍
Required: true, | ||
Type: schema.TypeString, | ||
Required: true, | ||
ValidateFunc: validate.ManagementGroupName, |
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.
the property is group_id but this is validating the name?
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.
Here group_id means this one in its ID:
/providers/Microsoft.ManagementGroup/managementGroups/{group_id}
the thing in other resources' IDs at the same position would be called as a name
, for instance
/subscriptions/{subsID}/resourceGroups/group1/providers/Microsoft.Compute/virtualMachines/{name}
And another reason is that if we call this group_id
in the validation function, the validation function would looks like ManagementGroupGroupID
or ManagementGroupID
. This would be confusing with the validation function of the real management group ID. Therefore I took this name.
Optional: true, | ||
Computed: true, | ||
ForceNew: true, | ||
ValidateFunc: validate.ManagementGroupName, |
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.
The is a group_id property but we are validating a name?
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.
Same reason as the above comment
azurerm/internal/services/policy/tests/resource_arm_policy_remediation_test.go
Outdated
Show resolved
Hide resolved
Nice to see getting closer to merge. Really looking forward to clean up our subscriptions programmatically. @ArcturusZhang Thank you for working on this, much appreciated! |
Hi @katbyte I have resolved the comments. And I just spotted a typo in the doc of |
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.
Thanks for the changes @ArcturusZhang! aside from two minor comments this LGTM 👍
azurerm/internal/services/policy/resource_arm_policy_remediation.go
Outdated
Show resolved
Hide resolved
Hi @katbyte I resolved the final two minor comments |
This has been released in version 2.3.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example: provider "azurerm" {
version = "~> 2.3.0"
}
# ... other configuration ... |
First at all, thank you for the great work. I know this is merged and you might not be following it but I discovered that ResourceDiscoveryMode option is not included. https://github.com/Azure/azure-sdk-for-go/blob/master/services/policyinsights/mgmt/2019-10-01/policyinsights/models.go#L2056 Could you please shed some light? let me know if you want me to create a new ticket. |
Well, this field initially does not exist in the version of go SDK terraform is using when I first work on this resource (Dec. 2019). I will take some time to add this into this resource, thanks for remind me. |
Makes total sense. azure is fast evolving. :) Thank you! |
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! |
This is a revised version of PR 5277.
A
azurerm_policy_remediation
is used along with aazurerm_policy_assignment
to remediate a policy.A policy can be assigned in 4 different scopes: Subscription, Resource Group, Resource and Management Group. This remediation resource supports all of them using a
scope
field similar to theazurerm_policy_assignment
.Since the id of a management group (like
/providers/Microsoft.Management/managementGroups/00000000-0000-0000-0000-000000000000
) is not a valid azure resource id, passing it toazure.ParseResourceId
will throw errors, I wrote some additional ID parsing functions to deal with this kind of situations.And additionally, since all of the ids in a remediation will be converted to lower cases, I am using case diff suppression everywhere.