feat(helm): add backend storage driver support#998
Conversation
manusa
left a comment
There was a problem hiding this comment.
Thanks for this contribution! The config-based storage driver is a good addition. However, I'd like to narrow the scope: let's ship only the config default and drop the per-call storage_driver parameter from the three tools.
Why: Exposing the storage driver as a per-call parameter adds API surface that's hard to remove later, and it's unclear when an AI agent would need to switch drivers mid-conversation. The config default covers the real use case (clusters that store Helm releases in ConfigMaps instead of Secrets). If a per-call override turns out to be needed, we can add it in a follow-up.
Concretely, what to remove:
- The
storage_driverproperty from thehelm_install,helm_list, andhelm_uninstalltool schemas (pkg/toolsets/helm/helm.go) - The
resolveStorageDriverfunction — instead, readhelmCfg.StorageDriverdirectly (with a nil guard) - The
storage_driverentries fromREADME.mdandpkg/mcp/testdata/toolsets-helm-tools.json
What stays (and looks good):
StorageDriverfield inConfig+ TOML parsingvalidateStorageDriverand its call fromConfig.Validate()- The
storageDriverparameter threaded throughInstall/List/Uninstall/newAction(the plumbing is fine, it just gets its value from config only) docs/configuration.mdupdates- Config tests
One additional note on the tests: the project guidelines prefer testing through the public API. Consider folding the TestValidateStorageDriver cases into TestValidate by exercising Config.Validate() with different StorageDriver values, rather than calling the unexported validateStorageDriver directly.
|
Hey @manusa. I'm running this in an environment with mixed drivers in use (on lower envs users can use standard "secret" driver while in upper envs access to Kube Secrets is blocked hence the need to handled releases backed by CMs) for last 3 days and it works great. No issues with agent switching between different drivers smoothly and showing releases installed via both drivers on single cluster or in mix of clusters. Keeping the config level control only could be a good move forward but that actually helps only in uniform environments where there's just one single driver in use - from my experience this is rare case. In an enterprise env like mine, it is quite common to ban Kube Secrets use and that forces Helm users to switch to CMs but that is rarely enforced across all environments. This implementation has been created based on the environments I've been dealing with for last couple years. I'm ok to just submit the portion of this feature and keep remaining parts on my feature build. Re tests - I'll move the |
Let's break the work in two then. For this PR, keep the narrowed scope which can be merged easily (it won't affect existing LLM performance). Follow-up PR with the optional parameter. For this one we'll need to improve the eval suite and find a way to test this in the CI environment.
I trust your judgement, but still the evals are a must right now for a consolidated toolset. |
Signed-off-by: Jacek Luczak <difrost.kernel@gmail.com>
a238b29 to
08b1da1
Compare
@manusa please review. |
Currently helm toolset supports only default, 'secret' storage driver. This PR adds support of defining default storage driver (via config file) and per-call driver overrides.
Supported values: "secret", "configmap" and "" (Helms default driver - currently "secret").