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

restore methods with 4.19 optional API params #77

Closed
wants to merge 1 commit into from

Conversation

shwstppr
Copy link
Contributor

ACS 4.19 makes some parameters optional for some of the APIs. If they are removed from the constructors it may break compatibility with older ACS releases. Since golang doesn't support method overloading it would be better to keep the original methods with all params including params which are optional now.

@weizhouapache
Copy link
Member

looks ok to me

@hrak @rohityadavcloud
what's your opinion ?

Copy link
Member

@rohityadavcloud rohityadavcloud left a comment

Choose a reason for hiding this comment

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

LGTM - but needs review/testing

@vishesh92
Copy link
Member

Next time we generate the code, we will have to do these changes again. If we plan to support this for next release only, then it's fine. Otherwise it would be better to make it part of the code generation itself.

@weizhouapache
Copy link
Member

Next time we generate the code, we will have to do these changes again. If we plan to support this for next release only, then it's fine. Otherwise it would be better to make it part of the code generation itself.

@vishesh92
yes, that's what have agreed.
we need to look for a permanant fix

@vishesh92
Copy link
Member

If someone upgrades their cloudstack-go SDK and builds their project, the build will fail only if some method which is now changed is being used. Any good IDE should be able to list down the parts of code where a change needs to be done.

IMO, we should mention the methods in the release notes instead of ensuring backward compatibility for cloudstack-go.

Another alternative approach I can think of is generating multiple modules as per the ACS version. Every release of cloudstack-go will have multiple modules for different ACS versions which are being supported at the time. (e.g. 4.18, 4.19, etc.). We can ignore minor version for this and ensure that we are backward compatible for all major versions. This would still require a code change, but can be easily done my a simple "Find & Replace" of the module.

e.g. "github.com/apache/cloudstack-go/v2/cloudstack" will need to be replaced with "github.com/apache/cloudstack-go/v2/cloudstack/4.19"

@weizhouapache
Copy link
Member

Another alternative approach I can think of is generating multiple modules as per the ACS version. Every release of cloudstack-go will have multiple modules for different ACS versions which are being supported at the time. (e.g. 4.18, 4.19, etc.). We can ignore minor version for this and ensure that we are backward compatible for all major versions. This would still require a code change, but can be easily done my a simple "Find & Replace" of the module.

e.g. "github.com/apache/cloudstack-go/v2/cloudstack" will need to be replaced with "github.com/apache/cloudstack-go/v2/cloudstack/4.19"

@vishesh92
good opinion 👍

@shwstppr shwstppr closed this Mar 6, 2024
shwstppr pushed a commit that referenced this pull request Mar 14, 2024
Add optional params from #77 to the requiredParams.go.
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.

4 participants