feat: initial release of jovvix Helm chart by drashti-itpl · Pull Request #151 · Improwised/charts

drashti-itpl · 2026-08-11T07:53:46Z

jovvix-chart

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

Fixes Issue

Changes proposed

Check List (Check all the applicable boxes)

My code follows the code style of this project.
My change requires changes to the documentation.
I have updated the documentation accordingly.
All new and existing tests passed.
This PR does not contain plagiarized content.
The title of my pull request is a short description of the requested changes.

Screenshots

Note to reviewers

- 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

…-values

HussainTechSavvy · 2026-08-17T07:51:44Z

Preparing review...

HussainTechSavvy

Code Review Summary

Verdict: Changes Requested — 2 issues, 5 suggestions

Critical

ci-values.yaml:14 — secretsCipher is 30 characters but Kratos requires exactly 32. Will cause CI to fail.
kratos-secret.yaml:11 — DSN uses .Values.database.username but DB secret uses .Values.postgres.secret.username. Mismatch if only one is set.

Warnings

values.yaml:534 — kratos.development: true leaks 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.backend helper in _helpers.tpl. Use it or remove it.
migration-job.yaml — Missing resources on wait-for-db initContainer.
postgres-statefulset.yaml:66 — terminationGracePeriodSeconds: 30 may be too low for PostgreSQL.
Consider squashing the 6 commits before merge.

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.

db-secret.yaml used postgres.secret.username while all other templates (kratos-secret, api-configmap, postgres-statefulset, initContainers) used database.username. Remove the redundant postgres.secret.username field so all components reference the same value.

Set kratos.development, kratos.kratos.development, and leak_sensitive_values to false for secure defaults in a publishable chart.

Replace dev SHA tags with empty strings for api, migration, and ui images in values.yaml. Tags default to Chart.yaml appVersion and can be overridden via --set at install time. Add Docker Hub links in comments for finding available tags. CI uses real tags from Docker Hub.

Rewrite ingress.yaml to route all endpoints (api, ui, kratos admin and public) using global.domain as hostname. Both Ingress and HTTPRoute are supported — HTTPRoute is the default, Ingress can be enabled with --set ingress.enabled=true. CI tests the default HTTPRoute mode.

Remove jovvix.ingress.backend and jovvix.ingress.pathtype defines from _helpers.tpl. Neither is referenced by any template after the ingress.yaml rewrite.

Increase default from 30s to 120s for PostgreSQL to allow in-flight transactions to complete during shutdown. Value is now configurable via postgres.terminationGracePeriodSeconds in values.yaml.

The Gateway API CRDs are not installed in the ct test cluster, but HTTPRoute is enabled by default. Disable api, ui, and kratos HTTPRoutes in ci-values.yaml so the chart installs without requiring the Gateway API CRDs, restoring CI to passing.

HussainTechSavvy

Code Review — Jovvix Helm Chart

Verdict: Changes Requested — All 8 items from my previous review have been addressed correctly. Below are 5 new issues found on the current head.

Warnings

values.yaml:472-473 — API still defaults to development/debug mode
api-configmap.yaml:40 — admin base URL omits the port (inconsistent with public URL)
values.yaml:12 — unused global.databaseHost config value

Suggestions

valkey-secret.yaml:6 — secret name coupled to sub-chart value
migration-job.yaml — no ordering between migration Job and API start

See inline comments for details.

Set debug and isDevelopment to false so the API does not run in development/debug mode by default. Production deployments should not leak sensitive values or bypass HTTPS enforcement.

SERVE_ADMIN_BASE_URL was missing the serveAdminPort suffix, making it inconsistent with SERVE_PUBLIC_BASE_URL. Add the port so both URLs are formed the same way.

global.databaseHost was never referenced by any template; the jovvix.databaseHost helper uses database.host instead. Drop the dead value to avoid confusion.

valkey-secret.yaml derived the Secret name from valkey.auth.usersExistingSecret, coupling our resource to a sub-chart config value. Add a chart-owned valkey.secret.name (default jovvix-valkey) and use it in the template instead. The sub-chart's usersExistingSecret is kept in sync to reference our secret.

HussainTechSavvy

Code Review — Jovvix Helm Chart (Round 2)

Verdict: Changes Requested

Previous review items — resolved

All 4 code fixes from the last round are correct and verified by rendering the chart:

✅ api.config.debug / isDevelopment now default to "false"
✅ dead global.databaseHost config removed
✅ SERVE_ADMIN_BASE_URL now includes :{{ serveAdminPort }} (consistent with the public URL)
✅ Valkey Secret name decoupled via valkey.secret.name (verified the Secret resolves consistently to jovvix-valkey)

No regressions were introduced by these changes.

Remaining outstanding item

The migration-Job / API startup-ordering suggestion was not addressed — migration-job.yaml is unchanged on this head. See the inline comment.

New finds from the full fresh review

Minor (non-blocking): the UI still defaults to mode: development / appEnv: local in values.yaml while the API and Kratos were just flipped to production-mode defaults. Not a security leak; just noted for consistency.

HussainTechSavvy · 2026-09-01T08:40:47Z

+    {{- include "jovvix.labels" . | nindent 4 }}
+    app.kubernetes.io/component: migration
+  annotations:
+    "helm.sh/hook": post-install,post-upgrade


SUGGESTION (still open from the previous review): The jovvix migrate up Job is a post-install,post-upgrade hook with no helm.sh/hook-weight, and the API Deployment only waits for the database to become ready (pg_isready in its initContainer) — nothing orders the API to start after this migration Job completes. On a fresh install the API pod can start and hit the DB before the schema migration finishes. Consider gating the API on migration completion (e.g. a lightweight initContainer that polls for a migration-complete marker, or documenting the intended ordering).

- Register valkey and ory helm repos in ct.yaml and CI workflow - Add ci/ci-values.yaml with required secrets and httpRoute disabled - Fix values.yaml trailing blank line (yamllint empty-lines rule) - Add maintainers to Chart.yaml - Add kratos.httpRoute.port parameter (replaces hardcoded port 80) - Add kratos.development warning comments - Move migration from post-install hook Job to API initContainer

HussainTechSavvy

Code Review — Jovvix Helm Chart (Round 3)

Verdict: Changes Requested

Previous items — resolved

✅ Migration ordering: the migration was moved from a post-install Job into an API initContainer ordered after wait-for-db and wait-for-redis. Good.

New issues from the migration-as-initContainer change

Migration silently skipped when api.initContainers.enabled: false (see inline)
Concurrent migrations on replica scale-out (see inline)
Stale "migration job" doc comment (see inline)

Still outstanding from earlier rounds

Empty image tag falls back to placeholder appVersion: "1.0.0" (see inline)

See inline comments for details.

HussainTechSavvy · 2026-09-08T09:12:19Z

+          resources:
+            {{- toYaml . | nindent 12 }}
+          {{- end }}
+        {{- if .Values.api.migration.enabled }}


WARNING (regression): The migration initContainer is nested inside the {{ if .Values.api.initContainers.enabled }} guard that starts at line 51. If a user sets api.initContainers.enabled: false (e.g. external DB/Redis where wait-for-* is unnecessary, or they supply their own init containers), the migration is silently skipped even though api.migration.enabled defaults to true. The previous Job was gated only on api.migration.enabled, so this is a behavior regression. Fix: gate the migration initContainer on api.migration.enabled alone, independent of api.initContainers.enabled.

HussainTechSavvy · 2026-09-08T09:12:19Z

+            {{- toYaml . | nindent 12 }}
+          {{- end }}
+        {{- if .Values.api.migration.enabled }}
+        - name: migration


WARNING: jovvix migrate up now runs in every API pod. With api.replicaCount > 1 or autoscaling.enabled, multiple replicas will run the migration concurrently (the old Job ran it once). This is safe only if the migration framework serializes via a DB advisory lock (golang-migrate does, but not every framework does). Please confirm the migration tooling is concurrency-safe, or gate the migration to a single replica.

HussainTechSavvy · 2026-09-08T09:12:19Z

+        tag: "1.36"
+        pullPolicy: IfNotPresent
+    resources: {}
+  #  Database migration job that runs "jovvix migrate up" before the API starts.


