New Resource: azurerm_monitor_action_group
#1725
Conversation
azurerm/provider.go
Outdated
| @@ -122,6 +122,7 @@ func Provider() terraform.ResourceProvider { | |||
| }, | |||
|
|
|||
| ResourcesMap: map[string]*schema.Resource{ | |||
| "azurerm_action_group": resourceArmActionGroup(), | |||
There was a problem hiding this comment.
given this a resource within Azure Monitor this should be azurerm_monitor_action_group (we should probably update the naming for the azurerm_metric_alertrule and azurerm_autoscale_setting resources too, but we should fix this for new resources going forward) #Resolved
azurerm/resource_arm_action_group.go
Outdated
|
|
||
| "short_name": { | ||
| Type: schema.TypeString, | ||
| Required: true, |
There was a problem hiding this comment.
is this only required for the SMS Receiver? if so - should this be optional? #WontFix
There was a problem hiding this comment.
It is only used in Email and SMS, but is required for the whole resource. Otherwise Azure complains with "Code": "GroupShortNameIsNullOrEmpty".
In reply to: 207882329 [](ancestors = 207882329)
azurerm/resource_arm_action_group.go
Outdated
|
|
||
| if v, ok := d.GetOk("webhook_receiver"); ok { | ||
| parameters.ActionGroup.WebhookReceivers = expandActionGroupWebHookReceiver(v.([]interface{})) | ||
| } |
There was a problem hiding this comment.
we can assign all of these directly:
emailReceiversRaw := d.Get("email_receiver").([]interface{})
emailReceivers = expandActionGroupEmailReceiver(emailReceiversRaw)
smsReceiversRaw := d.Get("sms_receiver").([]interface{})
smsReceivers = expandActionGroupSmsReceiver(smsReceiversRaw)
webhookReceiversRaw := d.Get("webhook_receiver").([]interface{})
webhookReceivers = expandActionGroupWebHookReceiver(webhookReceiversRaw)
parameters := insights.ActionGroupResource{
Location: utils.String(location),
ActionGroup: &insights.ActionGroup{
GroupShortName: utils.String(shortName),
Enabled: utils.Bool(enabled),
EmailReceivers: emailReceivers,
SmsReceivers: smsReceivers,
WebhookReceivers: webhookReceivers,
},
Tags: expandedTags,
}
``` #Resolved
azurerm/resource_arm_action_group.go
Outdated
|
|
||
| id, err := parseAzureResourceID(d.Id()) | ||
| if err != nil { | ||
| return fmt.Errorf("Error parsing action group resource ID \"%s\" during get: %+v", d.Id(), err) |
There was a problem hiding this comment.
since this is an unrecoverable state for users, this there's no point in wrapping it and we can return err directly here. #Resolved
azurerm/resource_arm_action_group.go
Outdated
| } | ||
|
|
||
| d.Set("short_name", *resp.GroupShortName) | ||
| d.Set("enabled", *resp.Enabled) |
There was a problem hiding this comment.
we can remove the pointer deference here and we should be accessing these on the .Properties block to match the Azure API Response #Resolved
There was a problem hiding this comment.
There is not .Properties field, only the embedded ActionGroup field is available.
In reply to: 207882533 [](ancestors = 207882533)
website/azurerm.erb
Outdated
| @@ -688,6 +688,10 @@ | |||
| <li<%= sidebar_current("docs-azurerm-resource-autoscale-setting") %>> | |||
| <a href="#">Monitor Resources</a> | |||
| <ul class="nav nav-visible"> | |||
| <li<%= sidebar_current("docs-azurerm-resource-action-group") %>> | |||
There was a problem hiding this comment.
this should be docs-azurerm-resource-monitor-action-group #Resolved
| --- | ||
| layout: "azurerm" | ||
| page_title: "Azure Resource Manager: azurerm_action_group" | ||
| sidebar_current: "docs-azurerm-resource-action-group" |
There was a problem hiding this comment.
this should be docs-azurerm-resource-monitor-action-group #Resolved
| The following arguments are supported: | ||
|
|
||
| * `name` - (Required) The name of the Action Group. Changing this forces a new resource to be created. | ||
| * `location` - (Required) The location of this Action Group. The only possible value is `Global`. |
There was a problem hiding this comment.
if this is the only available value, then it should be hard-coded (and not available for users to set) #Resolved
| * `enabled` - (Optional) Whether this action group is enabled. If an action group is not enabled, then none of its receivers will receive communications. Defaults to `true`. | ||
| * `email_receiver` - (Optional) The list of `email_receiver` blocks as defined below that are part of this action group. | ||
| * `sms_receiver` - (Optional) The list of `sms_receiver` blocks as defined below that are part of this action group. | ||
| * `webhook_receiver` - (Optional) The list of `webhook_receiver` blocks as defined below that are part of this action group. |
There was a problem hiding this comment.
we should update these to match the newer format:
* `email_receiver` - (Optional) One or more `email_receiver` blocks as defined below.
* `sms_receiver ` - (Optional) One or more `sms_receiver ` blocks as defined below.
* `webhook_receiver ` - (Optional) One or more `webhook_receiver ` blocks as defined below.
``` #Resolved
|
|
||
| The following attributes are exported: | ||
|
|
||
| * `id` - The Route ID. |
There was a problem hiding this comment.
The Route ID -> The ID of the Monitor Action Group #Resolved
JunyiYi
changed the title
azurerm_monitor_action_groupazurerm_monitor_action_group
JunyiYi
dismissed
tombuildsstuff’s stale review
Code updated according to the feedback.
JunyiYi
changed the title
azurerm_monitor_action_groupazurerm_monitor_action_group
There was a problem hiding this comment.
hi @JunyiYi
I've left a few minor comments in-line - the main issue being the remaining crash; if we can fix those up then we should be able to proceed with this.
Thanks!
azurerm/provider.go
Outdated
| @@ -122,6 +122,7 @@ func Provider() terraform.ResourceProvider { | |||
| }, | |||
|
|
|||
| ResourcesMap: map[string]*schema.Resource{ | |||
| "azurerm_monitor_action_group": resourceArmMonitorActionGroup(), | |||
There was a problem hiding this comment.
can we sort this alphabetically like the rest of the resources? #Resolved
| Location: utils.String(azureRMNormalizeLocation("Global")), | ||
| ActionGroup: &insights.ActionGroup{ | ||
| GroupShortName: &shortName, | ||
| Enabled: &enabled, |
There was a problem hiding this comment.
we should update these to be these utils.String(shortName) and utils.Bool(enabled) respectively #Resolved
| d.Set("name", name) | ||
| d.Set("resource_group_name", resGroup) | ||
|
|
||
| d.Set("short_name", resp.ActionGroup.GroupShortName) |
There was a problem hiding this comment.
if ActionGroup is nil here this'll crash; as such this needs to become:
if group := resp.ActionGroup; group != nil {
d.Set("short_name", group.GroupShortName)
d.Set("enabled", group.Enabled)
emailReceivers := flattenMonitorActionGroupEmailReceiver(group.EmailReceivers)
if err = d.Set("email_receiver", emailReceivers); err != nil {
return fmt.Errorf("Error flattening `email_receiver`: %+v", err)
}
smsReceivers := flattenMonitorActionGroupSmsReceiver(group.SmsReceivers)
if err = d.Set("sms_receiver", smsReceivers); err != nil {
return fmt.Errorf("Error setting `sms_receiver`: %+v", err)
}
webhookReceivers := flattenMonitorActionGroupWebHookReceiver(group.WebhookReceivers)
if err = d.Set("webhook_receiver", webhookReceivers); err != nil {
return fmt.Errorf("Error setting `webhook_receiver`: %+v", err)
}
}
(I've also fixed the error messages here, since the name/resource group aren't helpful in this context #Resolved
| "short_name": { | ||
| Type: schema.TypeString, | ||
| Required: true, | ||
| ValidateFunc: validation.NoZeroValues, |
There was a problem hiding this comment.
this string can be a maximum of 12 characters long, we should add validation to support this #Resolved
|
|
||
| The following attributes are exported: | ||
|
|
||
| * `id` - The ID of the Monitor Action Group. |
There was a problem hiding this comment.
we can ditch the word Monitor here since this is an Action Group within Azure Monitor #Resolved
| page_title: "Azure Resource Manager: azurerm_monitor_action_group" | ||
| sidebar_current: "docs-azurerm-resource-monitor-action-group" | ||
| description: |- | ||
| Manages an Action Group of Azure monitoring service |
There was a problem hiding this comment.
of Azure monitoring service -> within Azure Monitor #Resolved
|
|
||
| # azurerm_monitor_action_group | ||
|
|
||
| Manages an Action Group of Azure monitoring service. |
There was a problem hiding this comment.
of Azure monitoring service -> within Azure Monitor #Resolved
There was a problem hiding this comment.
hi @JunyiYi
Thanks for pushing the latest changes - this now LGTM 👍 I'll kick off the tests shortly
Thanks!
|
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 👉 hashibot-feedback@hashicorp.com. Thanks! |
ghost
locked and limited conversation to collaborators
In this PR, I introduced a new simple action group resource of Azure monitor.
I enabled three receivers (Email, SMS and Webhook) which should cover most of the use cases.
This PR is a first step of #1252 , because the resource (signal based alert) proposed in it relies on Action Group.