chore: update template & general improvement#633
Conversation
📝 WalkthroughWalkthroughThis PR overhauls the openrag-stack Helm chart: version bump and dependency conditions in Chart.yaml, shared labeling/secret-naming helper templates, provider-based secrets rendering (values/externalSecret/vaultStaticSecret), refactored per-component manifests (admin-ui, openrag, reranker, RayCluster) using value-driven configuration, fullname-scoped naming, updated PVC/migration job wiring, ingress consolidation, new NOTES.txt and extra-objects templates, extensive values.yaml/values-linagora.yaml changes, doc updates, and UI Dockerfile ownership fixes. ChangesOpenRAG Helm Chart Overhaul
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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
added
the
chore
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
infra/charts/openrag-stack/values-linagora.yaml (1)
1-5: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winOverlay
storageClass: longhornwon't reach postgresql/milvus sub-charts.See companion comment on
values.yaml(persistence.storageClass anchor) — becausepostgresql.primary.persistence.storageClassandmilvus.minio/milvus.etcdstorageClass are set via a YAML alias to the base file'spersistence.storageClass, this overlay's override only updates the top-levelpersistence.storageClasskey and does not propagate to those sub-chart paths. Explicitly setpostgresql.primary.persistence.storageClass: longhorn,milvus.minio.persistence.storageClass: longhorn, andmilvus.etcd.persistence.storageClass: longhornhere as well.🤖 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 `@infra/charts/openrag-stack/values-linagora.yaml` around lines 1 - 5, The overlay only updates the top-level persistence.storageClass, so the PostgreSQL and Milvus sub-charts still keep the base-file alias values. Update this values overlay to explicitly set postgresql.primary.persistence.storageClass, milvus.minio.persistence.storageClass, and milvus.etcd.persistence.storageClass to longhorn alongside the existing persistence settings so the override applies everywhere.
🧹 Nitpick comments (1)
infra/charts/openrag-stack/values.yaml (1)
348-364: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
adminUi.ingressis missing the fields its own doc references.The comment above instructs setting the same
hoston both this Ingress block andopenrag.ingressfor single-vhost routing, butadminUi.ingressonly declaresenabled: true— noclassName,host,annotations, ortlsstubs likeopenrag.ingresshas (lines 411-423). If the admin-ui template reads these keys, they'll silently resolve to empty/nil; adding stub keys here would make the documented configuration surface discoverable.🤖 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 `@infra/charts/openrag-stack/values.yaml` around lines 348 - 364, The adminUi.ingress values block is missing the configuration keys referenced by its own routing note, so add stub fields for the admin UI ingress shape to match openrag.ingress. Update the ingress section in values.yaml to include the same discoverable keys (such as className, host, annotations, and tls) alongside enabled, so templates like admin-ui ingress rendering can resolve them consistently and the documented single-vhost setup is exposed in the chart values.
🤖 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 `@infra/charts/openrag-stack/templates/admin-ui.yaml`:
- Around line 25-27: The admin UI service account template is using a `with`
block that treats explicit `false` as empty, so `automountServiceAccountToken`
gets omitted from the manifest. Update the `adminUi` template logic to check for
nil/defined presence instead of truthiness so `false` is rendered intentionally,
and keep the field emission tied to the `automountServiceAccountToken` value in
`admin-ui.yaml`.
In `@infra/charts/openrag-stack/templates/infinity.yaml`:
- Around line 25-27: The `with` block around `automountServiceAccountToken` in
the infinity template skips rendering when the value is explicitly `false`, so
the setting is omitted instead of being enforced. Update the template logic to
check the presence of `.Values.reranker.automountServiceAccountToken` without
using `with`, and ensure the `automountServiceAccountToken` field is emitted for
both `true` and `false` values so the `reranker` service account configuration
is applied consistently.
In `@infra/charts/openrag-stack/templates/NOTES.txt`:
- Around line 14-18: Update the NOTES text to warn only about whole-list
overrides of vllm.servingEngineSpec.modelSpec, since Helm replaces entire list
values from a values file but an indexed --set on
modelSpec[N].hf_token.secretName only affects that single entry. In the
NOTES.txt guidance, keep the warning focused on replacing the full modelSpec
list and remove language that says an indexed --set wipes
repository/tag/modelURL/resources for all entries; reference the
vllm.servingEngineSpec.modelSpec and hf_token.secretName examples when revising
the message.
In `@infra/charts/openrag-stack/templates/openrag.yaml`:
- Around line 26-28: The automountServiceAccountToken template block in the
OpenRAG manifest is using a truthiness check, so a false value gets omitted
entirely. Update the conditional around the openrag.automountServiceAccountToken
field to check for key presence instead of value truthiness, so the OpenRAG
chart can render an explicit false and allow operators to disable service
account token automounting.
In `@infra/charts/openrag-stack/templates/raycluster.yaml`:
- Around line 159-190: The Ray dashboard Ingress in raycluster.yaml is exposing
an unauthenticated administrative surface; update the ingress template around
the ray.ingress block to prevent external access unless protection is explicitly
enforced. Require an internal-only ingress class or mandatory auth-related
annotations for the dashboard route, and make the ray.ingress values document
the risk and default to a safer, non-public configuration. Use the existing
ray.ingress.enabled, className, annotations, and backend service wiring to keep
the change localized.
- Around line 25-27: The conditional rendering around
automountServiceAccountToken in the Ray cluster template is using truthiness, so
a false value is omitted and Kubernetes falls back to the default. Update the
template logic in the head and worker specs to check for nil/defined value
instead of using with, so the automountServiceAccountToken field is emitted for
both true and false. Locate the affected blocks in raycluster.yaml around the
automountServiceAccountToken entries and apply the same fix in both places.
In `@infra/charts/openrag-stack/values.yaml`:
- Around line 90-100: The PostgreSQL password is currently anchored as a literal
in the postgresql.auth.password values block, which can leave
env.secrets.POSTGRES_PASSWORD out of sync when overrides are applied. Update the
chart wiring so postgresql.auth.password and env.secrets.POSTGRES_PASSWORD both
resolve from the same template-backed value, or enforce that they must be set
together; use the existing postgresql.auth.password and
env.secrets.POSTGRES_PASSWORD entries as the points to align.
---
Duplicate comments:
In `@infra/charts/openrag-stack/values-linagora.yaml`:
- Around line 1-5: The overlay only updates the top-level
persistence.storageClass, so the PostgreSQL and Milvus sub-charts still keep the
base-file alias values. Update this values overlay to explicitly set
postgresql.primary.persistence.storageClass,
milvus.minio.persistence.storageClass, and milvus.etcd.persistence.storageClass
to longhorn alongside the existing persistence settings so the override applies
everywhere.
---
Nitpick comments:
In `@infra/charts/openrag-stack/values.yaml`:
- Around line 348-364: The adminUi.ingress values block is missing the
configuration keys referenced by its own routing note, so add stub fields for
the admin UI ingress shape to match openrag.ingress. Update the ingress section
in values.yaml to include the same discoverable keys (such as className, host,
annotations, and tls) alongside enabled, so templates like admin-ui ingress
rendering can resolve them consistently and the documented single-vhost setup is
exposed in the chart values.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c0a0788f-d49f-4518-a9c6-dabab8c94513
⛔ Files ignored due to path filters (1)
infra/charts/openrag-stack/Chart.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
.github/workflows/build_rc.ymldocs/content/docs/documentation/kubernetes.mdinfra/charts/openrag-stack/Chart.yamlinfra/charts/openrag-stack/templates/NOTES.txtinfra/charts/openrag-stack/templates/_helpers.tplinfra/charts/openrag-stack/templates/admin-ui.yamlinfra/charts/openrag-stack/templates/configmap-env.yamlinfra/charts/openrag-stack/templates/extra-objects.yamlinfra/charts/openrag-stack/templates/infinity.yamlinfra/charts/openrag-stack/templates/ingress.yamlinfra/charts/openrag-stack/templates/openrag.yamlinfra/charts/openrag-stack/templates/postgres-migration-job.yamlinfra/charts/openrag-stack/templates/pvc.yamlinfra/charts/openrag-stack/templates/raycluster.yamlinfra/charts/openrag-stack/templates/secrets-env.yamlinfra/charts/openrag-stack/values-linagora.yamlinfra/charts/openrag-stack/values.yamlinfra/docker/ui.Dockerfile
💤 Files with no reviewable changes (1)
- infra/charts/openrag-stack/templates/ingress.yaml
| DO NOT use "--set vllm.servingEngineSpec.modelSpec[N].hf_token.secretName=...". | ||
| Helm never merges lists across values sources — it replaces the whole list — | ||
| so a --set (or a values file) that only touches one field of one modelSpec | ||
| entry silently wipes out repository/tag/modelURL/resources for ALL FOUR | ||
| entries, breaking every vLLM deployment. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files:\n'
git ls-files infra/charts/openrag-stack/templates/NOTES.txt infra/charts/openrag-stack/values.yaml infra/charts/openrag-stack/Chart.yaml infra/charts/openrag-stack/templates -z \
| tr '\0' '\n' | sed 's#^`#-` #'
printf '\nNOTES.txt excerpt:\n'
cat -n infra/charts/openrag-stack/templates/NOTES.txt | sed -n '1,120p'
printf '\nSearch for modelSpec references:\n'
rg -n "modelSpec|servingEngineSpec|hf_token|secretName" infra/charts/openrag-stack -SRepository: linagora/openrag
Length of output: 2221
🌐 Web query:
Helm values list merge semantics --set replaces lists charts documentation
💡 Result:
In Helm, the merge semantics for lists and maps differ significantly when using the --set flag or values files. Maps are generally merged, meaning that new keys are added and existing keys are updated [1]. In contrast, lists (arrays) follow a replacement strategy [1][2]. When you use --set to provide a new value for a list, it completely replaces the existing list rather than merging the elements [1][2]. However, Helm provides specific syntax to target and override individual items within a list by their index [2][3][4]. Using the syntax --set key[index].property=value, you can modify specific fields of an object within an array without replacing the entire list [2][4]. Key takeaways: - Full list replacement: Setting a list key directly (e.g., --set list={a,b}) will overwrite the entire original list [1][2]. - Indexed overrides: You can perform deep overrides on objects within a list using array index notation (e.g., --set list.field=newvalue) [2][3][4]. - No native append: Standard --set does not natively support appending to lists (e.g., using a '-' index) in the way some other configuration tools do; it expects a numerical index to perform an override [3]. - Merging behavior: When multiple --values files or --set flags are used, the last (right-most) instruction takes precedence [5][6]. For complex scenarios where you need to compose or merge lists, standard Helm does not provide a built-in deep merge for arrays, though discussions and proposed features for better list management are ongoing within the community [7][4]. Developers often work around this by defining separate lists in values files and using template functions like concat if the chart supports it, or by structuring values as dictionaries (maps) rather than lists when frequent overriding is required [2][3].
Citations:
- 1: --set with an array index delete all the previous content of the array helm/helm#11734
- 2: Override one field of list in --set helm/helm#6853
- 3: Append to lists using array index syntax (
--set servers[-].port=80) helm/helm#30820 - 4: Enhancement: Allow deep overriding of Values in a list using
-f values.yamlthe way it works with--sethelm/helm#13075 - 5: https://github.com/helm/helm-www/blob/main/docs/helm/helm_install.md
- 6: https://v3-1-0.helm.sh/docs/helm/helm_install/
- 7: feat: Allow deep merge of lists with an optional index helm/helm#31314
🌐 Web query:
site:helm.sh docs values files arrays are replaced, not merged --set list semantics
💡 Result:
In Helm, when you use the --set flag to provide values, arrays (lists) are replaced, not merged [1][2][3]. When you define a list using --set, such as --set name={a,b,c}, it overrides the entire list at that path if it existed previously in the values [2][3]. It does not append to or merge with any existing list elements; the new list completely replaces the old one [2][3]. While individual items within a list can be modified using array index syntax (e.g., --set servers.port=80) [4][2][3], the top-level list structure itself behaves with replacement semantics rather than additive merging when defined via --set [2][3]. If you require complex merging of lists, this is typically handled by defining your values in a YAML file and using the --values (-f) flag, which provides more control over the merge process, or by relying on chart template logic to handle list concatenation [1][5][2].
Citations:
- 1: https://v3-1-0.helm.sh/docs/chart_template_guide/values_files/
- 2: https://v3-1-0.helm.sh/docs/intro/using_helm/
- 3: https://v2.helm.sh/docs/using_helm/
- 4: https://v3-1-0.helm.sh/docs/chart_best_practices/values/
- 5: https://v3-1-0.helm.sh/docs/topics/charts/
🌐 Web query:
Helm "Arrays are merged" values files replaced lists official documentation
💡 Result:
In Helm, when multiple values files or --set flags are provided, arrays (lists) are replaced, not merged [1][2]. When Helm processes multiple values sources, it performs a deep merge for maps (dictionaries), but it treats arrays as atomic values [3][2]. If a higher-priority values file or a --set flag defines an array, it completely replaces the array defined in the lower-priority values files or the default values.yaml [2]. This behavior is intentional and documented by the Helm project as a design choice to maintain predictability and avoid ambiguity regarding how lists should be merged (e.g., whether to append, prepend, or update existing elements) [2]. Consequently, if you need to combine list items from multiple files, you must handle that logic within your chart templates (for example, by using dict/map structures instead of lists or performing manual template logic) [3][2][4]. There is no native flag or configuration in Helm to enable "deep merging" for arrays [2].
Citations:
- 1: feat: Merge lists when passing multiple values files, allowing to sig… helm/helm#30632
- 2: Helm should preform deep merge on multiple values files helm/helm#3486
- 3: Multiple Values files - keys are overwritten not merged helm/helm#11581
- 4: Values files don't get deeply merged helm/helm#6668
Narrow this to whole-list overrides. Helm replaces list values from a values file, but --set vllm.servingEngineSpec.modelSpec[N].hf_token.secretName=... only updates that indexed entry. As written, the note wrongly implies indexed --set will wipe sibling fields from every modelSpec item.
🤖 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 `@infra/charts/openrag-stack/templates/NOTES.txt` around lines 14 - 18, Update
the NOTES text to warn only about whole-list overrides of
vllm.servingEngineSpec.modelSpec, since Helm replaces entire list values from a
values file but an indexed --set on modelSpec[N].hf_token.secretName only
affects that single entry. In the NOTES.txt guidance, keep the warning focused
on replacing the full modelSpec list and remove language that says an indexed
--set wipes repository/tag/modelURL/resources for all entries; reference the
vllm.servingEngineSpec.modelSpec and hf_token.secretName examples when revising
the message.
Ahmath-Gadji
left a comment
Ahmath-Gadji
left a comment
There was a problem hiding this comment.
Nice modernization overall — the NOTES.txt list-merge warning, proxy fail-fast checks, checksum/config annotation and the pinned infinity tag are all welcome. I rendered the chart with helm template and left inline comments on a handful of issues that I think should be addressed before merge (a few are confirmed rendering bugs, a few are security regressions vs the previous chart).
Two concerns that don't fit a single line:
Upgrade path / breaking renames. Every resource is renamed (rag-* → openrag-*, Release.Name-* → fullname-based), and postgresql/milvus gain fullnameOverrides. For any existing release not named openrag, an upgrade renames the Postgres/Milvus StatefulSets and creates fresh empty PVCs; even for a release named openrag, the app PVCs change name (rag-data → openrag-data), orphaning existing data (the old PVCs only survive via helm.sh/resource-policy: keep). The selector-label change (app.kubernetes.io/instance: .Release.Name → fullname) is part of the same story. Could we get an upgrade/migration note in NOTES.txt or docs/content/docs/documentation/kubernetes.md spelling this out?
venv coordination removed. The init-venv-sync / init-venv init containers (uv sync + .ready flag on the shared RWX venv PVC) are gone from both the openrag Deployment and the RayCluster, but the venv PVC is still mounted at /app/.venv everywhere. What populates the venv now, and what prevents the head, workers and app pod from racing concurrent uv runs on the same PVC at first boot? If the v2.0.0-rc.1 images bake the venv into the image, the PVC mount (which shadows it) should probably go too.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Documentation