-
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_api_management_certificate
- allow key vault certificates
#11175
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.
Thanks for the pr @MattiasAng - overall looks good but i've left a couple comments inline to address
azurerm/internal/services/apimanagement/api_management_certificate_resource.go
Show resolved
Hide resolved
5db764b
to
f2f8935
Compare
f2f8935
to
b416c65
Compare
@manicminer @katbyte This should be good now that API management version is updated. |
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 @MattiasAng - looks like we have a crash during the tests thou
"key_vault_secret_id": { | ||
Type: schema.TypeString, | ||
Optional: true, | ||
ValidateFunc: keyVaultValidate.NestedItemIdWithOptionalVersion, | ||
ConflictsWith: []string{"data", "password"}, | ||
}, | ||
|
||
"key_vault_identity_client_id": { |
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.
doses it make sense to put these into a key_vault
block?
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.
It makes sense, especially since that is how the request to API is done, but in my opinion for small resources like this it reduces the complexity if all properties are on top-level and we can handle conflicts between properties.
I don't see a lot of changes coming to this resource in the future after this is implemented.
Let me know if you want it changed.
|
Sorry, only ran keyvault tests, should be good now. See reply on your question inline.
|
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 @MattiasAng - LGTM 👍
This has been released in version 2.58.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.58.0"
}
# ... other configuration ... |
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. |
Dependency on #11146
Fixes #10138
Creating this pull request even though dependency is not merged yet since I want feedback on if implementation is OK. Unfortunately Microsoft is not so consistent with which ID of User Assigned Managed Identity they want on resources.