-
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
Add support for all action group receiver types #4260
Conversation
- ITSM - Azure App Push - Automation Runbook - Voice - Azure Function - Logic App
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 - taking a look through this is looking pretty good to me, if we can fix up the comments and add these new fields to the documentation then we should be able to run the tests and get this merged 👍
Thanks!
As this PR is being added I noticed that it still doesn't seem to have support for the UseCommonAlertSchema attribute which is exposed in the Go SDK and included in the example from #4203 Is that on purpose? |
I wrote the issue initially with that in the proposed configuration before realizing that the ArmRoleReceiver doesn't appear until the azure-sdk-for-go 2018-09-01/insights version, and broad support for UseCommonAlertSchema appears in 2019-03-01/insights. I ended up removing them both from the current commits and leaving the import version where it is at 2018-03-01. I'm willing to do another PR to add that back in and bump the API as soon as this is merged. I'll also edit the PR comments above to note that this does not address #3603. Thanks! Edit: For clarity |
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.
Thanks for the revisions @mcdafydd,
I ran the tests in our CI system and uncovered some issues. In addition to the comments i've left inline the following errors occurred:
------- Stdout: -------
=== RUN TestAccAzureRMMonitorActionGroup_singleReceiverUpdate
=== PAUSE TestAccAzureRMMonitorActionGroup_singleReceiverUpdate
=== CONT TestAccAzureRMMonitorActionGroup_singleReceiverUpdate
--- FAIL: TestAccAzureRMMonitorActionGroup_singleReceiverUpdate (92.80s)
testing.go:568: Step 1 error: errors during apply:
Error: Error creating or updating action group "acctestActionGroup-190915175608251158" (resource group "acctestRG-190915175608251158"): insights.ActionGroupsClient#CreateOrUpdate: Failure responding to request: StatusCode=400 -- Original Error: autorest/azure: Service returned an error. Status=400 Code="ItsmRegionIsNotValid" Message="ItsmRegionIsNotValid"
on /opt/teamcity-agent/temp/buildTmp/tf-test380149236/main.tf line 7:
(source code not available)
FAIL
and
------- Stdout: -------
=== RUN TestAccAzureRMMonitorActionGroup_voiceReceiver
=== PAUSE TestAccAzureRMMonitorActionGroup_voiceReceiver
=== CONT TestAccAzureRMMonitorActionGroup_voiceReceiver
--- FAIL: TestAccAzureRMMonitorActionGroup_voiceReceiver (74.13s)
testing.go:568: Step 0 error: errors during apply:
Error: Error creating or updating action group "acctestActionGroup-190915175608333284" (resource group "acctestRG-190915175608333284"): insights.ActionGroupsClient#CreateOrUpdate: Failure responding to request: StatusCode=400 -- Original Error: autorest/azure: Service returned an error. Status=400 Code="UnSupportedCountryCode" Message="The voice receiver with country code 86 is not supported."
on /opt/teamcity-agent/temp/buildTmp/tf-test031498177/main.tf line 7:
(source code not available)
FAIL
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.
I believe the comments were all addressed in recent commits.
Thanks!
Hi Tom - It looks like I accidentally forgot to click Submit Review on one of the two reviews. Is there anything else I'm supposed to do to make sure this PR is ready to be merged? Thanks! |
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.
Hi @mcdafydd,
Thank you for the revisions, taking a look at this today it seems like a bunch of the tests are failing:
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.
Updated import path to use new terraform-plugin-sdk
@katbyte Thanks for the heads up. I see the changes in master to terraform-plugin-sdk and am working on https://www.terraform.io/docs/extend/plugin-sdk.html#step-2-migrate now. |
@katbyte we should be back in business. |
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 pushing those changes - taking a look through besides a couple of minor issues with the tests (which I hope you don't mind but so that we can get this merged for v1.36 I'm going to push a commit to fix) this otherwise LGTM 👍
Thanks!
dismissing since changes have been pushed
b40255e
to
4a87ce2
Compare
Awesome! 👍 Thanks a lot for helping me through my first PR here @tombuildsstuff @katbyte |
Most recent commit should resolve merge conflicts from #4483. |
Closing this pull request in favor of #4638. |
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! |
Related issue: #4203
This PR adds support for the 6 additional action group receivers available in
github.com/Azure/azure-sdk-for-go/services/preview/monitor/mgmt/2018-03-01/insights
If acceptable, I would also suggest we update the insights API import paths to at least
github.com/Azure/azure-sdk-for-go/services/preview/monitor/mgmt/2018-09-01/insights
which includes the arm_role_receiver type as well. The Azure Portal now has a "secured webhook" type as well, but I don't see that in any of the API versions in azure-sdk-for-go master yet.
Fixes #4203
Edit: Removed "Fixes #3603". Will submit fixes for that issue in a follow-on PR.