Skip to content
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

Merged
merged 6 commits into from
Mar 14, 2024

Conversation

Copy link
Member

@stephybun stephybun left a 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 {
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Contributor Author

@teowa teowa Mar 11, 2024

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.

Copy link
Member

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.

Copy link
Contributor Author

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

Comment on lines 349 to 377
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,
},
}
}
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

@stephybun stephybun left a 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!

website/docs/d/stack_hci_cluster.html.markdown Outdated Show resolved Hide resolved
Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks @teowa 👍

@stephybun stephybun merged commit 716b09a into hashicorp:main Mar 14, 2024
33 checks passed
@github-actions github-actions bot added this to the v3.96.0 milestone Mar 14, 2024
reastyn pushed a commit to reastyn/terraform-provider-azurerm that referenced this pull request Mar 14, 2024
…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
stephybun added a commit that referenced this pull request Mar 15, 2024
Copy link

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants