api_managment - support user assigned managed identities

[sirlatrom]

Fixes #6782.

Missing:

• Updated test
• Updated docs

[ArcturusZhang]

Hi @sirlatrom thanks for this PR!

Taking a look through this is looking good - if we can fix up the merge conflicts (there was a massive file renaming) and the minor comments and add an acceptance test for the user assigned identity (or probably an update test for updating the identity from one type to another, I assume this should be allowed, or if this is not allowed we may also need to mark the identity ForceNew), then this otherwise LGTM 👍

Thanks!

[sirlatrom]

updating the identity from one type to another, I assume this should be allowed, or if this is not allowed we may also need to mark the identity ForceNew

@ArcturusZhang Thanks for your thorough review!

I've verified that it's allowed to switch between all the four identity type values, although there is an apparently intermittent bug where all changes to the "identity" field in the API are completely ignored, with no error messages. I have opened a SR with Azure on that front, but seeing as how it works most of the times I've tried it, I would say ForceNew shouldn't be necessary.

I've tried to accommodate your suggestions to the best of my ability.

As for an acceptance test, I haven't found any for the SystemAssigned identity type anywhere, but I suppose I could add a minimal acceptance test next to the "basic" one in api_management_resource_test.go. The SR process is delaying my further efforts a bit, but I'll try and get the acceptance test ready by the end of the day.

[sirlatrom]

I've added an acceptance test which I think tests creating an API Management service with the identity type set to UserAssigned. I don't know how to make an update test, but I hope it is sufficient with the create test for now.

[ArcturusZhang]

Hi @sirlatrom thank you so much for the revisions!

While I found some potential issues about the unexpected diff given that when the user does not assign a block of identity, we will assign an identity of None for him/her. This means the identity should be Optional and Computed to handle this case. And after doing that, we will have to add an enum of None to the type attribute in identity to enable user can properly switch from like SystemAssigned to None. Please see the comment for more detail explanations.

And since the identity type can be switched could we also add a test for switching identity type? It will be a combination of the steps from other identity tests.

Thanks for the revisions again!

[sirlatrom]

Sorry I just pushed a combo of multiple PR's, will fix ASAP Fixed

[ArcturusZhang]

Hi @sirlatrom thanks for the revisions!

One thing that I forgot to mention (and may cause your extra work, sorry about this) is that the importStep in acc tests actually does the checks for you, which means we do not need to put manual check functions for those attributes values. Other than this, all looks good to me! Thanks

[katbyte]

Thanks for this PR @sirlatrom! LGTM 👍

[ghost]

This has been released in version 2.10.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.10.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!