SUGGESTION: This comment still reads "Database migration job that runs ...", but the migration is now an initContainer in the API Deployment, not a Kubernetes Job. Please update the wording to avoid misleading users (e.g. "migration initContainer").

HussainTechSavvy · 2026-09-08T09:12:19Z

+  image:
+    repository: improwised/jovvix-api
+    #  Required. Find available tags at: https://hub.docker.com/r/improwised/jovvix-api/tags
+    tag: ""


WARNING: api.image.tag (and the same pattern for api.migration.image.tag and ui.image.tag) defaults to "" but the templates render {{ .Values.api.image.tag | default .Chart.AppVersion }}. Since Chart.yaml sets appVersion: "1.0.0" (a placeholder), an empty tag resolves to improwised/jovvix-api:1.0.0, which likely does not exist (published tags are main-<sha> style, per ci-values.yaml). A default install would hit ImagePullBackOff. The # Required comment implies the tag must be set, but there is no required guard to enforce it. Either wrap the tag in required "...", or set appVersion to a real published tag and document that empty falls back to it.

…tion - Gate migration initContainer on api.migration.enabled alone, independent of api.initContainers.enabled, so migration still runs when wait-for-* containers are disabled (external DB/Redis) - Add retry loop to migration command for concurrency safety with replicaCount > 1 (relies on golang-migrate advisory locks) - Update stale migration job comment to migration initContainer - Replace default .Chart.AppVersion with required for api, migration, and ui image tags to prevent silent ImagePullBackOff on placeholder appVersion 1.0.0

HussainTechSavvy

Code Review — Jovvix Helm Chart (Round 4)

Verdict: Changes Requested

Round-3 items — all resolved

✅ Migration gating: initContainers block now or .Values.api.initContainers.enabled .Values.api.migration.enabled, with wait-for-* and migration gated independently.
✅ Concurrency: added until ./jovvix migrate up ... retry loop.
✅ Stale "migration job" comment corrected.
✅ Image tag required validation replaces the silent appVersion fallback.

CI is green on all four lint-test jobs.

New issues

README/install docs not updated for the now-required image tags (see inline)
Unbounded migration retry loop (see inline)

See inline comments for details.

HussainTechSavvy · 2026-09-09T11:32:30Z

+          command: ["/bin/sh", "-c"]
+          args:
+            - >-
+              until ./jovvix migrate up; do echo "migration failed, retrying..."; sleep 5; done


WARNING: until ./jovvix migrate up; do echo "migration failed, retrying..."; sleep 5; done retries indefinitely with no maximum attempt count. On a genuine migration failure (bad SQL, missing object — not just lock contention from a concurrent replica), the initContainer never exits non-zero, so the API pod hangs in Init forever and the error only surfaces in logs. The old Job had backoffLimit: 3 which surfaced persistent failures. Suggest bounding the retries (e.g. a max-attempt counter that exits non-zero after N failures) so real failures fail fast rather than hang.

HussainTechSavvy · 2026-09-09T11:32:30Z

+            {{- toYaml . | nindent 12 }}
+          {{- end }}
+        {{- end }}
+        {{- if .Values.api.migration.enabled }}


WARNING: api.migration.image.tag (and api.image.tag on line 101, plus ui.image.tag) is now required, but values.yaml still defaults all three to "", and the README's helm install command and "Required Secrets" table do not mention them. A user following the README verbatim will now fail at render time with "api.migration.image.tag must be set" (and the same for api/ui tags). Please update the README install examples and add these tags to the required-values list, or otherwise document that they must be supplied via --set.

drashti-itpl requested a review from Luv7804 August 11, 2026 07:53

drashti-itpl self-assigned this Aug 11, 2026

drashti-itpl requested review from ChandanTeekinavar and HussainTechSavvy August 11, 2026 07:53

drashti-itpl added 5 commits August 11, 2026 13:34

fix(jovvix): register valkey and ory helm repos for ct lint

8c17414

fix(jovvix): make helm chart pass ct lint

95c8acf

fix(jovvix): make helm chart pass ct lint

c4b00a0

fix: fixed the linting

99bd016

fix: fix(jovvix): shorten kratos secretsCipher to 32-char limit in ci…

5b6dca7

…-values

HussainTechSavvy reviewed Aug 17, 2026

View reviewed changes

Comment thread charts/jovvix-chart/ci/ci-values.yaml

Comment thread charts/jovvix-chart/templates/kratos-secret.yaml

Comment thread charts/jovvix-chart/values.yaml

Comment thread charts/jovvix-chart/values.yaml

Comment thread charts/jovvix-chart/templates/kratos-httproute.yaml

drashti-itpl added 9 commits August 18, 2026 15:05

fix(jovvix): remove unused username key from db secret

0c62700

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.

fix(kratos): default development mode and sensitive log leaking to false

72265ac

Set kratos.development, kratos.kratos.development, and leak_sensitive_values to false for secure defaults in a publishable chart.

refactor(_helpers): remove unused ingress backend and pathtype helpers

9870c6f

Remove jovvix.ingress.backend and jovvix.ingress.pathtype defines from _helpers.tpl. Neither is referenced by any template after the ingress.yaml rewrite.

fix(postgres): make terminationGracePeriodSeconds configurable

c6735c3

Increase default from 30s to 120s for PostgreSQL to allow in-flight transactions to complete during shutdown. Value is now configurable via postgres.terminationGracePeriodSeconds in values.yaml.

fix(migration): render resources on wait-for-db initContainer

7877140

drashti-itpl requested a review from HussainTechSavvy September 1, 2026 05:27

HussainTechSavvy requested changes Sep 1, 2026

View reviewed changes

drashti-itpl added 4 commits September 1, 2026 13:15

fix(values): default API to production mode

2c3c1bd

Set debug and isDevelopment to false so the API does not run in development/debug mode by default. Production deployments should not leak sensitive values or bypass HTTPS enforcement.

fix(api): include port in Kratos admin base URL

1b0c7a5

SERVE_ADMIN_BASE_URL was missing the serveAdminPort suffix, making it inconsistent with SERVE_PUBLIC_BASE_URL. Add the port so both URLs are formed the same way.

fix(values): remove unused global.databaseHost config

7e6fafd

global.databaseHost was never referenced by any template; the jovvix.databaseHost helper uses database.host instead. Drop the dead value to avoid confusion.

drashti-itpl requested a review from HussainTechSavvy September 1, 2026 08:20

HussainTechSavvy requested changes Sep 1, 2026

View reviewed changes

drashti-itpl requested a review from HussainTechSavvy September 1, 2026 11:15

HussainTechSavvy requested changes Sep 8, 2026

View reviewed changes

drashti-itpl requested a review from HussainTechSavvy September 9, 2026 11:08

HussainTechSavvy requested changes Sep 9, 2026

View reviewed changes

Conversation

drashti-itpl commented Aug 11, 2026

jovvix-chart

Fixes Issue

Changes proposed

Check List (Check all the applicable boxes)

Screenshots

Note to reviewers

Uh oh!

HussainTechSavvy commented Aug 17, 2026

Uh oh!

HussainTechSavvy left a comment

Choose a reason for hiding this comment

Code Review Summary

Critical

Warnings

Suggestions

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

HussainTechSavvy left a comment

Choose a reason for hiding this comment

Code Review — Jovvix Helm Chart

Warnings

Suggestions

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

HussainTechSavvy left a comment

Choose a reason for hiding this comment

Code Review — Jovvix Helm Chart (Round 2)

Previous review items — resolved

Remaining outstanding item

New finds from the full fresh review

Uh oh!

HussainTechSavvy Sep 1, 2026

Choose a reason for hiding this comment

Uh oh!

HussainTechSavvy left a comment

Choose a reason for hiding this comment

Code Review — Jovvix Helm Chart (Round 3)

Previous items — resolved

New issues from the migration-as-initContainer change

Still outstanding from earlier rounds

Uh oh!

HussainTechSavvy Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

HussainTechSavvy Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

HussainTechSavvy Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

HussainTechSavvy Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

HussainTechSavvy left a comment

Choose a reason for hiding this comment

Code Review — Jovvix Helm Chart (Round 4)

Round-3 items — all resolved

New issues

Uh oh!

HussainTechSavvy Sep 9, 2026

Choose a reason for hiding this comment

Uh oh!

HussainTechSavvy Sep 9, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone