-
Notifications
You must be signed in to change notification settings - Fork 2
fix(DMVP-8708): support Keda multiple autoscaling triggers #162
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
base: main
Are you sure you want to change the base?
Conversation
PR Summary 🚀
Keep going, team! This progress is exactly what we need to take our project to the next level! 🌟💪 |
charts/base/templates/keda.yaml
Outdated
| maxReplicaCount: {{ .Values.autoscaling.maxReplicas }} | ||
| triggers: | ||
| - type: {{ .Values.autoscaling.trigger.type }} | ||
| {{- range .Values.autoscaling.trigger }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets support both trigger and triggers, both with single/list ability, with backward compatibility, check config, ingress, multi-containers options we have with similar ways implemented
| - type: memory | ||
| metadata: | ||
| type: Utilization | ||
| value: "75" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- this code chunck seems placed in wrong place and should go into examples/base/with-keda-multiple-triggers.yaml file
- I see triggers.[].metadata.type has been deprecated in favor to triggers.[].metricType, so lets have helm code/templates and examples be aligned to not use deprecated form: https://keda.sh/docs/2.17/scalers/cpu/#trigger-specification
| @@ -1,4 +1,6 @@ | |||
| {{- if and .Values.autoscaling.enabled .Values.autoscaling.trigger }} | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this condition seems prevents the case when we pass variable triggers and not trigger
| @@ -1,4 +1,6 @@ | |||
| {{- if and .Values.autoscaling.enabled .Values.autoscaling.trigger }} | |||
| {{- $triggersRaw := (coalesce .Values.autoscaling.triggers .Values.autoscaling.trigger) }} | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets name this variable just $triggers
| name: {{ include "base.fullname" . }} | ||
| minReplicaCount: {{ .Values.autoscaling.minReplicas }} | ||
| maxReplicaCount: {{ .Values.autoscaling.maxReplicas }} | ||
| triggers: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems here we simply can use the following instead of all the range loop block, lets check if this works and have less code form used:
triggers: {{ toYaml $triggers | nindent 2 }}
No description provided.