-
Notifications
You must be signed in to change notification settings - Fork 8
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
codegen: support for conditional deployment strategy #549
Conversation
Issues linked to changelog: |
[[- if $strategy ]] | ||
strategy: | ||
[[- if $deploymentStrategy.Type ]] | ||
type: [[ $deploymentStrategy.Type ]] | ||
[[- end ]] | ||
[[- if $deploymentStrategy.RollingUpdate ]] | ||
rollingUpdate: | ||
[[ toYaml $deploymentStrategy.RollingUpdate | indent 6 ]] | ||
[[ toYaml $strategy | indent 4 ]] | ||
[[- else if $conditionalStrategy ]] | ||
strategy: | ||
[[- range $s := $conditionalStrategy ]] | ||
{{- if [[ $s.Condition ]] }} | ||
[[ toYaml $s.Strategy | indent 4 ]] | ||
{{- end }} |
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 seems like a bit much right? We just want to toggle the value of strategy here right? Couldn't we achieve that will something like this:
strategy: {{ <user-value> (e.g: Recreate) | default <value> (e.g: RollingUpdate) }}
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.
We want the strategy value to be automatically set based on a helm condition (if persistence is enabled or not), not from user input. Something like this:
spec:
strategy:
{{- if $.Values.redis.deployment.persistence.enabled }}
type: RollingUpdate
{{- end }}
{{- if not $.Values.redis.deployment.persistence.enabled }}
type: Recreate
{{- end }}
The deployment strategy is an object, not a plain string https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.26/#deploymentstrategy-v1-apps That's why we need the toYaml
function
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 just seems a bit weird having both in the template. Could we not include the default case since kubernetes would handle that?
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.
Actually don't want to block you on this since its pretty small. But I'm good with the changes here. I'll give @EItanya the rest of today to approve otherwise I'll do it
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 just seems a bit weird having both in the template. Could we not include the default case since kubernetes would handle that?
Good idea, I will do that in the Gloo Mesh PR
* codegen: support for conditional deployment strategy * Adding changelog file to new location * Deleting changelog file from old location --------- Co-authored-by: changelog-bot <changelog-bot>
* codegen: support for conditional deployment strategy * Adding changelog file to new location * Deleting changelog file from old location --------- Co-authored-by: changelog-bot <changelog-bot>
* codegen: support for conditional deployment strategy (#549)
* codegen: support for conditional deployment strategy (#549)
* codegen: support for conditional deployment strategy * Adding changelog file to new location * Deleting changelog file from old location --------- Co-authored-by: changelog-bot <changelog-bot>
Allow configuring different deployment strategies based on specific Helm conditions:
This is needed by Gloo Platform in order to use the
Recreate
strategy in the Redis deployment only when persistence is enabled.