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

codegen: support for conditional deployment strategy #549

Merged
merged 3 commits into from
Apr 23, 2024

Conversation

samuelvl
Copy link
Contributor

@samuelvl samuelvl commented Apr 22, 2024

Allow configuring different deployment strategies based on specific Helm conditions:

spec:
  strategy:
{{- if $.Values.painter.condition }}
    type: RollingUpdate
{{- end }}
{{- if not $.Values.painter.condition }}
    type: Recreate
{{- end }}

This is needed by Gloo Platform in order to use the Recreate strategy in the Redis deployment only when persistence is enabled.

@solo-changelog-bot
Copy link

Issues linked to changelog:
https://github.com/solo-io/gloo-mesh-enterprise/issues/15710

Comment on lines +64 to +72
[[- 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 }}
Copy link
Contributor

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) }}

Copy link
Contributor Author

@samuelvl samuelvl Apr 22, 2024

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

Copy link
Contributor

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?

Copy link
Contributor

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

Copy link
Contributor Author

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

@soloio-bulldozer soloio-bulldozer bot merged commit 046b20e into main Apr 23, 2024
5 checks passed
@soloio-bulldozer soloio-bulldozer bot deleted the samuelvl/conditional-strategy branch April 23, 2024 15:22
samuelvl pushed a commit that referenced this pull request Apr 26, 2024
* 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>
samuelvl pushed a commit that referenced this pull request Apr 26, 2024
* 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>
samuelvl pushed a commit that referenced this pull request Apr 29, 2024
* codegen: support for conditional deployment strategy (#549)
samuelvl pushed a commit that referenced this pull request Apr 29, 2024
* codegen: support for conditional deployment strategy (#549)
samuelvl pushed a commit that referenced this pull request Apr 30, 2024
* 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>
samuelvl pushed a commit that referenced this pull request Apr 30, 2024
…gy (#553)

* codegen: allow setting Strategy and PodSecurityContext (#538)

* codegen: support for conditional deployment strategy (#549)

* codegen: fix to prevent null strategy (#550)
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.

None yet

3 participants