[INFRA-216] Plane-CE: Add Horizontal Pod Autoscaler (HPA) to Plane-CE Deployments #134
Conversation
…i, worker, and beatworker in questions.yml
…ptions across web, space, admin, live, api, worker, and beatworker sections.
…ing HPA with specific resource utilization targets in values.yaml. Update deployment templates to reference individual autoscaling settings for each component. (#115)
… values.yaml for Plane CE.
…Enable Autoscaling' variable and its associated conditions for various components, streamlining the autoscaling settings for web, space, admin, live, api, worker, and beatworker.
|
""" WalkthroughThis update introduces conditional Horizontal Pod Autoscaler (HPA) resources to the Plane CE Helm chart for multiple components, replacing fixed resource requests with configurable autoscaling parameters. Supporting Helm templates, values, and questions are updated accordingly. Minor formatting and ignore pattern changes are also included, with no impact on workflow logic or public APIs. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Helm Chart
participant Kubernetes API
participant Metrics Server
User->>Helm Chart: Install/Upgrade with autoscaling values
Helm Chart->>Kubernetes API: Render templates
alt Metrics Server present
Helm Chart->>Kubernetes API: Deploy HPA resources for each component
Kubernetes API->>Metrics Server: Query resource metrics
Metrics Server-->>Kubernetes API: Return CPU/Memory usage
Kubernetes API->>Kubernetes API: Scale pods based on HPA thresholds
else Metrics Server absent
Helm Chart->>Kubernetes API: Skip HPA resources
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
Pull Request Linked with Plane Work Items Comment Automatically Generated by Plane |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
charts/plane-ce/templates/workloads/api.deployment.yaml (1)
79-115: Same autoscaling-value guarding required as noted for worker HPAPlease replicate the
default/requiredsafeguards suggested inworker.deployment.yamlto avoid invalid manifests when values are missing.charts/plane-ce/templates/workloads/admin.deployment.yaml (1)
63-97: Mirror the fixes suggested for worker HPAApply the same value-guarding and label-key alignment advice here.
charts/plane-ce/templates/workloads/space.deployment.yaml (1)
62-97: Mirror the fixes suggested for worker HPAApply the same value-guarding and label-key alignment advice here.
🧹 Nitpick comments (7)
charts/plane-ce/values.yaml (3)
84-84: Fix trailing spacesStatic analysis detected trailing spaces on this line.
- targetCPUUtilizationPercentage: 80 + targetCPUUtilizationPercentage: 80
81-85: Consider workload-specific autoscaling parametersAll components use identical autoscaling parameters. Consider whether different workload types (frontend vs backend vs workers) should have different scaling thresholds and replica limits based on their resource usage patterns and criticality.
For example:
- Frontend components (web, space, admin) might scale more aggressively with lower CPU thresholds
- Background workers might need higher memory thresholds due to processing patterns
- Critical components might need higher maxReplicas for better availability
Also applies to: 96-100, 111-115, 126-130, 141-145, 155-159, 169-173
218-218: Remove extra blank lineStatic analysis detected too many blank lines.
- api_key_rate_limit: "60/minute" - + api_key_rate_limit: "60/minute"charts/plane-ce/templates/workloads/worker.deployment.yaml (1)
55-57: Label key diverges from K8s convention
app.nameis not part of the recommendedapp.kubernetes.io/*label set and introduces a custom domain‐less key (dots are supposed to delimit domain and key).
Consider switching toapp.kubernetes.io/nameand aligning across all manifests for tooling interoperability.charts/plane-ce/questions.yml (2)
110-140: YAML lint violations – trailing spaces break CIMultiple lines (e.g. 120, 161, 165…) contain trailing whitespace flagged by YAML-lint.
Removing the trailing spaces keeps the chart lint-clean.- - variable: web.autoscaling.maxReplicas␠ + - variable: web.autoscaling.maxReplicasPlease sweep the file (grep
'[ \t]$') and strip the excess spaces.
100-110: Resource-request vars removed only for some components – inconsistent UX
memoryRequest/cpuRequestwere dropped fromwebbut are still present forspace,api,live, etc.
Either keep them everywhere (they are still useful for HPA as the scaling reference) or remove them everywhere to avoid confusing users.charts/plane-ce/templates/workloads/web.deployment.yaml (1)
70-71: Adopt the Kubernetes-recommended label keys
app.nameis non-standard and impedes label-based tooling. Prefer the official recommended labels:- labels: - app.name: {{ .Release.Namespace }}-{{ .Release.Name }}-web-hpa + labels: + app.kubernetes.io/name: {{ .Release.Name }}-web + app.kubernetes.io/instance: {{ .Release.Name }} + app.kubernetes.io/component: webThis keeps the chart consistent with broader ecosystem practices.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
charts/plane-ce/Chart.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
.github/workflows/chart-preview.yml(0 hunks).github/workflows/chart-releaser.yml(0 hunks).gitignore(1 hunks)charts/plane-ce/questions.yml(7 hunks)charts/plane-ce/templates/_helpers.tpl(1 hunks)charts/plane-ce/templates/workloads/admin.deployment.yaml(1 hunks)charts/plane-ce/templates/workloads/api.deployment.yaml(1 hunks)charts/plane-ce/templates/workloads/beat-worker.deployment.yaml(1 hunks)charts/plane-ce/templates/workloads/live.deployment.yaml(1 hunks)charts/plane-ce/templates/workloads/space.deployment.yaml(1 hunks)charts/plane-ce/templates/workloads/web.deployment.yaml(1 hunks)charts/plane-ce/templates/workloads/worker.deployment.yaml(1 hunks)charts/plane-ce/values.yaml(8 hunks)
💤 Files with no reviewable changes (2)
- .github/workflows/chart-releaser.yml
- .github/workflows/chart-preview.yml
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: mguptahub
PR: makeplane/helm-charts#98
File: charts/plane-enterprise/templates/certs/email-certs.yaml:1-14
Timestamp: 2025-05-22T09:40:55.645Z
Learning: In the makeplane/helm-charts repository, the Certificate resources follow a consistent pattern of only specifying the 'name' field in 'issuerRef' without explicit 'kind' or 'group' fields, using a common cert issuer approach across the application.
.gitignore (1)
Learnt from: mguptahub
PR: #98
File: charts/plane-enterprise/templates/certs/email-certs.yaml:1-14
Timestamp: 2025-05-22T09:40:55.645Z
Learning: In the makeplane/helm-charts repository, the Certificate resources follow a consistent pattern of only specifying the 'name' field in 'issuerRef' without explicit 'kind' or 'group' fields, using a common cert issuer approach across the application.
🪛 YAMLlint (1.37.1)
charts/plane-ce/values.yaml
[error] 84-84: trailing spaces
(trailing-spaces)
[warning] 218-218: too many blank lines (1 > 0)
(empty-lines)
charts/plane-ce/questions.yml
[error] 120-120: trailing spaces
(trailing-spaces)
[error] 161-161: trailing spaces
(trailing-spaces)
[error] 165-165: trailing spaces
(trailing-spaces)
[error] 166-166: trailing spaces
(trailing-spaces)
[error] 171-171: trailing spaces
(trailing-spaces)
[error] 176-176: trailing spaces
(trailing-spaces)
[error] 181-181: trailing spaces
(trailing-spaces)
[error] 234-234: trailing spaces
(trailing-spaces)
[error] 239-239: trailing spaces
(trailing-spaces)
[error] 243-243: trailing spaces
(trailing-spaces)
[error] 244-244: trailing spaces
(trailing-spaces)
[error] 249-249: trailing spaces
(trailing-spaces)
[error] 252-252: trailing spaces
(trailing-spaces)
[error] 254-254: trailing spaces
(trailing-spaces)
[error] 316-316: trailing spaces
(trailing-spaces)
[error] 320-320: trailing spaces
(trailing-spaces)
[error] 321-321: trailing spaces
(trailing-spaces)
[error] 326-326: trailing spaces
(trailing-spaces)
[error] 329-329: trailing spaces
(trailing-spaces)
[error] 331-331: trailing spaces
(trailing-spaces)
[error] 334-334: trailing spaces
(trailing-spaces)
[error] 336-336: trailing spaces
(trailing-spaces)
[error] 355-355: trailing spaces
(trailing-spaces)
[error] 356-356: trailing spaces
(trailing-spaces)
[error] 361-361: trailing spaces
(trailing-spaces)
[error] 365-365: trailing spaces
(trailing-spaces)
[error] 366-366: trailing spaces
(trailing-spaces)
[error] 369-369: trailing spaces
(trailing-spaces)
[error] 371-371: trailing spaces
(trailing-spaces)
[error] 386-386: trailing spaces
(trailing-spaces)
[error] 391-391: trailing spaces
(trailing-spaces)
[error] 395-395: trailing spaces
(trailing-spaces)
[error] 396-396: trailing spaces
(trailing-spaces)
[error] 401-401: trailing spaces
(trailing-spaces)
[error] 404-404: trailing spaces
(trailing-spaces)
[error] 406-406: trailing spaces
(trailing-spaces)
🔇 Additional comments (6)
.gitignore (1)
7-7: LGTM: Standard Helm practiceAdding
charts/plane-ce/chartsto .gitignore follows Helm best practices for excluding chart dependency artifacts from version control.charts/plane-ce/templates/_helpers.tpl (2)
3-3: LGTM: Critical bug fixThe missing
{{- end }}statement would cause template rendering failures. Good catch!
5-12: Helm lookup RBAC context
Thelookupfunction runs at render time using the Helm client’s kubeconfig and requires cluster-level GET/LIST access on ClusterRole resources. Charts cannot bundle those privileges—ensure the user or process performinghelm installhas the necessary RBAC to listClusterRole(e.g. metrics-server’s role). No changes needed to the templates.charts/plane-ce/templates/workloads/beat-worker.deployment.yaml (1)
49-84: HPA consistency verified across all workload templates
All eight*.deployment.yamlfiles undercharts/plane-ce/templates/workloadsinclude akind: HorizontalPodAutoscalerwithapiVersion: autoscaling/v2, identical conditional metrics blocks, and correctscaleTargetRefsettings—no further changes needed.charts/plane-ce/templates/workloads/live.deployment.yaml (1)
70-104: Consistent HPA implementationThe HPA resource follows the same pattern as other workload templates with proper conditional rendering, correct target references, and appropriate metrics configuration.
charts/plane-ce/templates/workloads/web.deployment.yaml (1)
78-96: Guard against an emptymetricsblockThe surrounding
ifsuppressesmetrics:when both targets arenil, but keeps the preceding indent/newline, producing:metrics:with no list items—invalid YAML. Either:
- Move the
metrics:key inside the innerif, or- Add a second guard to remove the key when the list is empty.
Example:
- {{- if or .Values.web.autoscaling.targetCPUUtilizationPercentage .Values.web.autoscaling.targetMemoryUtilizationPercentage }} - metrics: + {{- if or .Values.web.autoscaling.targetCPUUtilizationPercentage .Values.web.autoscaling.targetMemoryUtilizationPercentage }} + metrics: ... {{- end }}Likely an incorrect or invalid review comment.
| {{- if eq (include "enable.hpa" . | trim) "true" }} | ||
| apiVersion: autoscaling/v2 | ||
| kind: HorizontalPodAutoscaler |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make HPA apiVersion cluster-aware
autoscaling/v2 is only available on clusters ≥ v1.23. Rendering this unconditionally will break installs on older clusters that still expose autoscaling/v2beta2.
Dynamically select the API version based on .Capabilities.KubeVersion.Version.
-apiVersion: autoscaling/v2
+{{- $hpaApi := ternary "autoscaling/v2" "autoscaling/v2beta2" (semverCompare ">=1.23-0" .Capabilities.KubeVersion.Version) -}}
+apiVersion: {{ $hpaApi }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {{- if eq (include "enable.hpa" . | trim) "true" }} | |
| apiVersion: autoscaling/v2 | |
| kind: HorizontalPodAutoscaler | |
| {{- if eq (include "enable.hpa" . | trim) "true" }} | |
| {{- $hpaApi := ternary "autoscaling/v2" "autoscaling/v2beta2" (semverCompare ">=1.23-0" .Capabilities.KubeVersion.Version) -}} | |
| apiVersion: {{ $hpaApi }} | |
| kind: HorizontalPodAutoscaler |
🤖 Prompt for AI Agents
In charts/plane-ce/templates/workloads/web.deployment.yaml around lines 63 to
65, the HPA resource uses a fixed apiVersion "autoscaling/v2" which is only
supported on Kubernetes clusters version 1.23 and above. To fix this, modify the
template to dynamically select the apiVersion based on the cluster version
available in .Capabilities.KubeVersion.Version, using "autoscaling/v2beta2" for
older clusters and "autoscaling/v2" for newer ones, ensuring compatibility
across different cluster versions.
…default values for minReplicas, maxReplicas, and averageUtilization for CPU and memory across all workloads.
… worker, and beatworker deployments; adjust autoscaling settings to increase maxReplicas and target utilization percentages across all components.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
charts/plane-ce/templates/workloads/api.deployment.yaml (1)
94-105: Quoted integers in HPA – API deployment will not validateRemove
| quotefromminReplicas,maxReplicas, andaverageUtilizationas shown earlier to keep them numeric.charts/plane-ce/templates/workloads/worker.deployment.yaml (1)
62-73: Quoted integers again – fix before releaseStrip
| quoteonminReplicas,maxReplicas,averageUtilizationexactly as in previous files.
🧹 Nitpick comments (3)
charts/plane-ce/values.yaml (2)
82-85: Trim trailing whitespace to satisfy yamllintLine 84 contains two trailing spaces, currently breaking the CI lint job. Remove them.
- targetCPUUtilizationPercentage: 90·· + targetCPUUtilizationPercentage: 90
217-219: Super-fluous blank lineYAML-Lint flags the extra blank line at EOF. Delete it to keep the file tidy.
charts/plane-ce/questions.yml (1)
120-140: Trailing whitespace all over – breaksyamllintgate
yamllintflags multiple lines (e.g., 125, 177, 194, 239, 264, 305 …) for trailing spaces. Clean these up to keep the chart CI-green.No functional impact, purely formatting.
Also applies to: 170-200, 230-260, 300-320, 410-430
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
charts/plane-ce/Chart.yaml(1 hunks)charts/plane-ce/questions.yml(7 hunks)charts/plane-ce/templates/workloads/admin.deployment.yaml(2 hunks)charts/plane-ce/templates/workloads/api.deployment.yaml(2 hunks)charts/plane-ce/templates/workloads/beat-worker.deployment.yaml(2 hunks)charts/plane-ce/templates/workloads/live.deployment.yaml(2 hunks)charts/plane-ce/templates/workloads/space.deployment.yaml(2 hunks)charts/plane-ce/templates/workloads/web.deployment.yaml(2 hunks)charts/plane-ce/templates/workloads/worker.deployment.yaml(2 hunks)charts/plane-ce/values.yaml(2 hunks)
🧠 Learnings (1)
📓 Common learnings
Learnt from: mguptahub
PR: makeplane/helm-charts#98
File: charts/plane-enterprise/templates/certs/email-certs.yaml:1-14
Timestamp: 2025-05-22T09:40:55.645Z
Learning: In the makeplane/helm-charts repository, the Certificate resources follow a consistent pattern of only specifying the 'name' field in 'issuerRef' without explicit 'kind' or 'group' fields, using a common cert issuer approach across the application.
🪛 YAMLlint (1.37.1)
charts/plane-ce/questions.yml
[error] 125-125: trailing spaces
(trailing-spaces)
[error] 177-177: trailing spaces
(trailing-spaces)
[error] 194-194: trailing spaces
(trailing-spaces)
[error] 195-195: trailing spaces
(trailing-spaces)
[error] 239-239: trailing spaces
(trailing-spaces)
[error] 246-246: trailing spaces
(trailing-spaces)
[error] 264-264: trailing spaces
(trailing-spaces)
[error] 265-265: trailing spaces
(trailing-spaces)
[error] 305-305: trailing spaces
(trailing-spaces)
[error] 312-312: trailing spaces
(trailing-spaces)
[error] 316-316: trailing spaces
(trailing-spaces)
[error] 343-343: trailing spaces
(trailing-spaces)
[error] 344-344: trailing spaces
(trailing-spaces)
[error] 370-370: trailing spaces
(trailing-spaces)
[error] 378-378: trailing spaces
(trailing-spaces)
[error] 381-381: trailing spaces
(trailing-spaces)
[error] 383-383: trailing spaces
(trailing-spaces)
[error] 413-413: trailing spaces
(trailing-spaces)
[error] 420-420: trailing spaces
(trailing-spaces)
[error] 422-422: trailing spaces
(trailing-spaces)
charts/plane-ce/values.yaml
[error] 84-84: trailing spaces
(trailing-spaces)
[warning] 218-218: too many blank lines (1 > 0)
(empty-lines)
✅ Files skipped from review due to trivial changes (1)
- charts/plane-ce/Chart.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- charts/plane-ce/templates/workloads/space.deployment.yaml
- charts/plane-ce/templates/workloads/web.deployment.yaml
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: mguptahub
PR: makeplane/helm-charts#98
File: charts/plane-enterprise/templates/certs/email-certs.yaml:1-14
Timestamp: 2025-05-22T09:40:55.645Z
Learning: In the makeplane/helm-charts repository, the Certificate resources follow a consistent pattern of only specifying the 'name' field in 'issuerRef' without explicit 'kind' or 'group' fields, using a common cert issuer approach across the application.
🪛 YAMLlint (1.37.1)
charts/plane-ce/questions.yml
[error] 125-125: trailing spaces
(trailing-spaces)
[error] 177-177: trailing spaces
(trailing-spaces)
[error] 194-194: trailing spaces
(trailing-spaces)
[error] 195-195: trailing spaces
(trailing-spaces)
[error] 239-239: trailing spaces
(trailing-spaces)
[error] 246-246: trailing spaces
(trailing-spaces)
[error] 264-264: trailing spaces
(trailing-spaces)
[error] 265-265: trailing spaces
(trailing-spaces)
[error] 305-305: trailing spaces
(trailing-spaces)
[error] 312-312: trailing spaces
(trailing-spaces)
[error] 316-316: trailing spaces
(trailing-spaces)
[error] 343-343: trailing spaces
(trailing-spaces)
[error] 344-344: trailing spaces
(trailing-spaces)
[error] 370-370: trailing spaces
(trailing-spaces)
[error] 378-378: trailing spaces
(trailing-spaces)
[error] 381-381: trailing spaces
(trailing-spaces)
[error] 383-383: trailing spaces
(trailing-spaces)
[error] 413-413: trailing spaces
(trailing-spaces)
[error] 420-420: trailing spaces
(trailing-spaces)
[error] 422-422: trailing spaces
(trailing-spaces)
charts/plane-ce/values.yaml
[error] 84-84: trailing spaces
(trailing-spaces)
[warning] 218-218: too many blank lines (1 > 0)
(empty-lines)
🔇 Additional comments (2)
charts/plane-ce/templates/workloads/api.deployment.yaml (1)
49-53: CPU request > cluster quota?Jumping directly from
50mto1000mmay starve smaller clusters. Double-check this is intentional; otherwise consider a more conservative default (e.g.300m) and let HPA handle bursts.charts/plane-ce/templates/workloads/admin.deployment.yaml (1)
48-53: Resource increase looks reasonable
Bumping the admin pod’s default request from 50 m/50 Mi to 100 m/200 Mi (and raising the limit) makes sense given the recent load spikes we have observed. No functional concerns here.
charts/plane-ce/templates/workloads/beat-worker.deployment.yaml
Outdated
Show resolved
Hide resolved
…ryRequest from 10000Mi to 1000Mi in values.yaml and deployment template. Adjust whitespace and formatting in questions.yml and README.md for consistency.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
charts/plane-ce/README.md (1)
150-153: Streamline phrasing of resource-request descriptionsThe repeated sentence “Every deployment in kubernetes can be set to use minimum cpu they are allowed to use” is hard to parse and appears in every table. A concise wording improves readability without losing meaning.
- | web.cpuRequest | 100m | | Every deployment in kubernetes can be set to use minimum cpu they are allowed to use. This key sets the cpu request for this deployment to use.| + | web.cpuRequest | 100m | | Minimum CPU requested for this deployment. | - | web.memoryRequest | 200Mi | | Every deployment in kubernetes can be set to use minimum memory they are allowed to use. This key sets the memory request for this deployment to use.| + | web.memoryRequest | 200Mi | | Minimum memory requested for this deployment. |Please replicate the cleaner wording for the
cpuRequest/memoryRequestrows in the other deployment sections to keep the documentation consistent.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
charts/plane-ce/README.md(6 hunks)charts/plane-ce/questions.yml(7 hunks)charts/plane-ce/templates/workloads/api.deployment.yaml(2 hunks)charts/plane-ce/values.yaml(3 hunks)
🧠 Learnings (2)
📓 Common learnings
Learnt from: mguptahub
PR: makeplane/helm-charts#98
File: charts/plane-enterprise/templates/certs/email-certs.yaml:1-14
Timestamp: 2025-05-22T09:40:55.645Z
Learning: In the makeplane/helm-charts repository, the Certificate resources follow a consistent pattern of only specifying the 'name' field in 'issuerRef' without explicit 'kind' or 'group' fields, using a common cert issuer approach across the application.
charts/plane-ce/README.md (1)
Learnt from: mguptahub
PR: #98
File: charts/plane-enterprise/templates/certs/email-certs.yaml:1-14
Timestamp: 2025-05-22T09:40:55.645Z
Learning: In the makeplane/helm-charts repository, the Certificate resources follow a consistent pattern of only specifying the 'name' field in 'issuerRef' without explicit 'kind' or 'group' fields, using a common cert issuer approach across the application.
🚧 Files skipped from review as they are similar to previous changes (3)
- charts/plane-ce/values.yaml
- charts/plane-ce/templates/workloads/api.deployment.yaml
- charts/plane-ce/questions.yml
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: mguptahub
PR: makeplane/helm-charts#98
File: charts/plane-enterprise/templates/certs/email-certs.yaml:1-14
Timestamp: 2025-05-22T09:40:55.645Z
Learning: In the makeplane/helm-charts repository, the Certificate resources follow a consistent pattern of only specifying the 'name' field in 'issuerRef' without explicit 'kind' or 'group' fields, using a common cert issuer approach across the application.
charts/plane-ce/README.md (1)
Learnt from: mguptahub
PR: #98
File: charts/plane-enterprise/templates/certs/email-certs.yaml:1-14
Timestamp: 2025-05-22T09:40:55.645Z
Learning: In the makeplane/helm-charts repository, the Certificate resources follow a consistent pattern of only specifying the 'name' field in 'issuerRef' without explicit 'kind' or 'group' fields, using a common cert issuer approach across the application.
🔇 Additional comments (2)
charts/plane-ce/README.md (2)
154-157: Double-check memory-based HPA fields
targetMemoryUtilizationPercentageis not supported in theautoscaling/v1API (CPU only).
If the chart now emits anautoscaling/v2orautoscaling/v2beta2manifest with a propermetricsblock, great—otherwise these fields will be silently ignored.Confirm that:
- The rendered HPA YAML uses
apiVersion: autoscaling/v2*.- A
metricsarray with theresource: { name: "memory", target: … }entry is present.If either item is missing the doc should drop the memory line or the template should be fixed.
167-174: Verify README defaults match values.yamlThe defaults for CPU/memory requests, limits, and autoscaling thresholds in the README must exactly reflect
charts/plane-ce/values.yamlto prevent users from copying stale values. Please confirm the following README sections align with the values file:• Lines 167–174
• Lines 184–191
• Lines 201–208
• Lines 221–228
• Lines 241–248
• Lines 256–263You can run:
rg -U -A3 '^(web|space|admin|live|api|worker|beatworker):' charts/plane-ce/values.yaml \ | grep -E 'cpuRequest|memoryRequest|cpuLimit|memoryLimit|minReplicas|maxReplicas|targetCPUUtilizationPercentage|targetMemoryUtilizationPercentage'If any value differs from the README, update either the README or
values.yaml.
…ng unnecessary quotes around default values for minReplicas, maxReplicas, and averageUtilization for CPU and memory across all workloads.
…onal logic for HPA support. Adjust resource requests and limits for admin, api, beat-worker, live, space, web, and worker deployments based on HPA configuration, ensuring optimized performance under varying load conditions.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
charts/plane-ce/templates/workloads/beat-worker.deployment.yaml (1)
7-8: Avoid settingspec.replicaswhen an HPA is attachedWith an HPA managing the Deployment, Helm continuously reconciling
replicas:to1
will fight the autoscaler. Make this field conditional:- replicas: {{ .Values.beatworker.replicas | default 1 }} + {{- if ne (include "enable.hpa" . | trim) "true" }} + replicas: {{ .Values.beatworker.replicas | default 1 }} + {{- end }}charts/plane-ce/templates/workloads/worker.deployment.yaml (1)
7-8: Drop hard-coded replica count when HPA controls the DeploymentAs with beat-worker, wrap
replicas:in a negated HPA check to avoid controller
conflicts.- replicas: {{ .Values.worker.replicas | default 1 }} + {{- if ne (include "enable.hpa" . | trim) "true" }} + replicas: {{ .Values.worker.replicas | default 1 }} + {{- end }}
🧹 Nitpick comments (2)
charts/plane-ce/templates/workloads/beat-worker.deployment.yaml (1)
72-73: CastminReplicas/maxReplicasto integersIf users override the values with strings (
"3"), the HPA will be rejected.
Force-cast:- minReplicas: {{ .Values.beatworker.autoscaling.minReplicas | default 1 }} - maxReplicas: {{ .Values.beatworker.autoscaling.maxReplicas | default 5 }} + minReplicas: {{ (.Values.beatworker.autoscaling.minReplicas | default 1) | int }} + maxReplicas: {{ (.Values.beatworker.autoscaling.maxReplicas | default 5) | int }}charts/plane-ce/templates/workloads/worker.deployment.yaml (1)
71-72: Force integer type forminReplicas/maxReplicas- minReplicas: {{ .Values.worker.autoscaling.minReplicas | default 1 }} - maxReplicas: {{ .Values.worker.autoscaling.maxReplicas | default 5 }} + minReplicas: {{ (.Values.worker.autoscaling.minReplicas | default 1) | int }} + maxReplicas: {{ (.Values.worker.autoscaling.maxReplicas | default 5) | int }}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
charts/plane-ce/templates/workloads/admin.deployment.yaml(2 hunks)charts/plane-ce/templates/workloads/api.deployment.yaml(2 hunks)charts/plane-ce/templates/workloads/beat-worker.deployment.yaml(2 hunks)charts/plane-ce/templates/workloads/live.deployment.yaml(2 hunks)charts/plane-ce/templates/workloads/space.deployment.yaml(2 hunks)charts/plane-ce/templates/workloads/web.deployment.yaml(2 hunks)charts/plane-ce/templates/workloads/worker.deployment.yaml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- charts/plane-ce/templates/workloads/admin.deployment.yaml
- charts/plane-ce/templates/workloads/space.deployment.yaml
- charts/plane-ce/templates/workloads/web.deployment.yaml
- charts/plane-ce/templates/workloads/live.deployment.yaml
- charts/plane-ce/templates/workloads/api.deployment.yaml
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: mguptahub
PR: makeplane/helm-charts#98
File: charts/plane-enterprise/templates/certs/email-certs.yaml:1-14
Timestamp: 2025-05-22T09:40:55.645Z
Learning: In the makeplane/helm-charts repository, the Certificate resources follow a consistent pattern of only specifying the 'name' field in 'issuerRef' without explicit 'kind' or 'group' fields, using a common cert issuer approach across the application.
🪛 YAMLlint (1.37.1)
charts/plane-ce/templates/workloads/beat-worker.deployment.yaml
[warning] 27-27: wrong indentation: expected 8 but found 10
(indentation)
charts/plane-ce/templates/workloads/worker.deployment.yaml
[warning] 27-27: wrong indentation: expected 8 but found 10
(indentation)
| {{- if eq (include "enable.hpa" . | trim) "true" }} | ||
| requests: | ||
| cpu: {{ .Values.worker.cpuRequest | default "500m" | quote }} | ||
| memory: {{ .Values.worker.memoryRequest | default "2000Mi" | quote }} | ||
| limits: | ||
| cpu: {{ .Values.worker.cpuLimit | default "1000m" | quote}} | ||
| memory: {{ .Values.worker.memoryLimit | default "4000Mi" | quote }} | ||
| {{- else }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Indentation under resources: is off – will produce invalid YAML
Same issue as the beat-worker template: the if/else directives eat the leading
spaces, so requests: / limits: end up two spaces too deep.
- resources:
- {{- if eq (include "enable.hpa" . | trim) "true" }}
- requests:
- cpu: {{ .Values.worker.cpuRequest | default "500m" | quote }}
- memory: {{ .Values.worker.memoryRequest | default "2000Mi" | quote }}
- limits:
- cpu: {{ .Values.worker.cpuLimit | default "1000m" | quote }}
- memory: {{ .Values.worker.memoryLimit | default "4000Mi" | quote }}
- {{- else }}
- requests:
- cpu: {{ .Values.worker.cpuRequest | default "50m" | quote }}
- memory: {{ .Values.worker.memoryRequest | default "50Mi" | quote }}
- limits:
- cpu: {{ .Values.worker.cpuLimit | default "500m" | quote }}
- memory: {{ .Values.worker.memoryLimit | default "1000Mi" | quote }}
- {{- end }}
+ resources:
+{{- if eq (include "enable.hpa" . | trim) "true" }}
+{{- nindent 10 "requests:\n cpu: {{ .Values.worker.cpuRequest | default \"500m\" | quote }}\n memory: {{ .Values.worker.memoryRequest | default \"2000Mi\" | quote }}\nlimits:\n cpu: {{ .Values.worker.cpuLimit | default \"1000m\" | quote }}\n memory: {{ .Values.worker.memoryLimit | default \"4000Mi\" | quote }}" }}
+{{- else }}
+{{- nindent 10 "requests:\n cpu: {{ .Values.worker.cpuRequest | default \"50m\" | quote }}\n memory: {{ .Values.worker.memoryRequest | default \"50Mi\" | quote }}\nlimits:\n cpu: {{ .Values.worker.cpuLimit | default \"500m\" | quote }}\n memory: {{ .Values.worker.memoryLimit | default \"1000Mi\" | quote }}" }}
+{{- end }}Fixing this prevents yamllint errors and deployment failures.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 YAMLlint (1.37.1)
[warning] 27-27: wrong indentation: expected 8 but found 10
(indentation)
🤖 Prompt for AI Agents
In charts/plane-ce/templates/workloads/worker.deployment.yaml around lines 26 to
33, the indentation under the resources: section is incorrect due to the if/else
directives removing leading spaces, causing requests: and limits: to be indented
too deeply and produce invalid YAML. Adjust the template so that the if/else
blocks preserve the correct indentation level for requests: and limits:,
ensuring they align properly under resources: to prevent yamllint errors and
deployment failures.
Description
Summary by CodeRabbit