feat: allow specifying deployment in helm chart without autoscaling#41397
feat: allow specifying deployment in helm chart without autoscaling#41397wyattwalter merged 4 commits intoreleasefrom
Conversation
wyattwalter
requested review from
nidhi-nair,
pratapaprasanna and
sharat87
as code owners
WalkthroughThe PR bumps the Helm chart version, adds a workload.kind parameter (Deployment | StatefulSet), updates templates to choose workload kind and persistence behavior based on workload.kind and autoscaling, adds related values, and introduces Helm tests and README docs for the new option. Changes
Sequence Diagram(s)sequenceDiagram
participant User as Values.yaml / Overrides
participant Helm as Helm templates
participant K8s as Rendered Manifests
Note over User,Helm: Determine workload.kind and autoscaling.enabled
User->>Helm: provide .Values.workload.kind, .Values.autoscaling.enabled, .Values.persistence.*
Helm->>Helm: set $workloadKind = upper(.Values.workload.kind)
Helm->>Helm: set $useDeployment = ($workloadKind == "DEPLOYMENT") or .Values.autoscaling.enabled
alt $useDeployment true
Helm->>K8s: render Deployment (use .Values.replicas if autoscaling disabled)
opt persistence enabled & no existingClaim
Helm->>K8s: render PV and PVC
end
else $useDeployment false
Helm->>K8s: render StatefulSet (replicas fixed / serviceName present)
alt persistence enabled & no existingClaim
Helm->>K8s: render PV and PVC only when not using Deployment (prior behavior)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
deploy/helm/tests/persistence_test.yaml (1)
46-52: Test covers the StatefulSet case, but missing explicit Deployment scenario.This test validates that standalone PV/PVC are not created when using StatefulSet (default). However, there's no test explicitly verifying that PV/PVC ARE created when
workload.kind: Deploymentwithautoscaling.enabled: falseandpersistence.enabled: true. Consider adding a test case for this critical use case (the main feature of the PR).Add this test case to persistence_test.yaml:
- name: PV and PVC should be created when workload is Deployment, persistence enabled, and autoscaling disabled set: workload: kind: Deployment persistence: enabled: true existingClaim: enabled: false autoscaling: enabled: false asserts: - isKind: of: PersistentVolumeClaim template: persistentVolumeClaim.yaml - isKind: of: PersistentVolume template: persistentVolume.yaml
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
deploy/helm/tests/__snapshot__/defaults_snapshot_test.yaml.snapis excluded by!**/*.snap
📒 Files selected for processing (8)
deploy/helm/Chart.yaml(1 hunks)deploy/helm/README.md(1 hunks)deploy/helm/templates/deployment.yaml(2 hunks)deploy/helm/templates/persistentVolume.yaml(1 hunks)deploy/helm/templates/persistentVolumeClaim.yaml(1 hunks)deploy/helm/tests/persistence_test.yaml(1 hunks)deploy/helm/tests/workload_type_test.yaml(1 hunks)deploy/helm/values.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Aishwarya-U-R
Repo: appsmithorg/appsmith PR: 29405
File: app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2_Widget_API_Pagination_spec.js:37-41
Timestamp: 2024-10-08T15:32:39.374Z
Learning: The pull request titled "test: Cypress | Replace static with Dynamic waits - Part 1" is part of a phased approach where only certain test specifications are targeted for static wait removal in the initial phase. Future phases will address additional specs.
Learnt from: Aishwarya-U-R
Repo: appsmithorg/appsmith PR: 29405
File: app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2_Widget_API_Pagination_spec.js:37-41
Timestamp: 2024-07-26T21:12:57.228Z
Learning: The pull request titled "test: Cypress | Replace static with Dynamic waits - Part 1" is part of a phased approach where only certain test specifications are targeted for static wait removal in the initial phase. Future phases will address additional specs.
🪛 YAMLlint (1.37.1)
deploy/helm/templates/persistentVolumeClaim.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
deploy/helm/templates/persistentVolume.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🔇 Additional comments (8)
deploy/helm/Chart.yaml (1)
14-14: Version bump looks good.Patch version increment is appropriate for this feature addition.
deploy/helm/templates/persistentVolumeClaim.yaml (1)
1-3: Template logic correctly gates PVC creation.The new conditional variables and gating logic align well with the workload-type selection strategy. PVC creation is now correctly scoped to Deployment workloads only, while StatefulSet uses volumeClaimTemplates.
deploy/helm/values.yaml (1)
274-290: Configuration parameters are well-designed and documented.Default values maintain backward compatibility (StatefulSet), and constraints are clearly documented. The replicas parameter is correctly scoped to Deployment-without-autoscaling scenarios.
deploy/helm/README.md (1)
99-104: Documentation is clear and well-positioned.Accurately describes the workload.kind parameter, its interaction with autoscaling, and replicas usage. Good placement in the deployment parameters section.
deploy/helm/templates/persistentVolume.yaml (1)
1-3: Consistent with PVC logic.Template variables and conditions mirror the PVC file, ensuring PV and PVC resources are created and destroyed in tandem.
deploy/helm/tests/workload_type_test.yaml (1)
1-33: Test coverage is comprehensive and well-structured.All critical scenarios are covered: default StatefulSet, explicit Deployment, replicas usage, and autoscaling override. Assertions are precise.
deploy/helm/templates/deployment.yaml (2)
15-23: Deployment replica logic is correct.Replicas are correctly omitted when autoscaling is enabled (allowing HPA to control) and set from
.Values.replicasotherwise. This enables the feature's core use case: fixed replica count without HPA.
189-216: Volume provisioning logic is sound.Correctly uses volumeClaimTemplates for StatefulSet and explicit PVC for Deployment. Gating by
$useDeploymentensures the right approach is used for each workload type.
2 participants
Description
Allows specifying that the deployment of Appsmith should be done via the Deployment controller without enabling autoscaling. This is useful in situations where you want to be able to set a number of replicas without using HPAs or if you want the flexibility to set replica count to 0 to temporarily pause the instance.
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags=""
🔍 Cypress test results
Warning
Tests have not run on the HEAD 35fba39 yet
Wed, 19 Nov 2025 03:42:56 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Documentation
Tests
Chores