helm: Allow configuring agent memory#1869
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a configurable per-agent memory block (with enabled, ttlDays, and modelConfigRef) to every built-in agent subchart, exposes those same knobs at the top-level helm/kagent/values.yaml so users can override them, and introduces an .editorconfig for repo-wide formatting defaults.
Changes:
- Add
memory.enabled/memory.ttlDays/memory.modelConfigRefto each agent'svalues.yamland render a correspondingmemory:block in each agent'stemplates/agent.yamlwhen enabled. - Surface the same
modelConfigRefandmemory.*fields per agent in the top-levelhelm/kagent/values.yaml. - Add a root
.editorconfigand minor whitespace/style cleanup.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| helm/kagent/values.yaml | Adds modelConfigRef/memory blocks for each agent; contains a duplicated memory: key and an inconsistent ~ value under kgateway-agent. |
| helm/agents/{k8s,kgateway,istio,promql,observability,argo-rollouts,helm,cilium-policy,cilium-manager,cilium-debug}/values.yaml | Introduce memory defaults (enabled: false, ttlDays: 15, modelConfigRef: ""). |
| helm/agents/{...}/templates/agent.yaml | Conditionally render memory.modelConfig and memory.ttlDays under spec.declarative when memory.enabled is true. |
| .editorconfig | New repo-wide formatting rules (LF, UTF-8, 2-space indent, trim trailing whitespace). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@mesutoezdil here's another example. |
I think we are about to break helm best practices again if we go that route. Similar like I recently fixed to be able to pass through per agent settings in my earlier PR. This is really the way to pass through values to subcharts. One example of a chart that does this correctly is the spire helm chart(s). Please let's not break the pattern again. global is another pattern that allows global defaults. Usually used for e.g. image registry. And usually that is used combined with post component ways of overriding. Even though there is duplication like this it makes the helm charts usable as individual components. Which is actually great as it allows people to make their own composite charts for their own specific usecases. |
1f2fce9 to
8b1fbf3
Compare
|
Fixed the DCO just now. |
89c9946 to
5a7a65e
Compare
5a7a65e to
0ff1f9f
Compare
Signed-off-by: Marco Franssen <marco.franssen@gmail.com>
0ff1f9f to
f70aec1
Compare
There was a problem hiding this comment.
Can we please remove this from the PR. If we want an .editorconfig let's leave that in a separate PR where we can discuss it
There was a problem hiding this comment.
2 issues need to be fixed.
required argument order is wrong in all 10 templates
The Helm required function signature is required "message" .value, but every template has the arguments swapped:
# current (broken)
modelConfig: {{ required .modelConfigRef "A compatible modelConfig must be provided when memory is enabled" }}
# should be
modelConfig: {{ required "A compatible modelConfig must be provided when memory is enabled" .modelConfigRef }}
With the arguments flipped, required treats the literal string as the value and .modelConfigRef as the error message.
An empty modelConfigRef will silently render the error string as the field value instead of failing at template render time.
This affects all 10 agent templates.
Trailing blank lines in argo-rollouts/templates/agent.yaml
The diff adds 6 empty lines at the end of the file.
Also agree with EItanya that .editorconfig belongs in a separate PR.
No description provided.