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 optional params for 4.19 compatability #80

Merged
merged 3 commits into from
Mar 14, 2024

Conversation

vishesh92
Copy link
Member

Add optional params from #77 to the requiredParams.go.

Copy link
Contributor

@shwstppr shwstppr left a comment

Choose a reason for hiding this comment

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

Code LGTM

@weizhouapache
Copy link
Member

@vishesh92
clustertype is always an optional parameter, right?
In the official ACS/cloudstack-go releases I mean.

@weizhouapache
Copy link
Member

I checked the diff betwen this PR and latest v2.15.0

  1. guest OS (no need to fix I think)
 type GuestOSServiceIface interface {
        AddGuestOs(p *AddGuestOsParams) (*AddGuestOsResponse, error)
-       NewAddGuestOsParams(details map[string]string, oscategoryid string, osdisplayname string) *AddGuestOsParams
+       NewAddGuestOsParams(oscategoryid string, osdisplayname string) *AddGuestOsParams
        AddGuestOsMapping(p *AddGuestOsMappingParams) (*AddGuestOsMappingResponse, error)

...
-       NewUpdateGuestOsParams(details map[string]string, id string, osdisplayname string) *UpdateGuestOsParams
+       NewUpdateGuestOsParams(id string, osdisplayname string) *UpdateGuestOsParams

  1. CKS
diff --git a/cloudstack/KubernetesService.go b/cloudstack/KubernetesService.go
index 07bcd3e..4dcf1f1 100644
--- a/cloudstack/KubernetesService.go
+++ b/cloudstack/KubernetesService.go
@@ -31,7 +31,7 @@ type KubernetesServiceIface interface {
        AddKubernetesSupportedVersion(p *AddKubernetesSupportedVersionParams) (*AddKubernetesSupportedVersionResponse, error)
        NewAddKubernetesSupportedVersionParams(mincpunumber int, minmemory int, semanticversion string) *AddKubernetesSupportedVersionParams
        CreateKubernetesCluster(p *CreateKubernetesClusterParams) (*CreateKubernetesClusterResponse, error)
-       NewCreateKubernetesClusterParams(description string, kubernetesversionid string, name string, serviceofferingid string, size int64, zoneid string) *CreateKubernetesClusterParams
+       NewCreateKubernetesClusterParams(clustertype string, name string, zoneid string) *CreateKubernetesClusterParams
        DeleteKubernetesCluster(p *DeleteKubernetesClusterParams) (*DeleteKubernetesClusterResponse, error)
        NewDeleteKubernetesClusterParams(id string) *DeleteKubernetesClusterParams
        DeleteKubernetesSupportedVersion(p *DeleteKubernetesSupportedVersionParams) (*DeleteKubernetesSupportedVersionResponse, error)


@weizhouapache
Copy link
Member

thanks @vishesh92

it looks terraform will not work

https://github.com/apache/cloudstack-terraform-provider/blob/d524e07e497b44caf3252fe9ffeefbc6bd138570/cloudstack/resource_cloudstack_kubernetes_cluster.go#L165

	// Create a new parameter struct
	p := cs.Kubernetes.NewCreateKubernetesClusterParams(name, kubernetesVersionID, name, serviceOfferingID, size, zoneID)

Copy link
Member

@weizhouapache weizhouapache left a comment

Choose a reason for hiding this comment

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

checked the diff between v2.15.0 and this PR, did not find other compatibility issues except the guest OS

I think it is good to merge. @vishesh92 thanks

@shwstppr shwstppr merged commit f763951 into apache:main Mar 14, 2024
1 check passed
@vishesh92 vishesh92 deleted the 4.19-backward-compat branch March 14, 2024 12:00
shwstppr pushed a commit that referenced this pull request May 9, 2024
After merging PR #80, optional params always needs to be set.
In case of the optional parameter of type UUID, the only option is to set the value to an empty string. This results in a request to ACS with the optional parameter in the request with an empty string. ACS throws an error because it can't find the associated id in the database.

Actual change is only in generate.go. Rest of the changes are in generated code.
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