New Resource: azurerm_api_management_api_diagnostic

[sirlatrom]

Closes #4760.
Adds support for azuremonitor as identifier to azurerm_api_management_diagnostic

Missing:

• Tests for the new resource

[hctv19]

Really looking forward to this resource being implemented - we currently have to set this configuration with ARM templates directly and there's a bug with the azurerm_template_deployment resource type that's been causing us some headaches. Thanks for this work @sirlatrom !

[jonatasbaldin]

Hi there! Look forward for this implementation too, thanks a lot for the hard work 💪

[sirlatrom]

@jackofallops Can this be put on a milestone do you think?

[jackofallops]

Hi @sirlatrom
I started to review this and noticed a bug in the azurerm_api_management_diagnostic that I think you may have based this on (since the same bug is present). Specifically the diagnosticId and apiName parameters of the CreateOrUpdate call are in the wrong order. I'm going need to spend a little time digging into the implications of fixing that before I finish up this review, so apologies in advance if it takes a little while.

I wasn't involved in the other diagnostic resource (that I remember) - is there a reason that it, and this, doesn't support the other properties of the DiagnosticContractProperties? I'd like to understand the intended use-case better, as it currently feels a little lacking?

[sirlatrom]

Hi @sirlatrom
I started to review this and noticed a bug in the azurerm_api_management_diagnostic that I think you may have based this on (since the same bug is present). Specifically the diagnosticId and apiName parameters of the CreateOrUpdate call are in the wrong order. I'm going need to spend a little time digging into the implications of fixing that before I finish up this review, so apologies in advance if it takes a little while.

I wasn't involved in the other diagnostic resource (that I remember) - is there a reason that it, and this, doesn't support the other properties of the DiagnosticContractProperties? I'd like to understand the intended use-case better, as it currently feels a little lacking?

You guessed correctly that I copied and modified the azurerm_api_management_diagnostic resource, so I assumed it did things correctly. Since this here is a new resource, I can fix the argument order for that resource in this PR, and azurerm_api_management_diagnostic can be fixed in another PR.

I'll get around to making changes and addressing your order concerns on Monday CEST.

[jackofallops]

Thanks @sirlatrom - We need to be mindful that fixing the issue doesn't create a breaking change for existing resources, such as by changing ID's, recreating resources unexpectedly etc. It may be appropriate to have the change still, but knowing the impact in the first instance is important!

Thanks again :)

[sirlatrom]

@jackofallops I've now verified that the order of arguments to apimanagement.APIDiagnosticClient.CreateOrUpdate is correct after the recently pushed commit c8ab23d.

PS: Since this is a new resource with regards to the Terraform provider, there are no existing users of it. I have not changed any ID parsing functions, but merely provided the correct order of arguments to the CreateOrUpdate function, whereas prior to c8ab23d the arguments were given in an incorrect order for apiid string and diagnosticID string.

[jackofallops]

Hi @sirlatrom

Thanks for the updates, I've left a few minor comments below, but otherwise LGTM.

Thanks!

[sirlatrom]

@jackofallops I've applied your suggestions, please let me know if any further modifications are needed.

[sirlatrom]

@tombuildsstuff I've added the client initialization in 5025a9d.

[jackofallops]

Hi @sirlatrom
I ran the tests, and they all failed with:

Error: ID contained more segments than required: "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/acctestRG-200904094049117016/providers/Microsoft.ApiManagement/service/acctestAM-200904094049117016/apis/acctestAMA-200904094049117016/diagnostics/applicationinsights"

I couldn't see an immediately obvious problem with your ID parse code, but it's somehow dropping through to the a fail on ValidateNoEmptySegments

Could you add a unit test for that parser and investigate?

[sirlatrom]

Hi @sirlatrom
I ran the tests, and they all failed with:

Error: ID contained more segments than required: "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/acctestRG-200904094049117016/providers/Microsoft.ApiManagement/service/acctestAM-200904094049117016/apis/acctestAMA-200904094049117016/diagnostics/applicationinsights"

I couldn't see an immediately obvious problem with your ID parse code, but it's somehow dropping through to the a fail on ValidateNoEmptySegments

Could you add a unit test for that parser and investigate?

@jackofallops Thanks for finding that. I've added a unit test for that parser in 936bc51, and in
24f1206 suggested that the id.Path variable that is checked in ValidateNoEmptySegments is included in the error message, to help any future recipients of that error message.

[jackofallops]

Hi @sirlatrom
Thanks for adding the additional unit test, and the additional info to the path error output. It uncovered the root of the odd issue, which I've commented on below. (I think they're the only instances, might be worth another quick scan since these resources are so similarly named)

Thanks!

[jackofallops]

Hi @sirlatrom
Hope you don't mind, but I pushed a couple fixes this morning to your branch. The basic and requiresImport are now passing, but on re-review I'm not sure the update scenario is valid? Update tests are for in-place changes and there's a change to a Forcenew property in that scenario. The config for that scenario also fails as the test2 resources have the same names as the test, so trigger an import error. Can you review the test and adjust as appropriate?
Thanks.

[sirlatrom]

Hi @sirlatrom
Hope you don't mind, but I pushed a couple fixes this morning to your branch. The basic and requiresImport are now passing, but on re-review I'm not sure the update scenario is valid? Update tests are for in-place changes and there's a change to a Forcenew property in that scenario. The config for that scenario also fails as the test2 resources have the same names as the test, so trigger an import error. Can you review the test and adjust as appropriate?
Thanks.

Hi @jackofallops,
Thanks for doing the work properly when I apparently failed to 😄
As for the update scenario, it should be testing changing the logger ID, but changing the API name should arguably result in a new resource.
I'll try and go through that scenario and make it actually do what it should.

[jackofallops]

Thanks @sirlatrom
This LGTM now, and tests are passing.

Outdated

[ghost]

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

[ghost]

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!