Support for auto-mitigate for Monitor scheduled query rules alerts #13036

[cloutier]

No description provided.

[cloutier]

I've incorporated changes from feedback from @Klaas- and I'm ready for another round of review

[Klaas-]

I think you can use "ConflictsWith" to ensure that no one sets them both, right?

[cloutier]

"ConflictsWith" is interesting! I was looking at other ressources like application_gateway and they were validating in the update function, but ConflictsWith is the prefered way, right? Is it somewhat new?

[Klaas-]

I am not sure, I just started with terraform today :)

[cloutier]

Ah no problem! @katbyte and @tombuildsstuff, what do you think?

[Klaas-]

So I tested the way it's currently implemented in this PR.
The error when using thorttling and auto_mitigate together, only in apply the error will happen? Is that wanted behavior?

2nd problem: even if I leave throttling away and I test it I still get Status=400 Code="BadRequest" Message="Auto mitigation must be disabled when action suppression is set".
Looking at the debug it still sends "throttlingInMin":0.
I am guessing they need to be not send at all for that API to work -- or would this be considered an API error on microsofts part because 0 minutes equals turned off?

[Klaas-]

there is another issue: #12341 that says this will need a new azure go sdk version

[Klaas-]

I am not sure where the throttlingInMin must be suppressed when it's zero, would that happen somewhere here:

terraform-provider-azurerm/internal/services/monitor/monitor_scheduled_query_rules_alert_resource.go

Line 408 in eb2c5e8

ThrottlingInMin: utils.Int32(int32(throttling)),

or should that be handled here:
https://github.com/Azure/azure-sdk-for-go/blob/fbb66cedeb94af0a56ff6e8f31a44314af3ef114/services/preview/monitor/mgmt/2021-07-01-preview/insights/models.go#L479-L481

what I tested is inside the models.go I can just check if it's bigger than 0 and it works as expected.

I do not think it still needs a newer go sdk version, as stated in #12341, maybe that's needed for the 1 minute intervals not the statefulness.

[cloutier]

I've done changes according to feedback. I've switched to use ConflictsWith and simplified the Update function.

Ideally I would touch as little as possible "throttle" in this PR. I want to help out a coworker that needs "auto_mitigate" as soon as possible.

I'm ready for another round of review

[Klaas-]

@cloutier did you see #13213 (comment) I think you also need to change something in

terraform-provider-azurerm/internal/services/monitor/monitor_scheduled_query_rules_alert_resource.go

Line 406 in bf33fce

ThrottlingInMin: utils.Int32(int32(throttling)),

otherwise it will always send "throttlingInMin":0

I am not sure if you should change something in azure sdk or here though :)

[cloutier]

@Klaas- Yes I have! This is outside the scope of what I want to do with this PR though

[Klaas-]

but without that auto mitigate won't work

[cloutier]

Can you suggest a test scenario that worries you? I can add that

[cloutier]

Thanks for the feedback @katbyte! I've done the changes you suggested.

I've added a test for the undocumented quirk that @Klaas- was talking about, but I saw it the logs of the pipeline that it hasn't actually run:

     testcase.go:88: Acceptance tests skipped unless env 'TF_ACC' set
--- SKIP: TestAccMonitorScheduledQueryRules_AutoMitigate (0.00s)

When is that test actually run? Also should I do things locally to run the test or is there a way to run it in this PR verification?

[Klaas-]

@cloutier I don't think the PR tests actually do tests on azure, if I understood https://github.com/hashicorp/terraform-provider-azurerm/blob/main/README.md#developing-the-provider correctly you need to run those manually.
for the tests I ran last friday I simply compiled your branch myself and put it into my local terraform also as described in the readme. I'll do the same tests with your updates next week (I'm off this week, and I don't have a private azure account). But as I said earlier, I don't think the changes suffice because you need to not send throttlingInMin at all which is not the case currently because the unset int32 is 0 and not nil.

[cloutier]

I think I've finally figured a way to not send throttling if it's not set (in the last commit).

My question for tests is more: Does it run once upon merges? Or just before releases? Or from time to time?

Also another question for @katbyte: The default in this PR is set to true to be consistent with other ressources in this provider, but the default in the api is false. Did I make the right choice?

[stephybun]

Hi @cloutier, thanks for the previous changes, unfortunately I was still getting test failures. I've left some comments, once those are fixed up the tests should run and then we can merge this 🙂.

[cloutier]

@stephybun Thanks for the feedback! 🙏 You are helping me getting better at Go and Terraform. Thanks also to @Klaas- for the warning about the pitfalls of this API

I think I'm ready for the final round of test and review

[Klaas-]

I've tested it, it now works and I can switch alerts from suppression to auto mitigation and back, nice work!

[stephybun]

Thanks @cloutier - tests are passing 🎆. This LGTM 🥳

[github-actions]

This functionality has been released in v2.77.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!

[github-actions]

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.