chart: support configuring worker controller type #325
Conversation
|
|
||
| controller: | ||
| type: statefulset | ||
| type: '{{ .Values.worker.config.type | default "statefulset" }}' |
There was a problem hiding this comment.
Any chance you could add this new worker.config.type parameter and its possible values to the chart's README.md (https://github.com/pabloromeo/clusterplex/blob/master/charts/clusterplex/README.md)?
There was a problem hiding this comment.
Done, however I used the helm-docs tool which generated a lot more changes than I was expecting. I could put my change in manually if you want.
There was a problem hiding this comment.
Ah, yeah, in fact you did the right thing. My instructions were incorrect.
Let me take a look tomorrow to see if the contents still looks correct and we'll merge it in.
craigcabrey
force-pushed
the
craigcabrey/feat/worker-controller-type
branch
from
0cf3ba9 to
0456a89
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for configuring the worker controller type in the ClusterPlex Helm chart, allowing users to choose between StatefulSet (default) and Deployment controllers for worker pods.
Key changes:
- Adds
worker.config.typeconfiguration parameter with default value "statefulset" - Updates worker template to use the configurable controller type
- Modifies codec volume handling to disable when using deployment controller
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| charts/clusterplex/values.yaml | Adds new type configuration parameter under worker config with documentation |
| charts/clusterplex/templates/worker.yaml | Updates controller type to use template variable and modifies codec volume configuration |
| charts/clusterplex/README.md | Updates documentation table with new configuration parameter and version bumps |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| persistence: | ||
| codecs: | ||
| {{- toYaml .Values.worker.codecVolumes | nindent 4 }} | ||
| enabled: {{ and .Values.worker.codecVolumes.enabled (eq .Values.worker.config.type "deployment") }} |
There was a problem hiding this comment.
The logic for enabling codec volumes is inverted. The condition should enable codec volumes for StatefulSet (not Deployment) since the comment in values.yaml indicates that codec volumes may require additional configuration when using deployment. This should be (eq .Values.worker.config.type \"statefulset\").
| enabled: {{ and .Values.worker.codecVolumes.enabled (eq .Values.worker.config.type "deployment") }} | |
| enabled: {{ and .Values.worker.codecVolumes.enabled (eq .Values.worker.config.type "statefulset") }} |
2 participants
This is a minimal backwards compatible change to support changing the controller type for workers. It is currently hardcoded as a statefulset, but I wanted to use a deployment instead.