-
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
azurerm_app_service - support for the acr_use_managed_identity_creds and acr_user_managed_identity_id properties #12745
Conversation
@haraldatbmw the app_service_slot_resource does not need an update directly as it shares the siteConfig mapping with the app_service_resource. |
@jackofallops This PR ready for testing. |
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 @mozts2005 - Thanks for this PR (and reworking it from the older one). I've left some comments inline below.
Also, can you add an acceptance test that confirms this is set as expected?
Thanks!
@jackofallops I have added acceptance tests and found a bug in the app_service_resource. Please review. |
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 @mozts2005 - Apologies for the delay in getting back to this review. I've left some comments / changes below. If those can be addressed, I'll get this through the test suite and we should be good to merge.
Thanks!
@jackofallops I have some questions about your review. Thanks for your help. |
4b98d11
to
cc368f3
Compare
…and acr_user_managed_identity_id properties
Co-authored-by: Steve <11830746+jackofallops@users.noreply.github.com>
Co-authored-by: Steve <11830746+jackofallops@users.noreply.github.com>
cc368f3
to
def0679
Compare
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 @mozts2005 - Thanks for the changes, this LGTM now 👍
This functionality has been released in v2.71.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! |
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. |
Fixes #12277
Fixes #7491
Fixes #9962
I have opened this as an updated version of pr #12399 to make the history clean.
Depends on #12635 which has been merged.