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

v2.15.0 breaking change regarding displayText handling for CreateNetwork #70

Closed
hrak opened this issue Aug 11, 2023 · 10 comments
Closed
Assignees

Comments

@hrak
Copy link

hrak commented Aug 11, 2023

PR #53 introducing CS 4.18 support removed displayText as a parameter to NewCreateNetworkParams(). This makes sense when talking to CS 4.18 (the parameter is no longer mandatory in CS 4.18), but when talking to CS < 4.18, this introduced a breaking change in the old behavior, resulting in Unable to execute API command createnetwork due to missing parameter displaytext.

func (s *NetworkService) NewCreateNetworkParams(name string, networkofferingid string, zoneid string) *CreateNetworkParams {

I am not entirely sure whether this is something that needs to be fixed here, or in implementations of cloudstack-go, but its a undocumented breaking change nonetheless.

@shwstppr
Copy link
Contributor

shwstppr commented Feb 9, 2024

@rohityadavcloud @hrak what should be the way forward here? With each ACS release, there will be some breaking changes so should we try to maintain backward compatibility or should we document them?
Even with the current 4.19 support, there are many such changes. Eg: https://github.com/apache/cloudstack-go/pull/76/files#diff-b5f2881ff8e94586b79e300b43bb24a69303821045c600afc70b0ce265141bcdL460-R460

@rohityadavcloud
Copy link
Member

Since this is optional in newer versions but not removed, the go-sdk can still call/pass it?

@weizhouapache
Copy link
Member

Since this is optional in newer versions but not removed, the go-sdk can still call/pass it?

@rohityadavcloud
as understand, the main problem is, users have to adapt their code for the newer version, which cause backward compatibility

for example, with old go-sdk, they can create networkACL parameter by

params := cloudstack.NetworkACLServiceIface.NewCreateNetworkACLListParams(name, vpcId)

now they have to use

params := cloudstack.NetworkACLServiceIface.NewCreateNetworkACLListParams(name)
params.SetVpcId(vpcId)

Both methods should be supported.

@rohityadavcloud
Copy link
Member

Agree @weizhouapache - ideally only the older method should be supported. However, we don't promise backward compatibility with the go-sdk, your proposal makes sense.

@shwstppr
Copy link
Contributor

@rohityadavcloud @weizhouapache @hrak I looked into generate code and currently I didn't find a way to effectively find such breaking changes. Maybe we need to use two listApis.json - one latest and one from the previous release to address such breaking changes. Any ideas?
Would it make sense to continue 2.16.0 release while we work on this issue? We can create a new release once this is fixed.

@weizhouapache
Copy link
Member

@shwstppr
to keep backward compatibility, can we manually add the methods which are removed by #76 ?
(keep both new and old methods)

@shwstppr
Copy link
Contributor

@weizhouapache okay I'll add those removed methods

@rohityadavcloud
Copy link
Member

Agree with @weizhouapache, cc @shwstppr

@shwstppr
Copy link
Contributor

@weizhouapache @rohityadavcloud I've created #77 to restore breaking changes from ACS 4.19. Do we need to do it for 4.18 as well since this issue relates to that?

@shwstppr
Copy link
Contributor

Closing this based on recent changes and provision for forcing params to be required in #79
@hrak please reopen if we need further work.

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

No branches or pull requests

4 participants