-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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 optional location parameter and default to Global #20151
#20603
Conversation
mikemadeja
commented
Feb 21, 2023
•
edited
Loading
edited
- Add location parameter to this issue: Support for action_group location #20151 Support for action_group location #20151
- Location defaults to global to avoid breaking current configs for users
- Location forces new one to be created when changing locations
- Updated document to reflect the new parameter
Hello @stephybun, may I ask what is breaking in regards to this change? The original code defaults to "Global", which I have defaulted to. But now you can add location and set it any of the regions the API offers. |
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 this PR @mikemadeja!
I pre-emptively marked this as a breaking change under the assumption that we would use the same schema for location that we have elsewhere in the provider, however this turns out to be a unique case since it has defaulted to global
so far.
In addition to the comments left in-line we would also need to address the following points before we can get this merged:
- Normalise and set the location back to state in the read
- Add a test where location is set
Once that's done we can take another look through and hopefully get this merged.
Hello @stephybun, I added a test with location. But I am researching if anything else needs to be done to utilize those tests. Under internal/services/monitor/monitor_action_group_resource_test.go, I created a function called locationBasic and added a location field and set it to global. The test then expects global. |
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.
all tests are failing with
=== RUN TestAccMonitorActionGroup_emailReceiver
=== PAUSE TestAccMonitorActionGroup_emailReceiver
=== CONT TestAccMonitorActionGroup_emailReceiver
testcase.go:110: Step 2/2 error running import: ImportStateVerify attributes not equivalent. Difference is shown below. The - symbol indicates attributes missing after import.
map[string]string{
- "location": "global",
}
--- FAIL: TestAccMonitorActionGroup_emailReceiver (191.17s)
FAIL
i see that you pushed a fix 7 hours ago, that maybe have been just before/after i triggers them! the import state verify failing usually means there is something off in your read function, not saving location to state.
Thanks @katbyte and @stephybun. I made an error on one of the tests I copied and have updated. I also compiled it locally and verified the state is there so hopefully it was my error on the tests. May I ask where I can see the tests? Would it be this one? https://github.com/hashicorp/terraform-provider-azurerm/actions/runs/4263642693/jobs/7420650522 |
Hello @katbyte and @stephybun. Just wanted to see if there were any other concerns with this PR? Just want to make sure I didn't miss anything. :-) Thanks, Mike |
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 @mikemadeja
The GHA link you're referring to runs the unit tests in the provider. When we post test failures we're referring to the acceptance tests which are run in our CI tool. You're able to run these tests as well, just be aware that they will incur costs on your subscription since they spin up resources in Azure.
In addition to the comments on the tests we still need to set the value of location into the state in the read function, we do that by adding the following line:
d.Set("location", location.NormalizeNilable(resp.Location))
Here:
terraform-provider-azurerm/internal/services/monitor/monitor_action_group_resource.go
Lines 590 to 592 in 0186544
d.Set("name", id.Name) | |
d.Set("resource_group_name", id.ResourceGroup) | |
Once these points have been resolved this should be good to go, thanks!
internal/services/monitor/monitor_action_group_resource_test.go
Outdated
Show resolved
Hide resolved
internal/services/monitor/monitor_action_group_resource_test.go
Outdated
Show resolved
Hide resolved
Co-authored-by: stephybun <[email protected]>
Co-authored-by: stephybun <[email protected]>
Thanks @stephybun, I appreciate the help. Thought this was an easy change so I'm learning a lot on how all this works. |
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 @mikemadeja LGTM 🎉
I hope you don't mind I pushed two small changes to your branch that put the property in the correct order in the docs which I overlooked in the review, as well as to format the Terraform config in the test. Thanks again for your work on this!
Thanks @stephybun, no problem. |
This functionality has been released in v3.47.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you! |
azurerm_monitor_action_group
- Add optional location parameter and default to Global #20151
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |