Add support for event_grouping_setting of azurerm_sentinel_alert_rule_scheduled #10078
Conversation
There was a problem hiding this comment.
Thanks @neil-yechenwei - overall this looks good but i have one comment about the property name i've left inline
| @@ -73,6 +73,24 @@ func resourceSentinelAlertRuleScheduled() *schema.Resource { | |||
| ValidateFunc: validation.StringIsNotEmpty, | |||
| }, | |||
|
|
|||
| "event_grouping_setting": { | |||
There was a problem hiding this comment.
I think we can remove setting
| "event_grouping_setting": { | |
| "event_grouping": { |
| MaxItems: 1, | ||
| Elem: &schema.Resource{ | ||
| Schema: map[string]*schema.Schema{ | ||
| "aggregation_kind": { |
There was a problem hiding this comment.
given there is only one property here? maybe we should pull this out and call it
| "aggregation_kind": { | |
| "Event_grouping_aggregation_methold": { |
(kind isn't great, methold is more descriptive)
There was a problem hiding this comment.
@katbyte , I am not sure whether api would introduce other properties for setting event grouping in the future. So if I put this out, I still need to update the property schema like this and it would become breaking change once api introduced other properties. Does it make sense?
There was a problem hiding this comment.
It's "method" not "methold", right?
There was a problem hiding this comment.
Yes method, and its up to you, honestly its not a big deal to deprecate a string property if they add one
There was a problem hiding this comment.
OK. Thanks. I prefer to keep it.
|
@katbyte , thanks for your comments. I've updated code and answered your questions. Please have an another look. |
|
This has been released in version 2.44.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.44.0"
}
# ... other configuration ... |
|
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 as resolved and limited conversation to collaborators
fixes #9725
Currently azurerm_sentinel_alert_rule_scheduled doesn't support grouping the events. So this PR is to implement it.
The EventGroupingSettings in rest api spec:
https://github.com/Azure/azure-rest-api-specs/blob/96ba256c1f61d4dc2a7928d7658e024b9b1615a6/specification/securityinsights/resource-manager/Microsoft.SecurityInsights/preview/2019-01-01-preview/SecurityInsights.json#L9262
--- PASS: TestAccSentinelAlertRuleScheduled_basic (160.00s)
--- PASS: TestAccSentinelAlertRuleScheduled_complete (160.04s)
--- PASS: TestAccSentinelAlertRuleScheduled_withAlertRuleTemplateGuid (160.07s)
--- PASS: TestAccSentinelAlertRuleScheduled_requiresImport (192.13s)
--- PASS: TestAccSentinelAlertRuleScheduled_updateEventGroupingSetting (222.86s)
--- PASS: TestAccSentinelAlertRuleScheduled_update (290.24s)