-
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_stack_hci_cluster
- support for identity
, export cloud_id
, service_endpoint
, resource_provider_object_id
#25031
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 @teowa. Two questions surrounding the expanding/flattening of the identity
block. Could you also fix the linting?
@@ -132,3 +158,21 @@ func (r StackHCIClusterDataSource) Read() sdk.ResourceFunc { | |||
}, | |||
} | |||
} | |||
|
|||
func flattenSystemAssignedToModel(input *identity.SystemAndUserAssignedMap) []identity.ModelSystemAssigned { |
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.
Is there a reason we can't use the existing flatten functions in the identity
package?
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.
The service API only supports systemAssigned Identity, which means there should be no identity_ids
field in the tf schema, but the swagger and SDK uses identity.SystemAndUserAssignedMap
, so need mannually skip the identity_ids
in expand/flatten function.
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.
Then we should raise an issue on the swagger since the definition doesn't align with the API behaviour and link it here. Can you please do that?
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.
change this in swagger would be considered as breaking change, and from service team the user assigned identity will be supported in the future, can we leave it for now, and remove this when user assigned identity is supported later.
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.
@teowa we should always raise an issue on the swagger spec when what is defined in the spec does not correspond to the behaviour of the API, which is the case here. Whether the service team actually make the breaking change to fix it or not is another question, this is about tracking for when the service is brought in line. Please open an issue on the swagger spec and link it here, like we do for all other discrepancies.
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.
@stephybun thanks for clarifying , I thought I need to modify the swagger. The issue has been raised: Azure/azure-rest-api-specs#28260
func expandSystemAssigned(input []interface{}) (*identity.SystemAndUserAssignedMap, error) { | ||
if len(input) == 0 || input[0] == nil { | ||
return &identity.SystemAndUserAssignedMap{ | ||
Type: identity.TypeNone, | ||
}, nil | ||
} | ||
|
||
return &identity.SystemAndUserAssignedMap{ | ||
Type: identity.TypeSystemAssigned, | ||
}, nil | ||
} | ||
|
||
func flattenSystemAssigned(input *identity.SystemAndUserAssignedMap) []interface{} { | ||
if input == nil { | ||
return []interface{}{} | ||
} | ||
|
||
if input.Type == identity.TypeNone { | ||
return []interface{}{} | ||
} | ||
|
||
return []interface{}{ | ||
map[string]interface{}{ | ||
"type": input.Type, | ||
"principal_id": input.PrincipalId, | ||
"tenant_id": input.TenantId, | ||
}, | ||
} | ||
} |
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.
Same question here, why can't we use the expand and flatten functions in the identity
package?
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.
The service API only supports systemAssigned Identity, which means there should be no identity_ids
field in the tf schema, but the swagger and SDK uses identity.SystemAndUserAssignedMap
, so need mannually skip the identity_ids
in expand/flatten function.
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 raising the issue on the swagger spec @teowa. There's two minor fixes for the docs left to do but then this should be good to go!
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.
LGTM thanks @teowa 👍
…d`, `service_endpoint`, `resource_provider_object_id` (hashicorp#25031) * wip * support for identity, export cloud_id, service_endpoint, resource_provider_object_id * fix * fix golangci-lint error * link swagger issue * fix doc
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. |
swagger: https://github.com/Azure/azure-rest-api-specs/blob/main/specification/azurestackhci/resource-manager/Microsoft.AzureStackHCI/stable/2023-08-01/clusters.json