Repository navigation
[3509] - Treat the UI as an optional client of the 1.x platform - #3527
asivanadi0 wants to merge 3 commits into
Conversation
Signed-off-by: Anpoo Sivanadi <sivanadi08@gmail.com>
Just saw that note. Ok. Lets pair on what the end state should be tomorrow. |
|
hiya @asivanadi0 thank you! OK I think this could work. I am wondering wether there's any kind of packaging changes we should do to to make sure the non ui code can be packaged differently, but maybe I'm over engineering it I'n my head? I was thinking about CVE payloads in the artifacts, also, as one of the problems to solve. |
|
Yeah, worth looking at. This toggle only skips deploying the UI; it doesn’t change what’s in the images. Did you have a particular image or dependency in mind for the CVE concern? I’d keep this PR focused on the toggle may be? Packaging handled and seperately at #3430. |
|
Ah ya lets split that out to a separate issue to keep the chains flowing good idea. I will try to test this today if I can set up a cloud acct. |
Charlesthebird
left a comment
Charlesthebird
left a comment
There was a problem hiding this comment.
This automated review is a first pass. A manual review will follow once the blockers are addressed.
Nothing blocks merge. One Medium: a misconfigured ui.pdb still fails the render when ui.enabled=false.
What I checked ⚠️
- Each UI template is gated on
ui.enabled; ConfigMaps, Deployment, Service, ServiceAccount, PDB, HTTPRoute and Route all covered ✅ helm templatewithui.enabled=false,ui.pdb.enabled=true,ui.pdb.minAvailable=5fails inui-pdb.yaml:2⚠️ - Default
ui.enabled: truekeeps existing installs unchanged ✅
Written by 🤖
| @@ -1,6 +1,6 @@ | |||
| {{- $pdb := .Values.ui.pdb | default dict }} | |||
| {{- include "kagent.pdb.validate" (dict "pdb" $pdb "path" "ui.pdb") }} | |||
There was a problem hiding this comment.
Level: 🟠 Medium · Not Blocking
kagent.pdb.validate runs before the ui.enabled guard, so a stale ui.pdb (e.g. both minAvailable and maxUnavailable set) still fails helm template with the UI off. Move the include inside the if.
Written by 🤖
| port: http | ||
| periodSeconds: 30 | ||
| {{- end }} | ||
| {{ end }} No newline at end of file |
There was a problem hiding this comment.
[nit]
Level: 🟡 Low · Not Blocking
The closing {{ end }} has no - and the file has no trailing newline; {{- end }} plus a newline matches the other templates (same in the other four gated UI templates).
Written by 🤖
|
Terminal: reel.mp4Written by 🤖 |
Signed-off-by: Anpoo Sivanadi <sivanadi08@gmail.com>
Signed-off-by: Anpoo Sivanadi <sivanadi08@gmail.com>
Adds a top level
ui.enabledvalue to the kagent chart so you can turn the UI off. Refs #3509What changes
ui.enableddefaults totrueso existing installs render the same manifests as main, no UI pod restart on upgrade.ui.enabled=falsethe chart skips the UI Deployment, Service, ServiceAccount and both configmaps (ui config + nginx). UI PDB, HTTPRoute and the OpenShift Route are skipped too, even if their own flags are onTesting
ui.enabled=falserenders nothing UI related even with PDB/HTTPRoute turned on, and NOTES on/off.make helm-testpasseshelm lintandhelm templatewith the flag on and offKnown follow ups (not in this PR)
kagent dashboardalways port-forwards toservice/kagent-uiso it'll error out when the UI is disabledKAGENT_UI_URLinto agents ifui.externalUrlis set, even with UI offui.externalUrloauth2-proxydefaultsUPSTREAM_URLtohttp://kagent-ui:8080which wont have anything behind it when the UI is disabledThis might overlap with #3430 (moves UI serving into the controller). happy to rename the value if that lands so it still makes sense.
Test Results
make helm-testui-enabled_test.yaml,ui-enabled-notes_test.yaml)helm lint helm/kagenthelm lint helm/kagent --set ui.enabled=falsemainhelm templatediff againstmainhelm template --set ui.enabled=false--set ui.enabled=falsewithui.pdb.enabled,ui.httpRoute.enabled,ui.route.enabledset to trueDisclamer
AI assisted with implementing the helm changes and unit tests in this PR, including running the validation of the tests as well!