-
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
api_managment - support user assigned managed identities #6783
Conversation
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 @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!
azurerm/internal/services/apimanagement/resource_arm_api_management.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/apimanagement/resource_arm_api_management.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/apimanagement/resource_arm_api_management.go
Outdated
Show resolved
Hide resolved
Signed-off-by: Sune Keller <absukl@almbrand.dk>
Signed-off-by: Sune Keller <absukl@almbrand.dk>
…e type is set to SystemAssigned Signed-off-by: Sune Keller <absukl@almbrand.dk>
Signed-off-by: Sune Keller <absukl@almbrand.dk>
@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 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 |
Signed-off-by: Sune Keller <absukl@almbrand.dk>
I've added an acceptance test which I think tests creating an API Management service with the identity type set to |
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 @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!
azurerm/internal/services/apimanagement/api_management_data_source.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/apimanagement/api_management_data_source.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/apimanagement/api_management_resource.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/apimanagement/api_management_resource.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/apimanagement/tests/api_management_api_resource_test.go
Outdated
Show resolved
Hide resolved
|
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 @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
azurerm/internal/services/apimanagement/tests/api_management_resource_test.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/apimanagement/tests/api_management_resource_test.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/apimanagement/tests/api_management_resource_test.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/apimanagement/tests/api_management_resource_test.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/apimanagement/tests/api_management_resource_test.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/apimanagement/tests/api_management_resource_test.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/apimanagement/tests/api_management_resource_test.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/apimanagement/tests/api_management_resource_test.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/apimanagement/tests/api_management_resource_test.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/apimanagement/tests/api_management_resource_test.go
Outdated
Show resolved
Hide resolved
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 @sirlatrom! LGTM 👍
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 ... |
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! |
Fixes #6782.
Missing: