HPA: Remove controller.autoscaling.apiVersion, use capabilites instead.
#392
Conversation
Gacko
changed the title
controller.autoscaling.apiVersion, use capabilites inst…controller.autoscaling.apiVersion, use capabilites instead.
|
|
||
| apiVersion: {{ .Values.controller.autoscaling.apiVersion }} | ||
| {{- if and .Values.controller.autoscaling.enabled (or (eq .Values.controller.kind "Deployment") (eq .Values.controller.kind "Both")) (not .Values.controller.keda.enabled) -}} | ||
| apiVersion: {{ ternary "autoscaling/v2" "autoscaling/v2beta2" (semverCompare ">=1.23.0-0" .Capabilities.KubeVersion.Version) }} |
There was a problem hiding this comment.
We could still check for .Values.controller.autoscaling.apiVersion first. And instead of checking .Capabilities.KubeVersion.Version maybe use .Capabilities.APIVersions.Has "autoscaling/v2". See for example in upstream.
There was a problem hiding this comment.
I don't think this makes sense because the rest of the template is already made up for autoscaling/v2beta2 or higher and not working with autoscaling/v2beta1 or even autoscaling/v1. So even if you could override it, it would not work due to the .spec.
There was a problem hiding this comment.
Regarding Capabilities.APIVersions.Has: Yeah, I know. But I just sticked to most of the other manifests, they are still using the hard coded comparison.
There was a problem hiding this comment.
Ok, I was thinking about the alignment with upstream. There the setting for .Values.controller.autoscaling.apiVersion is documented and customers might be confused when we differ.
There was a problem hiding this comment.
Regarding Capabilities.APIVersions.Has: You want to know the apiVersion, so you should just ask for the apiVersion :) Since upstream also uses this, they should be fine when we request a PR. But I am not to opinionated about this..
There was a problem hiding this comment.
I will also drop the support for this field in my PR to upstream, I guess. As said, I don't think there is a real value addition for now. Also never saw this way of determining the API version before. 🤷
There was a problem hiding this comment.
Totally agree, should be dropped from upstream, so ok all good here for me. Regarding apiVersion check: Kubernetes installations with same version might still differ in provided api-resources and versions because of the feature gates. I guess that's why Helm provides this check and I think it should be best practice. But I don't want to block if you hesitate.
There was a problem hiding this comment.
Ahh! Yeah, you're right! Gonna use the Has. Thank you so much! :)
There was a problem hiding this comment.
Thank you very much for working on this PR! :)
|
I tested this on AWS 18.1.1 and AWS 17.4.4. So both Kubernetes version, 1.22 and 1.23. |
|
Gonna release app version v2.22.1 after this got merged. |
Tests on workload clusters (not always required)
For changes in the chart, chart templates, and ingress controller container images, I executed the following tests
to verify them working in live enviromnents:
Testing was done using
hello-world-app.Hint for KVM: