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

feat: add Azure Managed Identities attributes for ARO-HCP Clusters #1001

Conversation

miguelsorianod
Copy link
Contributor

We add Azure Managed Identities related attributes that are going to be leveraged for the ARO-HCP Clusters.

@miguelsorianod
Copy link
Contributor Author

miguelsorianod commented Oct 22, 2024

@machi1990 I think it should be fine to add this even when we don't have any code in CS to map in/out.

My understanding is that if the new content of the OCM SDK Go is leveraged:

  • When creating the type using the OCM SDK Go and it is sent to CS then CS will deserialize into the CS specific API type and nothing else will happen as there are no validations nor mapping to model layer.
  • When reading from CS and serializing into the OCM SDK Go type the attributes won't exist from CS so the corresponding attributes in the OCM Go SDK type will not be set or set to zero-values

Would you agree?

@machi1990
Copy link
Contributor

@machi1990 I think it should be fine to add this even when we don't have any code in CS to map in/out.

My understanding is that if the new content of the OCM SDK Go is leveraged:

  • When creating the type using the OCM SDK Go and it is sent to CS then CS will deserialize into the CS specific API type and nothing else will happen as there are no validations nor mapping to model layer.
  • When reading from CS and serializing into the OCM SDK Go type the attributes won't exist from CS so the corresponding attributes in the OCM Go SDK type will not be set or set to zero-values

Would you agree?

Yes. That's what will happen. In my opinion, it should be safe to roll this one out once review.

I've added it to my list and I'll get to reviewing it at some point today.

Copy link
Contributor

@machi1990 machi1990 left a comment

Choose a reason for hiding this comment

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

lgtm

Spotted one typo and left a corrective suggestion. Good to merge once addressed.

We add Azure Managed Identities related attributes that are going
to be leveraged for the ARO-HCP Clusters.
@miguelsorianod miguelsorianod force-pushed the add-cluster-azure-managedidentities-apitypes branch from 46ec9ad to cb9e76f Compare October 22, 2024 11:05
@miguelsorianod miguelsorianod merged commit d83a58d into openshift-online:main Oct 22, 2024
2 checks passed
@miguelsorianod miguelsorianod deleted the add-cluster-azure-managedidentities-apitypes branch October 22, 2024 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants