-
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_new_relic_monitor
- Support identity
#26115
azurerm_new_relic_monitor
- Support identity
#26115
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 @ms-zhenhua. It looks like there are some fundamental issues with the identity
property for this resource that should be raised with the Service Team, could you please open a swagger issue regarding the comments I left in-line and then link those in the code?
@@ -520,3 +532,34 @@ func flattenUserInfoModel(input *monitors.UserInfo) []UserInfoModel { | |||
|
|||
return append(outputList, output) | |||
} | |||
|
|||
// Currently the API only supports `SystemAssigned` type. |
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.
If the Swagger spec specifies that System and User assigned should be supported but the API in reality only supported SystemAssigned then we should raise an issue on the Rest API specs repo.
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.
created a issue to track this problem: Azure/azure-rest-api-specs#29256
if identityValue.Type != identity.TypeNone { | ||
properties.Identity = identityValue | ||
} |
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.
This isn't how the Type
field in Identity
is usually handled which would imply a bug, this should also be raised as an issue on the Rest API specs repo.
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.
created a issue to track this problem: Azure/azure-rest-api-specs#29257
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.
In the future: can we also update the API Definition to expose the Identity
field correctly too? We've spent a considerable amount of time standardising the behaviours (and methods) for the Identity
types, so we should lean on them where possible - as such I've opened hashicorp/pandora#4171 which'll allow this.
Hi @stephybun, thank you for your review. I have created the issues and added them to the comments. Please take another 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.
Thanks @ms-zhenhua LGTM 👍
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. |
Community Note
Description
Support
identity
forazurerm_new_relic_monitor
to enablemetrics flow
like data dogPR Checklist
For example: “
resource_name_here
- description of change e.g. adding propertynew_property_name_here
”Changes to existing Resource / Data Source
Testing
Change Log
Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.
azurerm_new_relic_monitor
- Supportidentity
This is a (please select all that apply):
Related Issue(s)
Fixes #0000
Note
If this PR changes meaningfully during the course of review please update the title and description as required.