gridscale / terraform-provider-gridscale

Terraform gridscale provider
https://registry.terraform.io/providers/gridscale/gridscale/latest/docs
Mozilla Public License 2.0
12 stars 11 forks source link

bugfix/fix-differing-determinations-of-template-at-k8s-resource #393

Closed alidaw closed 1 month ago

alidaw commented 1 month ago

The implementations out for review are used to fix differing determination of tempate at the k8s resource. The determination of the service template the k8s resource shall base on partly is implemented not that well thought out and recurring code fragments are spreaded, which increases risk of failure of logic. This makes reading and finally quick understanding of the logic that is intended to rule the determination of the service template harder and recurring calls of one and the same kind of iterations better should be avoided as well from perspective of performance. The issue the merge request has been created for rather is in-between what might be placed as a feature and a bug than being a critical bug, but finally - especially at the point where the update of a k8s resource is implemented the way it is implemented bears risk and logic differs from other places where the routines happen as well. Following I'll list what works correct and what needs fix/improvement.

What works correct:

What works not that well:

What works wrong:

Goal: Come with a centric implementation of the base logic to avoid need to run recurring code fragments, spreaded and partly differing code fragments and fix what went wrong at the implementation used to apply the update of a k8s resource.

nvthongswansea commented 1 month ago

@alidaw I think we can improve the performance a bit by calling paasTemplates, err := client.GetPaaSTemplateList(context.Background()) once, and pass paasTemplates to deriveK8sTemplateFromGSKVersion and deriveK8sTemplateFromGSKRelease.

alidaw commented 1 month ago

@alidaw I think we can improve the performance a bit by calling paasTemplates, err := client.GetPaaSTemplateList(context.Background()) once, and pass paasTemplates to deriveK8sTemplateFromGSKVersion and deriveK8sTemplateFromGSKRelease.

No. That doesn't make sense to me. I've adjusted the logic in a way that in case of creation or update anyway simply the function called deriveK8sTemplateFromResourceData would get called once and there also the function where the list of available PaaS templates will get fetched once. I don't see where anything gets called twice anymore. So we do better in remaining with the fetching of the list of available PaaS templates within respective function. The information about available PaaS templates shouldn't be manipulated from outside via paramter at function's call, but should be fetched within respective function as otherwise the task of that function would be alienated as the function is intended to work as source and not to get a source from outside to do iteration on it for derivation of template by given factor like UUID, release of GSDK version.