OCM-1988 | Changes to API model of addons_mgmt to support rosa cli changes

[venkateshsredhat]

As Addon Service Migration has been completed we need to update the ROSA CLI to use our API as result updating models so it reflects in ocm-sdk-go .

openshift-online/ocm-sdk-go@main...venkateshsredhat:ocm-sdk-go:OCM-1988 Generated SDK change for this .

[yashvardhan-kukreja]

@venkateshsredhat what about making a change here?

https://github.com/openshift-online/ocm-api-model/blob/main/model/addons_mgmt/v1/addon_version_type.model#L41

[yashvardhan-kukreja]

@venkateshsredhat why do we need this PR exactly?

when I tried to auto-generate the changes in ocm-sdk-go w.r.t your PR, The Golang-related changes were such as shifting from type AddonParameters to type AddonParametersList

AddonParameters has the following fields in the struct _bitmap and items

AddonParametersList has the following fields in the struct href, link , and items

But considering the fact, that the response's JSON from addons-service only returns items without any href or link field, the href and link attributes in the type AddonParametersList struct will be futile.

Also, addons_mgmt/v1/openapi.json seems to not be changing at all functionally i.e. the changes are minimal and there's no introduction of href / link under AddonParameterList type in the openapi spec.

[venkateshsredhat]

@venkateshsredhat why do we need this PR exactly?

when I tried to auto-generate the changes in ocm-sdk-go w.r.t your PR, The Golang-related changes were such as shifting from type AddonParameters to type AddonParametersList

AddonParameters has the following fields in the struct _bitmap and items

AddonParametersList has the following fields in the struct href, link , and items

But considering the fact, that the response's JSON from addons-service only returns items without any href or link field, the href and link attributes in the type AddonParametersList struct will be futile.

Also, addons_mgmt/v1/openapi.json seems to not be changing at all functionally i.e. the changes are minimal and there's no introduction of href / link under AddonParameterList type in the openapi spec.

The main idea for this was the ROSA cli while using clusters_mgmt had different return types and when i changed to using addons_mgmt there were some functions breaking because of different return type . So to better align these changes were made .

[yashvardhan-kukreja]

Imo it would be better to make ROSA use addons_mgmt with the respective (different) types for two reasons:

• Changing the type defs in the model can break the integrity of potentially existing clients of addons_mgmt SDK relying on the old types (backwards compatibility is crucial).
• More importantly, it would be faster than getting 4 PRs merged (none of whose permission our team has) across ocm-api-model and ocm-sdk-go.

Feel free to let me know your thoughts.

[venkateshsredhat]

Imo it would be better to make ROSA use addons_mgmt with the respective (different) types for two reasons:

• Changing the type defs in the model can break the integrity of potentially existing clients of addons_mgmt SDK relying on the old types (backwards compatibility is crucial).
• More importantly, it would be faster than getting 4 PRs merged (none of whose permission our team has) across ocm-api-model and ocm-sdk-go.

Feel free to let me know your thoughts.

Interesting take to this , any idea which are the existing client using addons_mgmt SDK atm ?
I know we have tickets to update OCM CLI so eventually OCM cli has to also be updated with addons_mgmt .

If go down the path where we are not making changes to Models then at ROSA side it fails at a lot of places because the mismatched return type doesn't have a lot of the methods which were present for clusters_mgmt .

Also imo opinion making this change is much better than making changes to ROSA though one crucial point from this is that we might wanna make the OCM cli change as well together so we can raise all the required API model changes together as currently it requires around 4 PRs to get this done .

@ashishmax31 any thoughts around this ?

[yashvardhan-kukreja]

Alright then, idm lgtm-ing this PR.
Looks good, I was just gearing towards having less change-footprint and if this PR getting merged helps with that, it's all good.

So /lgtm from my side.

Feel free to post on forum-cluster-management to get this PR merged.

[venkateshsredhat]

Thanks @yashvardhan-kukreja , Though i would like to know few things :
any idea which are the existing client using addons_mgmt SDK atm ?
Also Do you also feel we might wanna make the OCM cli change as well together so we can raise all the required API model changes together as currently it requires around 4 PRs to get this done ?

[yashvardhan-kukreja]

any idea which are the existing client using addons_mgmt SDK atm

addonservice client in the CS's codebase.
But it's going to be fine until it keeps its ocm-sdk-go version pinned. And before the ocm-sdk-go is updated, I imagine that client would be removed in favor of cleaning addon-related code in CS.

My preference was just emphasizing more towards having less communication overhead and choosing the quicker path.

Also Do you also feel we might wanna make the OCM cli change as well together so we can raise all the required API model changes together as currently it requires around 4 PRs to get this done ?

The first two PRs including this one would be in the ocm-api-model.
The merge of this two PRs would lead the two PR change in ocm-sdk-go.

The two PR change in ocm-sdk-go would lead the change in any client like ROSA cli wanting using this sdk.

So, just doing them in an ordered fashion makes sense or you can create dummy PRs as if the pre-requisite PRs are merged. All upto you.

[venkateshsredhat]

Okay , Thanks . @yashvardhan-kukreja could you lgtm this if all good .