Skip to content
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_monitor_action_group #1725

Merged
merged 17 commits into from
Aug 10, 2018
Merged

Conversation

JunyiYi
Copy link

@JunyiYi JunyiYi commented Aug 3, 2018

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.

@JunyiYi JunyiYi requested review from katbyte and WodansSon August 3, 2018 22:37
@JunyiYi JunyiYi self-assigned this Aug 3, 2018
@metacpp metacpp added this to the 1.13.0 milestone Aug 4, 2018
@metacpp metacpp self-requested a review August 4, 2018 15:37
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @JunyiYi

I've copied over the comments from #1714 - if you can work through the comments then we should be able to run the tests and take a look at this.

Thanks!

@@ -122,6 +122,7 @@ func Provider() terraform.ResourceProvider {
},

ResourcesMap: map[string]*schema.Resource{
"azurerm_action_group": resourceArmActionGroup(),
Copy link
Contributor

@tombuildsstuff tombuildsstuff Aug 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


"short_name": {
Type: schema.TypeString,
Required: true,
Copy link
Contributor

@tombuildsstuff tombuildsstuff Aug 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this only required for the SMS Receiver? if so - should this be optional? #WontFix

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)


if v, ok := d.GetOk("webhook_receiver"); ok {
parameters.ActionGroup.WebhookReceivers = expandActionGroupWebHookReceiver(v.([]interface{}))
}
Copy link
Contributor

@tombuildsstuff tombuildsstuff Aug 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


id, err := parseAzureResourceID(d.Id())
if err != nil {
return fmt.Errorf("Error parsing action group resource ID \"%s\" during get: %+v", d.Id(), err)
Copy link
Contributor

@tombuildsstuff tombuildsstuff Aug 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since this is an unrecoverable state for users, this there's no point in wrapping it and we can return err directly here. #Resolved

}

d.Set("short_name", *resp.GroupShortName)
d.Set("enabled", *resp.Enabled)
Copy link
Contributor

@tombuildsstuff tombuildsstuff Aug 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can remove the pointer deference here and we should be accessing these on the .Properties block to match the Azure API Response #Resolved

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is not .Properties field, only the embedded ActionGroup field is available.


In reply to: 207882533 [](ancestors = 207882533)

@@ -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") %>>
Copy link
Contributor

@tombuildsstuff tombuildsstuff Aug 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"
Copy link
Contributor

@tombuildsstuff tombuildsstuff Aug 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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`.
Copy link
Contributor

@tombuildsstuff tombuildsstuff Aug 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Contributor

@tombuildsstuff tombuildsstuff Aug 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Contributor

@tombuildsstuff tombuildsstuff Aug 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Route ID -> The ID of the Monitor Action Group #Resolved

@tombuildsstuff tombuildsstuff modified the milestones: 1.13.0, Future Aug 6, 2018
@tombuildsstuff tombuildsstuff removed the request for review from metacpp August 6, 2018 13:07
@JunyiYi JunyiYi changed the title New Resource: azurerm_monitor_action_group [WIP] New Resource: azurerm_monitor_action_group Aug 6, 2018
@JunyiYi JunyiYi dismissed tombuildsstuff’s stale review August 7, 2018 02:01

Code updated according to the feedback.

@JunyiYi JunyiYi changed the title [WIP] New Resource: azurerm_monitor_action_group New Resource: azurerm_monitor_action_group Aug 7, 2018
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

@@ -122,6 +122,7 @@ func Provider() terraform.ResourceProvider {
},

ResourcesMap: map[string]*schema.Resource{
"azurerm_monitor_action_group": resourceArmMonitorActionGroup(),
Copy link
Contributor

@tombuildsstuff tombuildsstuff Aug 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we sort this alphabetically like the rest of the resources? #Resolved

Location: utils.String(azureRMNormalizeLocation("Global")),
ActionGroup: &insights.ActionGroup{
GroupShortName: &shortName,
Enabled: &enabled,
Copy link
Contributor

@tombuildsstuff tombuildsstuff Aug 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Contributor

@tombuildsstuff tombuildsstuff Aug 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
Copy link
Contributor

@tombuildsstuff tombuildsstuff Aug 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Contributor

@tombuildsstuff tombuildsstuff Aug 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

@tombuildsstuff tombuildsstuff Aug 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

of Azure monitoring service -> within Azure Monitor #Resolved


# azurerm_monitor_action_group

Manages an Action Group of Azure monitoring service.
Copy link
Contributor

@tombuildsstuff tombuildsstuff Aug 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

of Azure monitoring service -> within Azure Monitor #Resolved

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @JunyiYi

Thanks for pushing the latest changes - this now LGTM 👍 I'll kick off the tests shortly

Thanks!

@tombuildsstuff
Copy link
Contributor

Tests pass:

screenshot 2018-08-10 at 14 06 36

@tombuildsstuff tombuildsstuff modified the milestones: Future, 1.13.0 Aug 10, 2018
@tombuildsstuff tombuildsstuff merged commit 6aa3dc0 into master Aug 10, 2018
@tombuildsstuff tombuildsstuff deleted the resource_actiongroup branch August 10, 2018 12:08
tombuildsstuff added a commit that referenced this pull request Aug 10, 2018
@ghost
Copy link

ghost commented Mar 30, 2020

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 ghost locked and limited conversation to collaborators Mar 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants