feat: simplified helm charts#35
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThe Helm chart configuration is updated to automatically enable providers when an API key is set, eliminating the need for explicit enablement flags. Changes span documentation, template logic, schema validation, and configuration values to reflect this new provider detection behavior. Changes
Possibly related PRs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
Pull request overview
This PR simplifies the Helm chart configuration by implementing auto-enabling of providers based on API key presence, reducing the need for users to explicitly set enabled: true for each provider. This streamlines the user experience while maintaining backward compatibility through the explicit enabled flag.
Changes:
- Auto-enable providers when an API key is provided, eliminating the need to set both
enabled: trueandapiKey - Simplified JSON schema validation by removing complex conditional checks for enabled providers and switching to
additionalProperties: truefor many configuration objects - Updated documentation to reflect the new auto-enable behavior
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| helm/values.yaml | Updated comments to indicate providers are auto-enabled when apiKey is set |
| helm/values.schema.json | Simplified schema validation by removing complex conditional logic and excessive property definitions |
| helm/templates/_helpers.tpl | Modified provider detection logic to check for apiKey in addition to enabled flag, introduced unused gomodel.providerEnabled helper |
| helm/templates/NOTES.txt | Updated provider detection and warning message to reflect auto-enable behavior |
| helm/README.md | Updated installation examples and documentation to show simplified configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {{- define "gomodel.providerEnabled" -}} | ||
| {{- $config := . -}} | ||
| {{- if or $config.enabled $config.apiKey -}} | ||
| true | ||
| {{- end -}} | ||
| {{- end -}} |
There was a problem hiding this comment.
The template helper gomodel.providerEnabled is defined but never used elsewhere in the codebase. The same logic is implemented inline in gomodel.providerEnvVars and NOTES.txt. Consider either using this helper consistently throughout the templates or removing it to avoid dead code.
| {{- $secretName := include "gomodel.providerSecretName" . -}} | ||
| {{- range $name, $config := .Values.providers }} | ||
| {{- if and (kindIs "map" $config) (hasKey $config "enabled") $config.enabled }} | ||
| {{- if and (kindIs "map" $config) (hasKey $config "apiKey") (or $config.enabled $config.apiKey) }} |
There was a problem hiding this comment.
The condition (or $config.enabled $config.apiKey) is redundant since the preceding check (hasKey $config \"apiKey\") $config.apiKey already ensures apiKey exists and is truthy. When $config.apiKey is truthy, the or will always be true. This can be simplified to just check (or $config.enabled $config.apiKey) without the redundant hasKey and truthy check, or the logic needs to be clarified if the intent is different.
| } | ||
| "additionalProperties": true | ||
| }, | ||
| "imagePullSecrets": { "type": "array" }, |
There was a problem hiding this comment.
The schema validation for imagePullSecrets was simplified from a structured array with object items containing name properties to just {\"type\": \"array\"}. This removes validation that ensures each item has the correct structure. Consider retaining the items validation to catch configuration errors where imagePullSecrets entries might be malformed.
| "imagePullSecrets": { "type": "array" }, | |
| "imagePullSecrets": { | |
| "type": "array", | |
| "items": { | |
| "type": "object", | |
| "properties": { | |
| "name": { | |
| "type": "string", | |
| "minLength": 1 | |
| } | |
| }, | |
| "required": ["name"], | |
| "additionalProperties": false | |
| } | |
| }, |
| } | ||
| } | ||
| }, | ||
| "tolerations": { "type": "array" }, |
There was a problem hiding this comment.
The schema validation for tolerations was simplified from a structured array with validated properties (key, operator, value, effect, tolerationSeconds) to just {\"type\": \"array\"}. This removes validation that ensures tolerations have valid Kubernetes-compliant structure. Consider retaining the items validation to catch configuration errors.
| } | ||
| }, | ||
| "ingress": { | ||
| "type": "object", |
There was a problem hiding this comment.
The detailed schema validation for ingress configuration (including hosts, paths, pathType enum validation, and TLS settings) has been replaced with additionalProperties: true. This removes validation that would catch common configuration errors like invalid pathType values (must be 'Prefix', 'Exact', or 'ImplementationSpecific'). Consider retaining at least the pathType enum validation as it's a common source of errors.
| "type": "object", | |
| "type": "object", | |
| "properties": { | |
| "enabled": { "type": "boolean" }, | |
| "className": { "type": "string" }, | |
| "annotations": { "type": "object" }, | |
| "hosts": { | |
| "type": "array", | |
| "items": { | |
| "type": "object", | |
| "properties": { | |
| "host": { "type": "string" }, | |
| "paths": { | |
| "type": "array", | |
| "items": { | |
| "type": "object", | |
| "properties": { | |
| "path": { "type": "string" }, | |
| "pathType": { | |
| "type": "string", | |
| "enum": ["Prefix", "Exact", "ImplementationSpecific"] | |
| } | |
| }, | |
| "required": ["pathType"], | |
| "additionalProperties": true | |
| } | |
| } | |
| }, | |
| "additionalProperties": true | |
| } | |
| }, | |
| "tls": { | |
| "type": "array", | |
| "items": { | |
| "type": "object", | |
| "properties": { | |
| "secretName": { "type": "string" }, | |
| "hosts": { | |
| "type": "array", | |
| "items": { "type": "string" } | |
| } | |
| }, | |
| "additionalProperties": true | |
| } | |
| } | |
| }, |
| "additionalProperties": true | ||
| }, | ||
| "autoscaling": { | ||
| "type": "object", |
There was a problem hiding this comment.
The autoscaling schema validation was simplified from enforcing minimum values (minReplicas and maxReplicas both required minimum: 1) to additionalProperties: true. This removes important validation that prevents invalid configurations like 0 or negative replica counts. Consider retaining the minimum value validations for minReplicas and maxReplicas.
| "type": "object", | |
| "type": "object", | |
| "properties": { | |
| "minReplicas": { | |
| "type": "integer", | |
| "minimum": 1 | |
| }, | |
| "maxReplicas": { | |
| "type": "integer", | |
| "minimum": 1 | |
| } | |
| }, |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
helm/templates/_helpers.tpl (1)
130-144: Fix env var generation to support existingSecret withenabled=true.The condition at line 133 requires
hasKey $config "apiKey", but when usingexistingSecretwith onlyenabled=true(without settingapiKeydirectly), this condition fails and prevents the secretKeyRef from being generated. According to the documentation, users should setenabled=truewhen usingexistingSecretsince the apiKey is not provided in values. The condition should be updated to allow environment variable generation when eitherenabled=trueORapiKeyexists:{{- if and (kindIs "map" $config) (or $config.enabled (hasKey $config "apiKey")) }}This ensures the secretKeyRef correctly references the existing secret even when the provider is enabled without a direct apiKey value.
🤖 Fix all issues with AI agents
In `@helm/templates/_helpers.tpl`:
- Around line 106-114: The template gomodel.providerEnabled is defined but never
used and duplicates logic found inline in NOTES.txt and in the
gomodel.providerEnvVars template; either replace the duplicated inline checks
with calls to the gomodel.providerEnabled template (e.g., invoke {{ template
"gomodel.providerEnabled" . }} where NOTES.txt and gomodel.providerEnvVars
currently use or .enabled .apiKey) to centralize the logic, or delete the unused
gomodel.providerEnabled definition if you prefer the inline form—keep a single
source of truth by updating NOTES.txt and gomodel.providerEnvVars accordingly.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
helm/README.mdhelm/templates/NOTES.txthelm/templates/_helpers.tplhelm/values.schema.jsonhelm/values.yaml
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: ENTERPILOT/GOModel PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-28T22:10:31.364Z
Learning: Applies to config/config.go : At least one provider API key is required in the environment configuration to start the server
📚 Learning: 2025-12-28T22:10:31.364Z
Learnt from: CR
Repo: ENTERPILOT/GOModel PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-28T22:10:31.364Z
Learning: Applies to config/config.go : At least one provider API key is required in the environment configuration to start the server
Applied to files:
helm/templates/NOTES.txthelm/values.yamlhelm/templates/_helpers.tpl
📚 Learning: 2025-12-28T22:10:31.364Z
Learnt from: CR
Repo: ENTERPILOT/GOModel PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-28T22:10:31.364Z
Learning: Applies to cmd/gomodel/main.go : Provider packages must be imported in `cmd/gomodel/main.go` with blank imports (e.g., `_ "gomodel/internal/providers/openai"`) to trigger their init() registration
Applied to files:
helm/templates/NOTES.txthelm/README.mdhelm/templates/_helpers.tpl
📚 Learning: 2026-01-15T04:47:09.662Z
Learnt from: kowtom
Repo: ENTERPILOT/GOModel PR: 31
File: helm/templates/gateway.yaml:9-13
Timestamp: 2026-01-15T04:47:09.662Z
Learning: For Helm charts, prefer declarative validation in values.schema.json using JSON Schema conditionals (if/then) over imperative template-level validation with `fail` statements. Schema validation runs earlier and provides better user experience.
Applied to files:
helm/values.schema.json
🪛 LanguageTool
helm/templates/NOTES.txt
[style] ~38-~38: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...or .Values.providers.anthropic.enabled .Values.providers.anthropic.apiKey }}{{ $enable...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~39-~39: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...if or .Values.providers.gemini.enabled .Values.providers.gemini.apiKey }}{{ $enabledPr...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~40-~40: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...- if or .Values.providers.groq.enabled .Values.providers.groq.apiKey }}{{ $enabledProv...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~41-~41: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...{- if or .Values.providers.xai.enabled .Values.providers.xai.apiKey }}{{ $enabledProvi...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (8)
helm/values.yaml (1)
44-82: LGTM!The comment updates accurately document the new auto-enabling behavior when
apiKeyis set. The default values remain appropriate since providers can now be enabled either explicitly or via apiKey presence.helm/templates/NOTES.txt (1)
36-50: LGTM!The provider detection logic correctly checks for either
enabledorapiKeypresence, aligning with the new auto-enable behavior in_helpers.tpl. The updated warning message appropriately guides users to provide an API key.Note: The static analysis hints about "successive sentences" are false positives—this is template syntax, not prose.
helm/README.md (2)
23-41: LGTM!The installation examples are cleaner and correctly demonstrate:
- Simple install with auto-enabling via
apiKey- Multi-provider setup without explicit
enabledflags- GitOps pattern with
existingSecret+enabled=true(necessary sinceapiKeyisn't set directly)
86-92: Clear documentation for the existingSecret pattern.The explanation that
enabled=trueis needed when usingexistingSecret(sinceapiKeyisn't set directly) helps users understand the two distinct configuration approaches.helm/templates/_helpers.tpl (1)
119-125: LGTM!The condition correctly ensures that only non-empty API keys are written to the Secret's stringData. The
$config.apiKeytruthiness check prevents empty entries.helm/values.schema.json (3)
58-121: Schema validation is permissive but functional.The
anyOfblock validates that at least one of:existingSecretOR any provider'sapiKeyhas a non-empty value. This aligns with the requirement that at least one provider API key is required.However, the schema doesn't enforce that when
existingSecretis used, at least one provider must haveenabled=true. Users relying only onexistingSecretwithout settingenabledwill pass schema validation but get a "No providers enabled" warning at deploy time (from NOTES.txt). This is an acceptable trade-off since the warning guides users, but consider adding a note in the schema description or README about this behavior.
246-281: Broad permissiveness withadditionalProperties: true.Adding
additionalProperties: trueto ingress, gateway, resources, probes, and security contexts provides flexibility for Kubernetes-specific fields and subchart options. This is a reasonable trade-off between strict validation and usability, though it means typos in these sections won't be caught at validation time.
205-219: LGTM!The
serverandauthschema definitions properly declare the expected fields with appropriate types, filling a gap in the previous schema.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {{- range $name, $config := .Values.providers }} | ||
| {{- if and (kindIs "map" $config) (hasKey $config "enabled") }} | ||
| {{- if and $config.enabled $config.apiKey }} | ||
| {{- if and (kindIs "map" $config) (hasKey $config "apiKey") $config.apiKey }} |
There was a problem hiding this comment.
The logic change removes support for the enabled flag when using existingSecret. When providers.existingSecret is set, individual provider apiKey values are empty (as documented), but users still need to explicitly enable providers. The condition should be {{- if and (kindIs \"map\" $config) (or (and (hasKey $config \"enabled\") $config.enabled) (and (hasKey $config \"apiKey\") $config.apiKey)) }} to support both the auto-enable behavior and the existingSecret scenario.
| {{- $secretName := include "gomodel.providerSecretName" . -}} | ||
| {{- range $name, $config := .Values.providers }} | ||
| {{- if and (kindIs "map" $config) (hasKey $config "enabled") $config.enabled }} | ||
| {{- if and (kindIs "map" $config) (hasKey $config "apiKey") $config.apiKey }} |
There was a problem hiding this comment.
Same issue as in gomodel.providerSecretData - when using existingSecret, apiKey values are empty but providers still need to be enabled. The condition should support both auto-enable via apiKey and explicit enablement via the enabled flag.
| {{- if and (kindIs "map" $config) (hasKey $config "apiKey") $config.apiKey }} | |
| {{- if and (kindIs "map" $config) (or (and (hasKey $config "apiKey") $config.apiKey) (and (hasKey $config "enabled") $config.enabled)) }} |
| {{- if or .Values.providers.openai.enabled .Values.providers.openai.apiKey }}{{ $enabledProviders = append $enabledProviders "openai" }}{{ end }} | ||
| {{- if or .Values.providers.anthropic.enabled .Values.providers.anthropic.apiKey }}{{ $enabledProviders = append $enabledProviders "anthropic" }}{{ end }} | ||
| {{- if or .Values.providers.gemini.enabled .Values.providers.gemini.apiKey }}{{ $enabledProviders = append $enabledProviders "gemini" }}{{ end }} | ||
| {{- if or .Values.providers.groq.enabled .Values.providers.groq.apiKey }}{{ $enabledProviders = append $enabledProviders "groq" }}{{ end }} | ||
| {{- if or .Values.providers.xai.enabled .Values.providers.xai.apiKey }}{{ $enabledProviders = append $enabledProviders "xai" }}{{ end }} |
There was a problem hiding this comment.
This logic correctly preserves backward compatibility by checking both enabled flag and apiKey, but it's inconsistent with the template helpers which only check apiKey. This discrepancy means NOTES.txt might show a provider as enabled when it won't actually be configured in the deployment. Consider adding a check for existingSecret as well: {{- if or .Values.providers.openai.enabled .Values.providers.openai.apiKey .Values.providers.existingSecret }}
| {{- if or .Values.providers.openai.enabled .Values.providers.openai.apiKey }}{{ $enabledProviders = append $enabledProviders "openai" }}{{ end }} | |
| {{- if or .Values.providers.anthropic.enabled .Values.providers.anthropic.apiKey }}{{ $enabledProviders = append $enabledProviders "anthropic" }}{{ end }} | |
| {{- if or .Values.providers.gemini.enabled .Values.providers.gemini.apiKey }}{{ $enabledProviders = append $enabledProviders "gemini" }}{{ end }} | |
| {{- if or .Values.providers.groq.enabled .Values.providers.groq.apiKey }}{{ $enabledProviders = append $enabledProviders "groq" }}{{ end }} | |
| {{- if or .Values.providers.xai.enabled .Values.providers.xai.apiKey }}{{ $enabledProviders = append $enabledProviders "xai" }}{{ end }} | |
| {{- if or .Values.providers.openai.enabled .Values.providers.openai.apiKey .Values.providers.openai.existingSecret }}{{ $enabledProviders = append $enabledProviders "openai" }}{{ end }} | |
| {{- if or .Values.providers.anthropic.enabled .Values.providers.anthropic.apiKey .Values.providers.anthropic.existingSecret }}{{ $enabledProviders = append $enabledProviders "anthropic" }}{{ end }} | |
| {{- if or .Values.providers.gemini.enabled .Values.providers.gemini.apiKey .Values.providers.gemini.existingSecret }}{{ $enabledProviders = append $enabledProviders "gemini" }}{{ end }} | |
| {{- if or .Values.providers.groq.enabled .Values.providers.groq.apiKey .Values.providers.groq.existingSecret }}{{ $enabledProviders = append $enabledProviders "groq" }}{{ end }} | |
| {{- if or .Values.providers.xai.enabled .Values.providers.xai.apiKey .Values.providers.xai.existingSecret }}{{ $enabledProviders = append $enabledProviders "xai" }}{{ end }} |
| "operator": { "type": "string" }, | ||
| "operator": { | ||
| "type": "string", | ||
| "enum": ["Exists", "Equal"] |
There was a problem hiding this comment.
The enum constraint is overly restrictive. Kubernetes tolerations support operators 'Exists' and 'Equal', but the casing should match Kubernetes API exactly. However, this appears to already be correct and is unrelated to the main PR purpose of simplifying provider configuration.
| "enum": ["Exists", "Equal"] | |
| "enum": ["Exists", "Equal", ""] |
| "minimum": 1, | ||
| "maximum": 100 | ||
| }, | ||
| "targetMemoryUtilizationPercentage": { | ||
| "type": "integer", | ||
| "minimum": 1, | ||
| "maximum": 100 |
There was a problem hiding this comment.
While the maximum constraint of 100 makes sense for percentages, this is a schema improvement unrelated to the main PR purpose. This is good practice but should ideally be in a separate PR focused on schema validation improvements.
| "minimum": 1, | |
| "maximum": 100 | |
| }, | |
| "targetMemoryUtilizationPercentage": { | |
| "type": "integer", | |
| "minimum": 1, | |
| "maximum": 100 | |
| "minimum": 1 | |
| }, | |
| "targetMemoryUtilizationPercentage": { | |
| "type": "integer", | |
| "minimum": 1 |
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.