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

azurerm_monitor_action_group: Add Arm Role Receiver and more common alert schema support #4638

Merged
merged 35 commits into from
Nov 18, 2019

Conversation

mcdafydd
Copy link
Contributor

Fixes #3603 and comments.

#4483 increased version of azure-sdk-for-go insights import to 2019-06-01. This provides broader support for common alert schema in more receiver types and adds the new ArmRoleReceiver.

@mcdafydd
Copy link
Contributor Author

I wasn't sure of the best way to get this in the list before getting #4260 merged. This PR is currently a branch off that PR.

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.

hey @mcdafydd

Thanks for this PR :)

I've taken a look through and left some comments inline, but this is looking good - if we can fix up the minor comments (and the tests pass) then this otherwise LGTM 👍

Thanks!

azurerm/data_source_monitor_action_group.go Outdated Show resolved Hide resolved
azurerm/data_source_monitor_action_group.go Outdated Show resolved Hide resolved
azurerm/data_source_monitor_action_group.go Outdated Show resolved Hide resolved
azurerm/data_source_monitor_action_group.go Outdated Show resolved Hide resolved
azurerm/data_source_monitor_action_group.go Outdated Show resolved Hide resolved
azurerm/resource_arm_monitor_action_group.go Outdated Show resolved Hide resolved
azurerm/resource_arm_monitor_action_group.go Outdated Show resolved Hide resolved
azurerm/resource_arm_monitor_action_group.go Outdated Show resolved Hide resolved
website/docs/r/monitor_action_group.html.markdown Outdated Show resolved Hide resolved
mcdafydd and others added 6 commits November 2, 2019 15:08
Co-Authored-By: Tom Harvey <tombuildsstuff@users.noreply.github.com>
Co-Authored-By: Tom Harvey <tombuildsstuff@users.noreply.github.com>
Co-Authored-By: Tom Harvey <tombuildsstuff@users.noreply.github.com>
Co-Authored-By: Tom Harvey <tombuildsstuff@users.noreply.github.com>
mcdafydd and others added 2 commits November 5, 2019 22:44
Commit remaining code review suggestions

Co-Authored-By: Tom Harvey <tombuildsstuff@users.noreply.github.com>
@mcdafydd
Copy link
Contributor Author

Hi @tombuildsstuff and @katbyte - just checking in to confirm whether there is anything else you need from me to get this PR merged. Thanks!

@ghost ghost removed the waiting-response label Nov 12, 2019
@katbyte katbyte changed the title Action groups: Add Arm Role Receiver and more common alert schema support azurerm_monitor_action_group: Add Arm Role Receiver and more common alert schema support Nov 17, 2019
@tombuildsstuff
Copy link
Contributor

@mcdafydd apologies - I can't submit the Review since Github appears to be having issues (but I'll try again in a bit) - what I'm trying to post is / why I've pushed a commit:


hey @mcdafydd

Thanks for pushing those changes - apologies for the delayed re-review here!

Running the tests it appears that a few of the assertions are incorrect which is causing them to fail - in particular accessing a list via foo.bar rather than foo.0.bar. Whilst I've gone through and commented where appropriate - for the Resource instead we can take a different approach and use the Import test to verify the fields match the Terraform Configuration (which means we can remove all of the assertions).

As such I hope you don't mind but so that we can get this merged I'm going to push a commit which fixed up the remaining comments and then switches the assertions within the resource for import tests

Thanks!

@tombuildsstuff
Copy link
Contributor

hey @mcdafydd

So running the tests on the whole these look pretty good:

Screenshot 2019-11-18 at 13 09 12

The 4 failing tests are due to #3806 - since that's an existing known issue (and outside the scope of this PR) I think we should be good to merge this for the moment; since from what I can see these should pass once #3806 is resolved.

Thanks!

@tombuildsstuff tombuildsstuff dismissed their stale review November 18, 2019 11:18

changes have been pushed - and Github's down so I can't approve it in a new review 🙃

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.

LGTM 👍

@tombuildsstuff tombuildsstuff merged commit 08cff6a into hashicorp:master Nov 18, 2019
@tombuildsstuff tombuildsstuff added this to the v1.37.0 milestone Nov 18, 2019
tombuildsstuff added a commit that referenced this pull request Nov 18, 2019
@mcdafydd mcdafydd deleted the r/remaining-action-groups branch November 18, 2019 15:21
@ghost
Copy link

ghost commented Nov 26, 2019

This has been released in version 1.37.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 = "~> 1.37.0"
}
# ... other configuration ...

@ghost
Copy link

ghost commented Mar 29, 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 👉 hashibot-feedback@hashicorp.com. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 29, 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.

Add missing features for Monitor Action Group senders
2 participants