Introduce Action Group resource of Azure Monitor #1419

JunyiYi · 2018-06-21T00:32:09Z

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.

(fixes #1252)

WodansSon · 2018-07-13T23:13:38Z

azurerm/resource_arm_action_group.go

+			"name": {
+				Type:     schema.TypeString,
+				Required: true,
+				ForceNew: true,


Could we add some validation here, like no empty values?

WodansSon · 2018-07-13T23:15:59Z

azurerm/resource_arm_action_group.go

+			},
+
+			"location":            locationSchema(),
+			"resource_group_name": resourceGroupNameSchema(),


This is really minor, but can we get a space between these two?

WodansSon · 2018-07-13T23:17:34Z

azurerm/resource_arm_action_group.go

+			"location":            locationSchema(),
+			"resource_group_name": resourceGroupNameSchema(),
+
+			"short_name": {


Can we ensure that there is no empty value check here as well?

ValidateFunc: validation.NoZeroValues,

Nice catch 👍

WodansSon · 2018-07-13T23:29:04Z

azurerm/resource_arm_action_group.go

+					Schema: map[string]*schema.Schema{
+						"name": {
+							Type:     schema.TypeString,
+							Required: true,


Same as above? Can we please validate all values that should not be empty?

Tried with Azure APIs to see which fields should be non-empty.

WodansSon · 2018-07-13T23:42:16Z

azurerm/resource_arm_action_group.go

+	parameters := insights.ActionGroupResource{
+		Location: utils.String(location),
+		ActionGroup: &insights.ActionGroup{
+			GroupShortName:   utils.String(shortName),


I don't think you need to use utils.String here you can just use & to get the value.

WodansSon · 2018-07-13T23:53:36Z

azurerm/resource_arm_action_group.go

+
+func expandActionGroupEmailReceiver(d *schema.ResourceData) *[]insights.EmailReceiver {
+	v, ok := d.GetOk("email_receiver")
+	if !ok {


Could we always return a valid empty array here (i.e. and only if the value is not nil and the length is greater than 0). In all cases we should return an empty array rather than nil.

Absolutely.

WodansSon · 2018-07-13T23:57:10Z

azurerm/resource_arm_action_group.go

+	return &receivers
+}
+
+func flattenActionGroupEmailReceiver(receivers *[]insights.EmailReceiver) []interface{} {


Minor, generally we group expand and flattens together. Could we reorder these so they are grouped together?

Definitely.

WodansSon · 2018-07-14T00:03:26Z

azurerm/resource_arm_action_group.go

+func expandActionGroupSmsReceiver(d *schema.ResourceData) *[]insights.SmsReceiver {
+	v, ok := d.GetOk("sms_receiver")
+	if !ok {
+		return nil


Unless there is a reason to return nil here, can we return an empty array as above?

WodansSon · 2018-07-14T00:04:41Z

azurerm/resource_arm_action_group.go

+func expandActionGroupWebHookReceiver(d *schema.ResourceData) *[]insights.WebhookReceiver {
+	v, ok := d.GetOk("webhook_receiver")
+	if !ok {
+		return nil


Unless there is a reason to return nil here, can we return an empty array as above?

WodansSon · 2018-07-14T00:08:26Z

azurerm/import_arm_action_group_test.go

+	"github.com/hashicorp/terraform/helper/resource"
+)
+
+func TestAccAzureRMActionGroup_importBasic(t *testing.T) {


Minor, you could move the import tests in with the regular tests if you would like. It looks like this is the direction that other providers are going, so we may want to do this as well.

Moved to resource_arm_action_group_test.go

Updated the code according to the reviews.

WodansSon

Hey @JunyiYi , everything LGTM except for the import test part, I think I may not have been clear in my communication of what I suggested to be changed, my bad. Can you please update the PR per my new inline suggestion?

WodansSon · 2018-07-27T22:51:29Z

azurerm/resource_arm_action_group_test.go

+	})
+}
+
+func TestAccAzureRMActionGroup_importBasic(t *testing.T) {


Can we delete the import function and add the import test as an additional test step in the existing resource test? Sorta like this (do this for all import tests in the PR)...

Steps: []resource.TestStep{ { Config: config, Check: resource.ComposeTestCheckFunc( testCheckAzureRMActionGroupExists(resourceName), resource.TestCheckResourceAttr(resourceName, "enabled", "true"), resource.TestCheckResourceAttr(resourceName, "email_receiver.#", "2"), resource.TestCheckResourceAttr(resourceName, "email_receiver.0.email_address", "admin@contoso.com"), resource.TestCheckResourceAttr(resourceName, "email_receiver.1.email_address", "devops@contoso.com"), resource.TestCheckResourceAttr(resourceName, "sms_receiver.#", "1"), resource.TestCheckResourceAttr(resourceName, "sms_receiver.0.country_code", "1"), resource.TestCheckResourceAttr(resourceName, "sms_receiver.0.phone_number", "1231231234"), resource.TestCheckResourceAttr(resourceName, "webhook_receiver.#", "1"), resource.TestCheckResourceAttr(resourceName, "webhook_receiver.0.service_uri", "http://example.com/alert"), ), }, { ResourceName: resourceName, ImportState: true, ImportStateVerify: true, },

I've changed the test cases.

Alread updated code according to the feedback.

WodansSon

@JunyiYi, Thanks for updating the tests. LGTM now!

WodansSon · 2018-08-01T22:20:07Z

tombuildsstuff

hi @JunyiYi @jeffreyCline

Unfortunately this PR has several issues (which I've left comments for) - but primarily there's a number of potential crashes which could occur due to invalid API responses which need to be addressed; so that we're able to merge other PR's I'm going to have to revert this PR for the moment. In order to get this merged please open a new PR to address the comments and we'll be happy to take another look.

tombuildsstuff · 2018-08-02T07:52:50Z

azurerm/resource_arm_action_group.go

+		for _, receiver := range *receivers {
+			val := make(map[string]interface{}, 0)
+			val["name"] = *receiver.Name
+			val["service_uri"] = *receiver.ServiceURI


there's two potential crashes here - these should be nil-checked

Azure won't allow nil values for these fields.

tombuildsstuff · 2018-08-02T07:52:59Z

azurerm/resource_arm_action_group.go

+			val := make(map[string]interface{}, 0)
+			val["name"] = *receiver.Name
+			val["country_code"] = *receiver.CountryCode
+			val["phone_number"] = *receiver.PhoneNumber


there's three potential crashes here - can we nil-check these?

Azure won't allow nil values for these fields.

tombuildsstuff · 2018-08-02T07:53:07Z

azurerm/resource_arm_action_group.go

+		for _, receiver := range *receivers {
+			val := make(map[string]interface{}, 0)
+			val["name"] = *receiver.Name
+			val["email_address"] = *receiver.EmailAddress


there's two potential crashes here - can we nil-check these?

Azure won't allow nil values for these fields.

tombuildsstuff · 2018-08-02T07:53:35Z

azurerm/resource_arm_action_group.go

+	}
+
+	d.Set("short_name", *resp.GroupShortName)
+	d.Set("enabled", *resp.Enabled)


we don't need to de-pointer these; also we should be accessing these in the .Properties block

It is a shortcut in go instead of accessing .Properties.

tombuildsstuff · 2018-08-02T07:54:07Z

azurerm/resource_arm_action_group.go

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


we don't need to wrap this error message

That's my personal style. I don't think it really matters.

tombuildsstuff · 2018-08-02T08:02:34Z

website/docs/r/action_group.html.markdown

+* `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.


All three of these should be updated to match the other resources:

* `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.

Align with whom? Check this documentation: https://github.com/terraform-providers/terraform-provider-azurerm/blob/master/website/docs/r/dns_ptr_record.html.markdown#argument-reference; and this one: https://github.com/terraform-providers/terraform-provider-azurerm/blob/master/website/docs/r/virtual_machine.html.markdown#argument-reference. None of them use your text.

Newer resources / changes use this format, there's a PR in-flight to migrate others over e.g.

https://www.terraform.io/docs/providers/azurerm/r/virtual_machine.html#storage_data_disk

https://www.terraform.io/docs/providers/azurerm/r/kubernetes_cluster.html#agent_pool_profile

tombuildsstuff · 2018-08-02T08:02:59Z

website/docs/r/action_group.html.markdown

+
+The following attributes are exported:
+
+* `id` - The Route ID.


presumably this should be "The ID of the Action Group"?

tombuildsstuff · 2018-08-02T08:04:50Z

azurerm/resource_arm_action_group_test.go

+					resource.TestCheckResourceAttr(resourceName, "sms_receiver.0.country_code", "1"),
+					resource.TestCheckResourceAttr(resourceName, "sms_receiver.0.phone_number", "1231231234"),
+					resource.TestCheckResourceAttr(resourceName, "webhook_receiver.#", "1"),
+					resource.TestCheckResourceAttr(resourceName, "webhook_receiver.0.service_uri", "http://example.com/alert"),


can we add some tests to go from empty -> basic and ensure these fields are set?

tombuildsstuff · 2018-08-02T08:06:02Z

azurerm/resource_arm_action_group_test.go

+				ResourceName:      resourceName,
+				ImportState:       true,
+				ImportStateVerify: true,
+			},


in general we don't test importing the updates, but rather full configurations. in this case can we split this test into two: one to test with it in the disabled state, and one to ensure it can go from disabled -> enabled -> disabled

tombuildsstuff · 2018-08-02T08:24:55Z

azurerm/resource_arm_action_group.go

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


we don't need to wrap this error message - so can we just return it directly?

Align with all other error messages in this code base would be a good style.

we consistently across the codebase don't wrap these error messages since they're an internal error users aren't going to be able to work around (vs a temporary issue in the Azure API in CRUD/Polling where they may be able to self-diagnose) - as such ID parsing error don't need wrapping.

ghost · 2019-03-06T14:14:26Z

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!

Add new Action Group resource of Azure Monitor

1ec27cc

JunyiYi self-assigned this Jun 21, 2018

Junyi Yi added 4 commits June 21, 2018 15:37

Add documentation of azurerm_action_group

4584f46

Fix location description in documentation.

24ed6ac

Add azurerm_action_group page to the ToC

3c89f4a

Add acceptance tests for azurerm_action_group

1f3eba2

JunyiYi changed the title ~~[WIP] Introduce Action Group resource of Azure Monitor~~ Introduce Action Group resource of Azure Monitor Jun 22, 2018

JunyiYi requested review from katbyte, WodansSon, tombuildsstuff and metacpp June 22, 2018 00:37

Junyi Yi added 3 commits June 21, 2018 18:32

Add import tests for azurerm_action_group

d2f5e65

Resolve conflict with master branch

bc9e096

Add action group resource back to providers.

21e1183

WodansSon added this to the 1.10.0 milestone Jul 13, 2018

WodansSon added the new-resource label Jul 13, 2018

WodansSon reviewed Jul 13, 2018

View reviewed changes

WodansSon previously requested changes Jul 14, 2018

View reviewed changes

tombuildsstuff modified the milestones: 1.10.0, Soon Jul 16, 2018

WodansSon added the service/monitor label Jul 16, 2018

Modify the code according to the feedbacks

589eb7e

WodansSon previously requested changes Jul 27, 2018

View reviewed changes

Junyi Yi added 3 commits July 29, 2018 21:40

Resolve conflict with master branch

463341e

Upgrade to the latest Go SDK

e498d2d

Include import test as one step after normal tests

f26338b

WodansSon approved these changes Aug 1, 2018

View reviewed changes

WodansSon merged commit 054a76b into master Aug 1, 2018

WodansSon deleted the resource_actiongroup branch August 1, 2018 22:55

tombuildsstuff reviewed Aug 2, 2018

View reviewed changes

katbyte restored the resource_actiongroup branch August 2, 2018 23:11

metacpp mentioned this pull request Aug 2, 2018

New Resource: azurerm_monitor_action_group #1714

Closed

JunyiYi mentioned this pull request Aug 3, 2018

New Resource: azurerm_monitor_action_group #1725

Merged

tombuildsstuff modified the milestones: Soon, Being Sorted Oct 25, 2018

ghost locked and limited conversation to collaborators Mar 6, 2019


		The following attributes are exported:

		* `id` - The Route ID.

Introduce Action Group resource of Azure Monitor #1419

Introduce Action Group resource of Azure Monitor #1419

Conversation

JunyiYi commented Jun 21, 2018 • edited by WodansSon Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WodansSon Jul 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WodansSon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WodansSon left a comment

Choose a reason for hiding this comment

WodansSon commented Aug 1, 2018

tombuildsstuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tombuildsstuff Aug 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Mar 6, 2019

JunyiYi commented Jun 21, 2018 •

edited by WodansSon

Loading

WodansSon Jul 13, 2018 •

edited

Loading

tombuildsstuff Aug 3, 2018 •

edited

Loading