Conversation
|
alex.goldenberg seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
Contributor
📝 WalkthroughWalkthroughThe Helm chart adds optional KEDA ChangesHelm operational resources
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
🔒 Container Vulnerability Scan (hub-migrations - amd64)Click to expand results |
Contributor
🔒 Container Vulnerability Scan (hub-migrations - arm64)Click to expand results |
Contributor
🔒 Container Vulnerability Scan (hub - amd64)Click to expand results |
Contributor
🔒 Container Vulnerability Scan (hub - arm64)Click to expand results |
doronkopit5
reviewed
Jul 14, 2026
doronkopit5
left a comment
doronkopit5
left a comment
Member
There was a problem hiding this comment.
didnt you say you have some issue with this
Author
yes, the issue was in CI/CD "Build Images and Deploy" did not applied changes to staging, so I did it manually. UPD: UPD2. Here you can see that it really included scaled object |
doronkopit5
reviewed
Jul 15, 2026
doronkopit5
left a comment
doronkopit5
left a comment
Member
There was a problem hiding this comment.
dont add prod and stg overlays here, i'll explain in private
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@helm/templates/pdb.yaml`:
- Around line 10-13: Update the PDB template’s maxUnavailable/minAvailable
selection to use explicit key-presence or null checks rather than truthiness and
the default function, so configured zero values are preserved and only an absent
value falls back to minAvailable’s intended default.
In `@helm/values-staging.yaml`:
- Around line 5-14: Replace the CPU-only KEDA triggers with demand-based scaling
by adding an appropriate request, queue, or concurrency metric in
helm/values-staging.yaml lines 5-14 and helm/values-production.yaml lines 9-18;
retain CPU only if it remains needed, and ensure both environments use the
corresponding demand metric configuration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a4d89f01-a3f3-4516-a92f-7ca2c43e56d1
📒 Files selected for processing (5)
helm/templates/pdb.yamlhelm/templates/scaling-object.yamlhelm/values-production.yamlhelm/values-staging.yamlhelm/values.yaml
Comment on lines
+10
to
+13
| {{- if .Values.pdb.maxUnavailable }} | ||
| maxUnavailable: {{ .Values.pdb.maxUnavailable }} | ||
| {{- else }} | ||
| minAvailable: {{ .Values.pdb.minAvailable | default 1 }} |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve explicit zero values in PDB configuration.
Helm treats numeric 0 as false/empty here, so maxUnavailable: 0 falls through to minAvailable, while minAvailable: 0 is replaced by the default 1. Both are valid Kubernetes settings and have different disruption semantics. Use key-presence/null checks instead of truthiness and default for these fields. (kubernetes.io)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@helm/templates/pdb.yaml` around lines 10 - 13, Update the PDB template’s
maxUnavailable/minAvailable selection to use explicit key-presence or null
checks rather than truthiness and the default function, so configured zero
values are preserved and only an absent value falls back to minAvailable’s
intended default.
Comment on lines
+5
to
+14
| # LLM hub proxy — CPU under-reads long inference calls | ||
| minReplicaCount: 1 | ||
| maxReplicaCount: 4 | ||
| pollingInterval: 30 | ||
| cooldownPeriod: 300 | ||
| triggers: | ||
| - type: cpu | ||
| metricType: Utilization | ||
| metadata: | ||
| value: "70" |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== files ==\n'
git ls-files helm | sed -n '1,120p'
printf '\n== ast outline values-staging.yaml ==\n'
ast-grep outline helm/values-staging.yaml --view expanded || true
printf '\n== ast outline values-production.yaml ==\n'
ast-grep outline helm/values-production.yaml --view expanded || true
printf '\n== relevant autoscaling references ==\n'
rg -n "minReplicaCount|maxReplicaCount|pollingInterval|cooldownPeriod|triggers|type: cpu|metricType: Utilization|ScaledObject|autoscal" helm -SRepository: traceloop/hub
Length of output: 2045
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== helm/values.yaml relevant section ==\n'
sed -n '60,110p' helm/values.yaml | cat -n
printf '\n== helm/templates/scaling-object.yaml ==\n'
sed -n '1,80p' helm/templates/scaling-object.yaml | cat -n
printf '\n== staging values ==\n'
sed -n '1,40p' helm/values-staging.yaml | cat -n
printf '\n== production values ==\n'
sed -n '1,50p' helm/values-production.yaml | cat -nRepository: traceloop/hub
Length of output: 3377
Use a demand-based KEDA trigger instead of CPU-only scaling.
Both helm/values-staging.yaml and helm/values-production.yaml enable KEDA with only a CPU utilization trigger, but the config itself notes that CPU under-reads long inference calls. Add a request/queue/concurrency metric, or document why CPU alone is sufficient.
📍 Affects 2 files
helm/values-staging.yaml#L5-L14(this comment)helm/values-production.yaml#L9-L18
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@helm/values-staging.yaml` around lines 5 - 14, Replace the CPU-only KEDA
triggers with demand-based scaling by adding an appropriate request, queue, or
concurrency metric in helm/values-staging.yaml lines 5-14 and
helm/values-production.yaml lines 9-18; retain CPU only if it remains needed,
and ensure both environments use the corresponding demand metric configuration.
Author
now Octopus applying scaled objects. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ScaledObjectandPodDisruptionBudgettemplates to enable workload autoscalingTest plan
helm template/helm lintrenders cleanly with the new scaling objectsRef: TLP-2389
Summary by CodeRabbit