feat(charts): Add Helm Chart mode for statefulset deployment without … #5998
Conversation
…persistence and update Readme values with helm-docs Signed-off-by: bennsel <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new mode configuration option to allow users to explicitly choose between deploying Dragonfly as a Deployment or StatefulSet, decoupling this decision from the storage.enabled flag. It also includes version updates and improvements to service configuration handling.
Key changes:
- Added a
modefield to values.yaml with valid options "deployment" or "statefulset" - Updated deployment and statefulset templates to consider the mode field in their conditional rendering logic
- Made service type optional and improved clusterIP handling logic
- Updated chart version from v0.12.0 to v1.34.2
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| contrib/charts/dragonfly/values.yaml | Adds new mode configuration field with default value "deployment" |
| contrib/charts/dragonfly/templates/deployment.yaml | Updates condition to check mode equals "deployment" in addition to storage being disabled |
| contrib/charts/dragonfly/templates/statefulset.yaml | Updates condition to allow statefulset mode regardless of storage setting and makes volumeClaimTemplates conditional |
| contrib/charts/dragonfly/templates/service.yaml | Makes service type optional and adjusts clusterIP logic to handle empty service type |
| contrib/charts/dragonfly/README.md | Updates version badges, adds new configuration parameters to documentation table |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1,4 +1,4 @@ | |||
| {{- if .Values.storage.enabled }} | |||
| {{- if or (.Values.storage.enabled) (eq .Values.mode "statefulset") }} | |||
There was a problem hiding this comment.
The condition allows both deployment and statefulset to render when storage.enabled is true and mode is 'deployment'. This creates conflicting resources. The condition should be: {{- if or (and .Values.storage.enabled (ne .Values.mode \"deployment\")) (eq .Values.mode \"statefulset\") }} to ensure only one resource type is created.
| {{- if or (.Values.storage.enabled) (eq .Values.mode "statefulset") }} | |
| {{- if or (and .Values.storage.enabled (ne .Values.mode "deployment")) (eq .Values.mode "statefulset") }} |
There was a problem hiding this comment.
This change introduces a behavioral difference compared to the previous implementation.
Previously, setting storage.enabled=true automatically switched the deployment type from Deployment to StatefulSet.
With this update, users must explicitly set mode: "statefulset" to deploy Dragonfly as a StatefulSet — even if persistence is enabled.
While this change provides clearer and more predictable configuration behavior, it may be considered breaking, as existing users relying on the implicit switch will need to update their values configuration.
There was a problem hiding this comment.
if storage enabled and mode == "", then create statefulset. If mode is set explicitly, follow mode.
| # -- Number of replicas to deploy | ||
| replicaCount: 1 | ||
|
|
||
| # -- Valid values are "deployment" or "statefulset". |
There was a problem hiding this comment.
The mode field lacks validation. Consider adding validation logic or a comment noting that invalid values will result in no workload being created, which could be confusing for users. Alternatively, implement a validation rule in the templates that produces a clear error message for invalid mode values.
| # -- Valid values are "deployment" or "statefulset". | |
| # -- Valid values are "deployment" or "statefulset". Setting any other value will result in no workload being created. |
There was a problem hiding this comment.
I would prefer to add a values.schema.json; I’ll include it shortly. This should help ensure proper validation and make the configuration more robust.
Co-authored-by: Copilot <[email protected]> Signed-off-by: Benjamin Kluge <[email protected]>
Signed-off-by: bennsel <[email protected]>
Signed-off-by: bennsel <[email protected]>
Abhra303
left a comment
Abhra303
left a comment
There was a problem hiding this comment.
See comments. Overall looks good.
| @@ -1,4 +1,4 @@ | |||
| {{- if not .Values.storage.enabled }} | |||
| {{- if and (not .Values.storage.enabled) (eq .Values.mode "deployment") }} | |||
There was a problem hiding this comment.
Please fix backward compatibility. If .Values.mode is empty string or deployment then proceed with the deployment. So the default behaviour doesn't change (even if the mode is not present).
There was a problem hiding this comment.
I set the mode in the values.yaml to “deployment” as the default. Shouldn’t that be sufficient as a fallback? I’ve actually prevented an empty mode value in the schema YAML.
| @@ -1,4 +1,4 @@ | |||
| {{- if .Values.storage.enabled }} | |||
| {{- if or (.Values.storage.enabled) (eq .Values.mode "statefulset") }} | |||
There was a problem hiding this comment.
if storage enabled and mode == "", then create statefulset. If mode is set explicitly, follow mode.
2 participants
This change introduces a new Helm chart value mode, which allows users to deploy Dragonfly as a StatefulSet even when no persistent storage is defined.
Previously, the chart only created a StatefulSet when persistence (storage.enabled) was configured. With this update, users can explicitly choose the deployment type via:
This provides greater flexibility for testing and ephemeral environments where a StatefulSet is desired but persistence is not required.
helm-docs has automatically updated the README with some new entries, including the new mode value.
The previous implementation remains unchanged.