Sync kubex charts from automation-controller main @ 71b5a52#110
Conversation
There was a problem hiding this comment.
CONTENT OF THIS REVIEW IS AI GENERATED
Overall assessment: This is a clean, well-scoped change that makes DENSIFY_BASE_URL configurable rather than hard-coded to https. The schema, default values, example values file, and documentation are all updated consistently. The primary risk is a missing nil/default guard in the template expression.
Risk level: Low
Critical issues:
- None
Major issues:
gateway-secret.yaml— nodefaultfallback forscheme: If a user's existing override omitskubex.url.schemeat the map level, the rendered URL becomes"://<host>". Adding(default "https" .Values.kubex.url.scheme)in theprintfcall is a one-token fix that makes the template self-defending.
Minor issues:
values.schema.json—schemeabsent fromif/thenrequired list: The conditional block that fires forcreateSecrets=truevalidateshostbut notscheme. This is acceptable if the template-leveldefaultfix is applied, but adding a JSON Schema"default": "https"annotation would be a nice machine-readable complement.Configuration-Reference.md— table row omits default and allowed values: The new row forkubex.url.schemeshould state the default (https) and the allowed values (http|https) so users don't have to cross-referencevalues.yamlor the schema.
DRY improvement opportunities:
- None identified; the
schemevalue is referenced in exactly one place in the templates.
Suggested next steps:
- Apply
(default "https" .Values.kubex.url.scheme)ingateway-secret.yamlto harden against missing-key scenarios. - Optionally add
"default": "https"to theschemeproperty invalues.schema.jsonfor machine-readable documentation. - Expand the
Configuration-Reference.mdtable row with default and allowed values.
| url: {{ .Values.kubex.url.host | b64enc | quote }} | ||
| DENSIFY_BASE_URL: {{ printf "https://%s" .Values.kubex.url.host | b64enc | quote }} | ||
| DENSIFY_BASE_URL: {{ printf "%s://%s" .Values.kubex.url.scheme .Values.kubex.url.host | b64enc | quote }} | ||
| {{- end }} |
There was a problem hiding this comment.
CONTENT OF THIS REVIEW IS AI GENERATED
[Severity: Major] [Confidence: High]
Location: charts/kubex-automation-engine/templates/gateway-secret.yaml:19
Issue: kubex.url.scheme is used directly in printf without a nil/empty guard, so if an existing values override omits this key the rendered URL will be "://<host>".
Why it matters: The values.yaml default provides scheme: "https" for fresh installs, but any user whose override file predates this PR will not have scheme set. Helm deep-merges values files; a key absent from an override is inherited from values.yaml, so most users are protected. However, if a user explicitly sets kubex.url: {host: "..."} as a map in their override (omitting scheme entirely at the map level), the default can be clobbered in some Helm versions. A default "https" call makes the template self-defending regardless:
DENSIFY_BASE_URL: {{ printf "%s://%s" (default "https" .Values.kubex.url.scheme) .Values.kubex.url.host | b64enc | quote }}Suggested fix: Add a default "https" fallback in the template expression as shown above.
| "enum": ["http", "https"] | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
CONTENT OF THIS REVIEW IS AI GENERATED
[Severity: Minor] [Confidence: High]
Location: charts/kubex-automation-engine/values.schema.json:333 (the url object's required list inside the if/then block)
Issue: The if/then conditional validation block (triggered when createSecrets=true) requires kubex.url.host but does not require kubex.url.scheme. This means schema validation will pass even when scheme is absent, which in turn allows the broken-URL scenario in the template to go undetected at helm install / helm upgrade validation time.
Why it matters: Schema validation is the first line of defence. If scheme must be present when the secret is created, adding it to the required list here (or relying on the default fix above) closes the gap. Given the template-level default fix is the stronger protection, this is a minor layered-defence gap rather than a blocker.
Suggested fix: Either (a) add "scheme" to the required array inside the url object in the then block, or (b) rely on the default "https" in the template and document that scheme is optional with a documented default — which is the current intent. If the latter, a JSON Schema default annotation on the scheme property would make the default machine-readable:
"scheme": {
"type": "string",
"enum": ["http", "https"],
"default": "https"
}| | Key | Description | | ||
| | --- | --- | | ||
| | `kubex.url.host` | Kubex hostname, for example `example.kubex.ai` | | ||
| | `kubex.url.scheme` | Scheme used when deriving `DENSIFY_BASE_URL` from `kubex.url.host` | |
There was a problem hiding this comment.
CONTENT OF THIS REVIEW IS AI GENERATED
[Severity: Minor] [Confidence: Medium]
Location: charts/kubex-automation-engine/docs/Configuration-Reference.md:49
Issue: The kubex.url.scheme table row does not document the default value (https) or the allowed values (http | https), unlike the inline comment in kubex-automation-values.yaml which does mention http.
Why it matters: Users consulting only the Configuration Reference will not know what values are valid or what the default is, increasing the chance of misconfiguration.
Suggested fix: Expand the description to include default and valid values, for example:
| `kubex.url.scheme` | Scheme used when deriving `DENSIFY_BASE_URL` from `kubex.url.host`. Allowed values: `http`, `https`. Default: `https`. |
This PR was created automatically to sync managed Kubex chart content.
Source commit:
71b5a52Managed chart scope:
charts/kubex-automation-enginecharts/kubex-crds