-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
azurerm_monitor_action_group: Add Arm Role Receiver and more common alert schema support #4638
Conversation
- ITSM - Azure App Push - Automation Runbook - Voice - Azure Function - Logic App
- Adds arm_role_receiver - Adds use_common_alert_schema to all supported receivers - Changes require insights import change from hashicorp#4483
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. |
There was a problem hiding this 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!
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>
Commit remaining code review suggestions Co-Authored-By: Tom Harvey <tombuildsstuff@users.noreply.github.com>
Hi @tombuildsstuff and @katbyte - just checking in to confirm whether there is anything else you need from me to get this PR merged. Thanks! |
@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 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! |
hey @mcdafydd So running the tests on the whole these look pretty good: 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! |
changes have been pushed - and Github's down so I can't approve it in a new review 🙃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
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 ... |
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! |
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.