feat: initial release of jovvix Helm chart - #151
Conversation
- Add chart with API, UI, Kratos auth, PostgreSQL and Valkey sub-charts - Configure HTTPRoute for Envoy Gateway with override support - Implement required-secret pattern (no secrets in values.yaml) - Add database migration as a plain Job with retry loop - Support external PostgreSQL and Redis/Valkey via --set overrides - Add wait-for-db and wait-for-redis initContainers - Add detailed inline documentation in values.yaml
|
Preparing review... |
HussainTechSavvy
left a comment
HussainTechSavvy
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Changes Requested — 2 issues, 5 suggestions
Critical
- ci-values.yaml:14 —
secretsCipheris 30 characters but Kratos requires exactly 32. Will cause CI to fail. - kratos-secret.yaml:11 — DSN uses
.Values.database.usernamebut DB secret uses.Values.postgres.secret.username. Mismatch if only one is set.
Warnings
- values.yaml:534 —
kratos.development: trueleaks sensitive values in logs. Should default to false. - values.yaml:393 — Hardcoded dev SHA image tags. Consider semver release tags for a publishable chart.
- kratos-httproute.yaml:24 — Port 80 hardcoded for Kratos backends instead of referencing values.
Suggestions
- ingress.yaml — Unused
jovvix.ingress.backendhelper in _helpers.tpl. Use it or remove it. - migration-job.yaml — Missing resources on wait-for-db initContainer.
- postgres-statefulset.yaml:66 —
terminationGracePeriodSeconds: 30may be too low for PostgreSQL. - Consider squashing the 6 commits before merge.
| enabled: false | ||
| secrets: | ||
| secretsDefault: ci-kratos-default-secret-0123456789 | ||
| secretsCookie: ci-kratos-cookie-secret-0123456789 |
There was a problem hiding this comment.
🔴 Critical: secretsCipher: ci-kratos-cipher-secret-01234567 is 30 characters. Kratos requires exactly 32. This will fail at install time.
Fix: secretsCipher: ci-kratos-cipher-secret-0123456789
| {{- include "jovvix.labels" . | nindent 4 }} | ||
| type: Opaque | ||
| stringData: | ||
| dsn: {{ printf "postgres://%s:%s@%s:%d/%s?sslmode=%s&search_path=kratos" .Values.database.username (include "jovvix.dbPassword" .) (include "jovvix.databaseHost" .) (.Values.database.port | int) .Values.database.name .Values.database.sslMode | quote }} |
There was a problem hiding this comment.
🔴 Critical: DSN uses {{ .Values.database.username }} but the DB secret at db-secret.yaml uses {{ .Values.postgres.secret.username }}. If a user sets postgres.secret.username without also setting database.username, the DSN will have wrong credentials.
Fix: Use the same source for both, or document the coupling.
| appEnv: local | ||
| host: 0.0.0.0 | ||
| mode: development | ||
| port: "4000" |
There was a problem hiding this comment.
kratos.development: true enables leak_sensitive_values in logs and development mode. Should default to false for production.
| "additionalProperties": false | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
main-77988f8-1785401490). For a publishable chart, consider using release semver tags or leaving tag: "" to fall back to appVersion.
| port: 80 | ||
| weight: 1 | ||
| - matches: | ||
| - path: |
There was a problem hiding this comment.
80 is hardcoded. If the Kratos sub-chart is configured with a different port, this breaks silently. Consider referencing a value or the sub-chart service port.
The db-<fullname> Secret carried a key sourced from , but no template ever read it — all DB-username consumers use directly. Remove the dead key and the now-unused value to eliminate the config/runtime mismatch.
2 participants
jovvix-chart
Fixes Issue
Changes proposed
Check List (Check all the applicable boxes)
Screenshots
Note to reviewers