feat(charts): add streaming-hub chart - #1712
Open
guimoreirar wants to merge 6 commits into
Open
Conversation
New chart promoted from develop as its own PR to main (no changes to other charts).
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces a complete ChangesStreaming Hub chart
Sequence Diagram(s)sequenceDiagram
participant ArgoCD
participant BootstrapPostgresJob
participant PostgreSQL
participant MigrationSecret
participant MigrationsJob
participant StreamingHubDeployment
ArgoCD->>BootstrapPostgresJob: Run PostgreSQL bootstrap hook
BootstrapPostgresJob->>PostgreSQL: Create database, role, and privileges
ArgoCD->>MigrationSecret: Create migration DSN Secret
ArgoCD->>MigrationsJob: Run schema migration hook
MigrationsJob->>PostgreSQL: Apply schema migrations
ArgoCD->>StreamingHubDeployment: Deploy streaming-hub workloads
Possibly related PRs
✨ Finishing Touches✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@charts/streaming-hub/README.md`:
- Around line 152-153: Update the stale template reference from
templates/secret.yaml to templates/secrets.yaml in
charts/streaming-hub/README.md lines 152-153 and
charts/streaming-hub/docs/TOPOLOGY.md lines 29-30; no other documentation
changes are needed.
- Around line 163-179: Update the “External dependencies” section in the README
to document the required pre-existing ghcr-credential image pull secret
referenced by streamingHub.imagePullSecrets. State that it must exist in the
release namespace because the chart does not create it, alongside the existing
Kafka, PostgreSQL, and OTEL dependency notes.
- Around line 56-63: Correct the load-bearing “DOUBLE-CONSUME HAZARD” callout in
the README by changing “All three roles” to accurately describe only the `all`
and `ingest` roles joining the ingest consumer group; keep the warning and
deployment guidance unchanged.
In `@charts/streaming-hub/templates/_deployment.tpl`:
- Around line 89-94: Update the extraEnvVars rendering block in the deployment
template to reject entries named STREAMING_HUB_ROLE,
STREAMING_HUB_POSTGRES_MAX_OPEN_CONNS, or STREAMING_HUB_POSTGRES_MAX_IDLE_CONNS
before emitting them. Make Helm rendering fail for any reserved name, while
continuing to render all other extra environment variables unchanged.
In `@charts/streaming-hub/templates/_helpers.tpl`:
- Around line 44-46: Update the streaming-hub.componentFullname helper to
truncate the base fullname before appending the component suffix, reserving
enough characters for the hyphen and component so suffixes such as ingest and
delivery are retained within the 63-character limit. Preserve the existing
fullname source and trailing-hyphen cleanup.
- Around line 13-15: Update the streaming-hub.fullname helper so its default
naming path incorporates .Release.Name alongside the chart name, while
preserving fullnameOverride behavior and the 63-character truncation and
trailing-hyphen trimming. Ensure generated resource names are unique for
multiple releases in the same namespace.
In `@charts/streaming-hub/templates/bootstrap-postgres.yaml`:
- Around line 78-104: Update the bootstrap PostgreSQL credential handling around
DB_USER_ADMIN, DB_ADMIN_PASSWORD, and DB_PASSWORD_HUB so passwords are never
rendered through literal value fields. Mirror the migration-secret.yaml pattern
by materializing a Secret from configured credentials when no existing secret is
supplied, then reference the resulting Secret with secretKeyRef unconditionally
while preserving existing-secret support.
- Around line 39-165: Harden and align the PostgreSQL bootstrap Job with the
sibling migrations Job by adding the same pod/container securityContext
settings, disabling automountServiceAccountToken, and configuring container
resources. Also propagate the chart’s nodeSelector, affinity, tolerations, and
imagePullSecrets so this hook shares the application and migrations scheduling
behavior; update the Job spec and psql container while preserving the existing
bootstrap logic.
- Around line 33-40: Add timeout protection to the bootstrap Job in the
manifest: set an appropriate activeDeadlineSeconds and configure PostgreSQL
connection and statement timeouts for the main container’s psql calls, covering
migration-secret, migrations, and rollout operations. Keep the existing TCP
reachability wait and restart behavior unchanged while ensuring the PreSync hook
cannot hang indefinitely.
- Around line 111-161: Update the bootstrap SQL setup around sql_lit and the
CREATE/GRANT/ALTER statements to safely escape double quotes in database and
role identifiers, defining DB_NAME_IDENT and DB_ROLE_IDENT for that purpose.
Replace raw DB_NAME and DB_ROLE interpolations inside quoted identifiers in
CREATE ROLE, CREATE DATABASE, and all privilege statements with the escaped
identifier variables, while leaving SQL string-literal escaping for the password
and existence checks unchanged.
- Around line 134-152: Update the existing-role branch in the bootstrap script
to reconcile the role password on every run, using ALTER ROLE for DB_ROLE with
the current DB_PASSWORD_HUB_LIT value before continuing. Preserve the existing
create-role behavior for missing roles and the database creation logic, while
ensuring reruns apply rotated credentials.
In `@charts/streaming-hub/templates/migration-secret.yaml`:
- Around line 1-23: Consolidate the duplicated migration flag parsing and
Secret-name construction used by this template and migrations.yaml into shared
helper definitions, then update both templates to reuse those helpers. Replace
the local $migrationsEnabled/$migUseExisting expressions and inline migration
Secret name in migration-secret.yaml with the shared symbols, preserving the
existing enablement conditions and generated name.
In `@charts/streaming-hub/templates/migrations.yaml`:
- Around line 1-24: Centralize the migrations enabled/useExistingSecret parsing
and migration Secret-name computation currently duplicated between
migrations.yaml and migration-secret.yaml. Add or reuse shared template helpers,
then update both templates to call those helpers while preserving the existing
required-secret validation and generated-name behavior.
- Around line 27-33: Update the migration image tag resolution around
$migrationTag to call the existing streaming-hub.defaultTag helper when both an
explicit tag and digest are absent, instead of duplicating the
streamingHub.image.tag and .Chart.AppVersion fallback logic. Preserve explicit
tag and digest precedence.
In `@charts/streaming-hub/values.schema.json`:
- Around line 19-41: The streamingHub schema should validate the pairing of
useExistingSecret and existingSecretName before template rendering. Update the
relevant streamingHub secrets schema definition to require a non-empty
existingSecretName whenever useExistingSecret is true, while preserving valid
configurations that do not use an existing secret.
In `@charts/streaming-hub/values.yaml`:
- Around line 434-436: Remove the unused top-level streaming-hub configuration
block, including its hardcoded image tag. Keep the existing streamingHub
camelCase values and documented empty-tag fallback unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6de7a63b-972c-40ca-879c-a8a879974faa
📒 Files selected for processing (28)
charts/streaming-hub/.helmignorecharts/streaming-hub/Chart.yamlcharts/streaming-hub/README.mdcharts/streaming-hub/docs/TOPOLOGY.mdcharts/streaming-hub/templates/_deployment.tplcharts/streaming-hub/templates/_helpers.tplcharts/streaming-hub/templates/all/deployment.yamlcharts/streaming-hub/templates/all/hpa.yamlcharts/streaming-hub/templates/all/pdb.yamlcharts/streaming-hub/templates/all/service.yamlcharts/streaming-hub/templates/bootstrap-postgres.yamlcharts/streaming-hub/templates/configmap.yamlcharts/streaming-hub/templates/delivery/deployment.yamlcharts/streaming-hub/templates/delivery/hpa.yamlcharts/streaming-hub/templates/delivery/pdb.yamlcharts/streaming-hub/templates/delivery/service.yamlcharts/streaming-hub/templates/ingest/deployment.yamlcharts/streaming-hub/templates/ingest/hpa.yamlcharts/streaming-hub/templates/ingest/pdb.yamlcharts/streaming-hub/templates/ingest/service.yamlcharts/streaming-hub/templates/ingress.yamlcharts/streaming-hub/templates/migration-secret.yamlcharts/streaming-hub/templates/migrations.yamlcharts/streaming-hub/templates/secrets.yamlcharts/streaming-hub/templates/serviceaccount.yamlcharts/streaming-hub/values-template.yamlcharts/streaming-hub/values.schema.jsoncharts/streaming-hub/values.yaml
Comment on lines
+89
to
+94
| {{- with $sh.extraEnvVars }} | ||
| {{- range . }} | ||
| - name: {{ .name }} | ||
| value: {{ .value | quote }} | ||
| {{- end }} | ||
| {{- end }} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the template and nearby values first.
ast-grep outline charts/streaming-hub/templates/_deployment.tpl --view expanded || true
printf '\n---\n'
ast-grep outline charts/streaming-hub/values.yaml --view expanded || true
printf '\n---\n'
rg -n "extraEnvVars|STREAMING_HUB_ROLE|POSTGRES_MAX_(OPEN|IDLE)_CONNS|envFrom|env:" charts/streaming-hub -SRepository: LerianStudio/helm
Length of output: 3427
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,130p' charts/streaming-hub/templates/_deployment.tpl
printf '\n--- values excerpt ---\n'
sed -n '160,210p' charts/streaming-hub/values.yaml
printf '\n--- docs excerpt ---\n'
sed -n '30,140p' charts/streaming-hub/docs/TOPOLOGY.md
printf '\n--- README excerpt ---\n'
sed -n '1,140p' charts/streaming-hub/README.mdRepository: LerianStudio/helm
Length of output: 17876
🌐 Web query:
Kubernetes duplicate env var names container env list last wins duplicate name behavior docs
💡 Result:
In Kubernetes, when duplicate environment variable names are present in a container's environment configuration, the following behavioral rules apply: 1. Explicitly Defined Env Precedence: Environment variables defined directly in the env list always take precedence over those defined via envFrom (ConfigMaps or Secrets) [1][2][3]. 2. envFrom Precedence: When using envFrom, if multiple sources (e.g., multiple ConfigMaps or Secrets) define the same key, the value from the last source in the list takes precedence [1][4][5]. 3. Duplicate Names in the env List: While Kubernetes does not explicitly forbid providing a list with duplicate name entries in a YAML configuration, doing so is not recommended and can lead to unpredictable behavior during updates [6][7]. Specifically: - When using kubectl apply (client-side), having duplicate entries can cause issues where modifying or removing one instance of a duplicate results in all instances of that variable being removed from the resulting object, effectively deleting the environment variable entirely [6][7]. - While some observed behaviors suggest the last entry in the env list "wins" (is the one used by the container) [8], relying on this is fragile and can conflict with how Kubernetes internal API patch operations handle keyed lists [7]. - Server-side apply handles these cases more gracefully than client-side apply, but the best practice remains to avoid duplicate names entirely [7]. It is highly recommended to ensure all environment variable names are unique within a container's definition to avoid configuration drift, silent failures, and issues with resource updates [7][5]. Use the env section for explicit overrides and envFrom for bulk settings, keeping the order of envFrom sources in mind if overlaps are unavoidable [3][5].
Citations:
- 1: https://stackoverflow.com/questions/66288565/duplicated-env-variable-names-in-pod-definition-what-is-the-precedence-rule-to
- 2: https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.30/
- 3: https://dataplexa.com/5721-kubernetes-lesson-21-environment-variables-inkubernetes/
- 4: https://stackoverflow.com/questions/73033285/configmap-and-secret-priority
- 5: https://aayushtuladhar.com/devops/Debugging_ExternalSecrets.html
- 6: Removing a duplicate environment variable from the container spec ends up deleting the environment var entirely kubernetes/kubernetes#106809
- 7: Env variable is missing in container, after cleanup of duplications in deployment kubernetes/kubernetes#121541
- 8: https://stackoverflow.com/questions/73443818/precedence-rule-in-container-env-list-with-duplicate-name-key-in-kubernetes
Reject reserved env names in extraEnvVars. extraEnvVars is appended after the role/pool env entries, so a duplicate STREAMING_HUB_ROLE, STREAMING_HUB_POSTGRES_MAX_OPEN_CONNS, or STREAMING_HUB_POSTGRES_MAX_IDLE_CONNS can shadow the topology-specific values. Fail rendering for those names; this list is documented as non-sensitive, and relying on duplicate env behavior is fragile.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@charts/streaming-hub/templates/_deployment.tpl` around lines 89 - 94, Update
the extraEnvVars rendering block in the deployment template to reject entries
named STREAMING_HUB_ROLE, STREAMING_HUB_POSTGRES_MAX_OPEN_CONNS, or
STREAMING_HUB_POSTGRES_MAX_IDLE_CONNS before emitting them. Make Helm rendering
fail for any reserved name, while continuing to render all other extra
environment variables unchanged.
Comment on lines
+13
to
+15
| {{- define "streaming-hub.fullname" -}} | ||
| {{- default (include "streaming-hub.name" .) .Values.fullnameOverride | trunc 63 | trimSuffix "-" }} | ||
| {{- end }} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the helper template and nearby usage sites.
git ls-files 'charts/streaming-hub/templates/_helpers.tpl' 'charts/streaming-hub/templates/*' | sed -n '1,200p'
echo '--- _helpers.tpl ---'
cat -n charts/streaming-hub/templates/_helpers.tpl | sed -n '1,220p'
echo '--- fullname references ---'
rg -n '"streaming-hub\.fullname"|include "streaming-hub\.fullname"|fullname' charts/streaming-hub/templates -g '!**/charts/**' -A2 -B2Repository: LerianStudio/helm
Length of output: 6858
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show all places where the fullname helper is used and how resource names are formed.
rg -n 'include "streaming-hub\.(fullname|componentFullname|serviceAccountName|secretName)"|name: \{\{ include "streaming-hub\.(fullname|componentFullname)' charts/streaming-hub/templates -A3 -B3Repository: LerianStudio/helm
Length of output: 12476
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether the chart documents single-release-only assumptions or name overrides.
rg -n 'fullnameOverride|nameOverride|one release|single release|same namespace|coexist|namespaceOverride|streaming-hub.fullname' charts/streaming-hub README* -A2 -B2Repository: LerianStudio/helm
Length of output: 8383
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the relevant README and values docs for release-scope expectations.
sed -n '48,75p' charts/streaming-hub/README.md
echo '---'
sed -n '1,35p' charts/streaming-hub/values.yamlRepository: LerianStudio/helm
Length of output: 2949
Make the default fullname release-scoped
charts/streaming-hub/templates/_helpers.tpl:13-15 ignores .Release.Name, so two installs of this chart into the same namespace render the same ConfigMap, Secret, ServiceAccount, Ingress, and workload names. Include the release name in the default path, or add an explicit one-release-per-namespace guard/documentation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@charts/streaming-hub/templates/_helpers.tpl` around lines 13 - 15, Update the
streaming-hub.fullname helper so its default naming path incorporates
.Release.Name alongside the chart name, while preserving fullnameOverride
behavior and the 63-character truncation and trailing-hyphen trimming. Ensure
generated resource names are unique for multiple releases in the same namespace.
Comment on lines
+1
to
+23
| {{- $streamingHub := .Values.streamingHub | default dict }} | ||
| {{- $migrations := get $streamingHub "migrations" | default dict }} | ||
| {{- $migrationsEnabled := eq (lower (toString (get $migrations "enabled" | default false))) "true" }} | ||
| {{- $migUseExisting := eq (lower (toString (get $migrations "useExistingSecret" | default false))) "true" }} | ||
| {{- /* | ||
| Minimal, migration-only Secret carrying ONLY STREAMING_HUB_POSTGRES_DSN. | ||
|
|
||
| Rendered ONLY when migrations are enabled AND a chart-managed credential is used | ||
| (migrations.useExistingSecret=false). In that case the migration Job runs as a | ||
| pre-install/pre-upgrade hook (weight -1) BEFORE the normal application Secret | ||
| (secrets.yaml, a normal main-sync resource) exists, so it needs its own credential | ||
| available early. This Secret is a PreSync hook at weight -5 / sync-wave -5 — after | ||
| bootstrap-postgres (-10), before the migration Job (-1). | ||
|
|
||
| It holds ONLY the DSN; the full application Secret stays a NORMAL (non-hook) | ||
| resource so its sensitive runtime keys are never left behind as orphaned hooks on | ||
| uninstall. | ||
| */}} | ||
| {{- if and $migrationsEnabled (not $migUseExisting) }} | ||
| apiVersion: v1 | ||
| kind: Secret | ||
| metadata: | ||
| name: {{ printf "%s-migrations" (include "streaming-hub.fullname" .) | trunc 63 | trimSuffix "-" }} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Duplicated enabled/useExistingSecret parsing and Secret-name computation vs migrations.yaml.
Lines 1-4 recompute the same $migrationsEnabled/$migUseExisting expressions as migrations.yaml (lines 5, 19), and line 23's name formula duplicates migrations.yaml line 24. If either copy drifts, the migrations Job could look up a Secret name that doesn't match what this template creates. See consolidated comment for a shared-helper fix spanning both files.
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@charts/streaming-hub/templates/migration-secret.yaml` around lines 1 - 23,
Consolidate the duplicated migration flag parsing and Secret-name construction
used by this template and migrations.yaml into shared helper definitions, then
update both templates to reuse those helpers. Replace the local
$migrationsEnabled/$migUseExisting expressions and inline migration Secret name
in migration-secret.yaml with the shared symbols, preserving the existing
enablement conditions and generated name.
Comment on lines
+1
to
+24
| {{- $streamingHub := .Values.streamingHub | default dict }} | ||
| {{- $migrations := get $streamingHub "migrations" | default dict }} | ||
| {{- $migrationImage := get $migrations "image" | default dict }} | ||
| {{- $defaultMigrationResources := dict "requests" (dict "cpu" "50m" "memory" "64Mi") "limits" (dict "cpu" "250m" "memory" "256Mi") }} | ||
| {{- $migrationsEnabled := eq (lower (toString (get $migrations "enabled" | default false))) "true" }} | ||
| {{- if $migrationsEnabled }} | ||
| {{- /* | ||
| OUT-OF-BAND schema migration Job. The hub never migrates itself; this Job applies | ||
| the SQL migrations BEFORE the app rolls out. PreSync hook at weight -1 — after | ||
| bootstrap-postgres (-10, creates role+db) and migration-secret (-5, carries the | ||
| DSN), before the app Deployment (main Sync). Runs ONCE regardless of | ||
| streamingHub.mode (all | split) — it is not per-role. | ||
|
|
||
| The runner is the stock migrate/migrate toolchain baked with the hub's migrations/ | ||
| dir; its entrypoint runs `migrate -path=/migrations -database="$STREAMING_HUB_POSTGRES_DSN" up`, | ||
| so the ONLY required env is STREAMING_HUB_POSTGRES_DSN (secretKeyRef). | ||
| */}} | ||
| {{- /* Secret resolution — STREAMING_HUB_POSTGRES_DSN source. */}} | ||
| {{- $migUseExisting := eq (lower (toString (get $migrations "useExistingSecret" | default false))) "true" }} | ||
| {{- $dsnSecretName := "" }} | ||
| {{- if $migUseExisting }} | ||
| {{- $dsnSecretName = required "streamingHub.migrations.existingSecretName is required when streamingHub.migrations.useExistingSecret=true" (get $migrations "existingSecretName") }} | ||
| {{- else }} | ||
| {{- $dsnSecretName = printf "%s-migrations" (include "streaming-hub.fullname" .) | trunc 63 | trimSuffix "-" }} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Duplicated enabled/useExistingSecret parsing and Secret-name computation vs migration-secret.yaml.
Lines 1-5 and 19 recompute the same boolean-parsing expressions as migration-secret.yaml (lines 1-4), and line 24's name formula duplicates migration-secret.yaml line 23. See consolidated comment for a shared-helper fix spanning both files.
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@charts/streaming-hub/templates/migrations.yaml` around lines 1 - 24,
Centralize the migrations enabled/useExistingSecret parsing and migration
Secret-name computation currently duplicated between migrations.yaml and
migration-secret.yaml. Add or reuse shared template helpers, then update both
templates to call those helpers while preserving the existing required-secret
validation and generated-name behavior.
Comment on lines
+19
to
+41
| "streamingHub": { | ||
| "type": "object", | ||
| "additionalProperties": true, | ||
| "properties": { | ||
| "mode": { | ||
| "type": "string", | ||
| "enum": ["all", "split"], | ||
| "description": "Topology switch. 'all' = one Deployment (role=all); 'split' = ingest + delivery. NEVER run both against one Kafka cluster (double-consume)." | ||
| }, | ||
| "common": { | ||
| "type": "object", | ||
| "additionalProperties": true, | ||
| "properties": { | ||
| "configmap": { | ||
| "type": "object" | ||
| } | ||
| } | ||
| }, | ||
| "secrets": { | ||
| "type": "object" | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Schema enforces mode enum but not the equally critical useExistingSecret/existingSecretName pairing.
Templates use required on existingSecretName when useExistingSecret: true, failing only at render time with a generic error. A conditional schema rule would surface this earlier via helm lint.
♻️ Proposed schema addition
"streamingHub": {
"type": "object",
"additionalProperties": true,
+ "allOf": [
+ {
+ "if": {
+ "properties": { "useExistingSecret": { "const": true } },
+ "required": ["useExistingSecret"]
+ },
+ "then": {
+ "properties": { "existingSecretName": { "type": "string", "minLength": 1 } },
+ "required": ["existingSecretName"]
+ }
+ }
+ ],
"properties": {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "streamingHub": { | |
| "type": "object", | |
| "additionalProperties": true, | |
| "properties": { | |
| "mode": { | |
| "type": "string", | |
| "enum": ["all", "split"], | |
| "description": "Topology switch. 'all' = one Deployment (role=all); 'split' = ingest + delivery. NEVER run both against one Kafka cluster (double-consume)." | |
| }, | |
| "common": { | |
| "type": "object", | |
| "additionalProperties": true, | |
| "properties": { | |
| "configmap": { | |
| "type": "object" | |
| } | |
| } | |
| }, | |
| "secrets": { | |
| "type": "object" | |
| } | |
| } | |
| } | |
| "streamingHub": { | |
| "type": "object", | |
| "additionalProperties": true, | |
| "allOf": [ | |
| { | |
| "if": { | |
| "properties": { "useExistingSecret": { "const": true } }, | |
| "required": ["useExistingSecret"] | |
| }, | |
| "then": { | |
| "properties": { "existingSecretName": { "type": "string", "minLength": 1 } }, | |
| "required": ["existingSecretName"] | |
| } | |
| } | |
| ], | |
| "properties": { | |
| "mode": { | |
| "type": "string", | |
| "enum": ["all", "split"], | |
| "description": "Topology switch. 'all' = one Deployment (role=all); 'split' = ingest + delivery. NEVER run both against one Kafka cluster (double-consume)." | |
| }, | |
| "common": { | |
| "type": "object", | |
| "additionalProperties": true, | |
| "properties": { | |
| "configmap": { | |
| "type": "object" | |
| } | |
| } | |
| }, | |
| "secrets": { | |
| "type": "object" | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@charts/streaming-hub/values.schema.json` around lines 19 - 41, The
streamingHub schema should validate the pairing of useExistingSecret and
existingSecretName before template rendering. Update the relevant streamingHub
secrets schema definition to require a non-empty existingSecretName whenever
useExistingSecret is true, while preserving valid configurations that do not use
an existing secret.
Comment on lines
+434
to
+436
| streaming-hub: | ||
| image: | ||
| tag: 1.0.1 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Stray unused streaming-hub: key with a hardcoded tag override.
This top-level key uses kebab-case (streaming-hub) while the entire chart contract uses camelCase (streamingHub). No template references .Values.streaming-hub, so this block is dead and has zero effect on rendering — but it does ship a misleading tag: 1.0.1 default that contradicts Chart.yaml's appVersion: "1.0.0" and the documented empty-tag fallback behavior (line 74).
🐛 Proposed fix
nodeSelector: {}
tolerations: {}
affinity: {}
-streaming-hub:
- image:
- tag: 1.0.1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| streaming-hub: | |
| image: | |
| tag: 1.0.1 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@charts/streaming-hub/values.yaml` around lines 434 - 436, Remove the unused
top-level streaming-hub configuration block, including its hardcoded image tag.
Keep the existing streamingHub camelCase values and documented empty-tag
fallback unchanged.
Route every non-sensitive env key through lerian-common.cfgValue into typed, schema-validated grouped blocks (precedence: common.configmap.<KEY> escape hatch > streamingHub.<group>.<field> > chart default). Defaults live in the template; the grouped blocks ship empty. Existing keys render byte-identical (0 value changes); 28 real .env-contract gaps closed (dispatch, pull, idempotency, autodisable, manifest, kafka TLS/SCRAM-user/CA, kek ref, aws, security opt-ins, swagger, health window, bare MULTI_TENANT_ENABLED, OTEL_INSECURE_EXPORTER). - PLUGIN_AUTH_ENABLED/ADDRESS resolve via global.auth (globalValue) so the env-wide auth endpoint is shared across services. - OTEL_* stay as cfgValue (observability group): the app uses a chart-level telemetry toggle + downward-API endpoint override, not lib-observability's ENABLE_TELEMETRY contract, so the otel.env helper does not apply. - KEK_REF and KAFKA_SCRAM_USERNAME move from the Secret to the ConfigMap (non-sensitive: a var-name reference and a username); the SCRAM password and DEV_KEK stay in the Secret. - Add lerian-common-helm 1.4.0 as the sole (library) dependency. - Regenerate values.schema.json (strict grouped blocks, mode enum preserved) and README.params.md. Role-injected per-Deployment vars (STREAMING_HUB_ROLE, Postgres pool sizes) remain out of the shared ConfigMap by design.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@charts/streaming-hub/README.params.md`:
- Around line 10-23: Correct the parameter metadata source or generator so
streamingHub.imagePullSecrets is documented as an array with its list default,
and deployment strategy, security context, probes, common, and migration
resources are documented as objects with accurate defaults instead of string/{}
values. Regenerate README.params.md and verify the affected entries, including
the additional referenced ranges, reflect the chart values schema.
In `@charts/streaming-hub/templates/configmap.yaml`:
- Around line 192-193: Update the extraEnvVars rendering in the ConfigMap
template to iterate over each list item and emit its name and value fields as
ConfigMap entries, matching the object structure used by _deployment.tpl. Remove
the map-style key/value iteration so numeric list indexes are not rendered as
keys and preserve quoting for values.
In `@charts/streaming-hub/values.schema.json`:
- Around line 297-300: Update streamingHub.tolerations, all.tolerations,
ingest.tolerations, and delivery.tolerations in
charts/streaming-hub/values.schema.json at lines 297-300, 850-853, 960-962, and
1068-1071 to accept arrays of objects instead of objects with arbitrary
properties. Change the four corresponding tolerations defaults in
charts/streaming-hub/values.yaml from empty maps to empty arrays.
- Around line 653-662: Update the migrations schema properties around
useExistingSecret and existingSecretName so that enabling an existing Secret
requires a non-empty existingSecretName, adding the conditional validation for
streamingHub.migrations.enabled and streamingHub.migrations.useExistingSecret
while preserving current defaults and descriptions.
In `@charts/streaming-hub/values.yaml`:
- Around line 218-219: Separate the two extraEnvVars contracts: in
charts/streaming-hub/values.yaml lines 218-219, rename the ConfigMap map key or
otherwise give the pod-level list a distinct key, preserving each intended value
shape. In charts/streaming-hub/README.params.md lines 56-59, remove the
duplicate entry and document each retained key with its actual shape.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d2cea381-f3f1-4e78-b07c-62607dcf64c4
⛔ Files ignored due to path filters (1)
charts/streaming-hub/Chart.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
charts/streaming-hub/Chart.yamlcharts/streaming-hub/README.params.mdcharts/streaming-hub/templates/configmap.yamlcharts/streaming-hub/values.schema.jsoncharts/streaming-hub/values.yaml
Comment on lines
+192
to
+193
| {{- range $key, $value := .Values.streamingHub.extraEnvVars }} | ||
| {{ $key }}: {{ $value | quote }} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Render extraEnvVars as list items.
templates/_deployment.tpl treats streamingHub.extraEnvVars as a list of {name, value} objects. This map-style loop renders list indexes such as 0 as ConfigMap keys and serializes the object as the value. Kubelet cannot expose those numeric keys through envFrom.
Proposed fix
- {{- range $key, $value := .Values.streamingHub.extraEnvVars }}
- {{ $key }}: {{ $value | quote }}
+ {{- range .Values.streamingHub.extraEnvVars }}
+ {{ .name }}: {{ .value | quote }}
{{- end }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| {{- range $key, $value := .Values.streamingHub.extraEnvVars }} | |
| {{ $key }}: {{ $value | quote }} | |
| {{- range .Values.streamingHub.extraEnvVars }} | |
| {{ .name }}: {{ .value | quote }} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@charts/streaming-hub/templates/configmap.yaml` around lines 192 - 193, Update
the extraEnvVars rendering in the ConfigMap template to iterate over each list
item and emit its name and value fields as ConfigMap entries, matching the
object structure used by _deployment.tpl. Remove the map-style key/value
iteration so numeric list indexes are not rendered as keys and preserve quoting
for values.
Comment on lines
+218
to
+219
| # -- Unmodeled extra env vars appended verbatim to the ConfigMap. | ||
| extraEnvVars: {} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use separate names for the two extraEnvVars contracts.
Line 219 adds a map-shaped streamingHub.extraEnvVars, but existing chart defaults already use that key as a list. A YAML mapping cannot retain both values. YAMLlint rejects this file, and permissive parsers retain only one shape. The schema models the map shape while the documentation advertises both shapes.
charts/streaming-hub/values.yaml#L218-L219: Rename this ConfigMap map or use a distinct key for the pod-level list.charts/streaming-hub/README.params.md#L56-L59: Remove the duplicate entry and document each retained key with its actual shape.
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 219-219: duplication of key "extraEnvVars" in mapping
(key-duplicates)
📍 Affects 2 files
charts/streaming-hub/values.yaml#L218-L219(this comment)charts/streaming-hub/README.params.md#L56-L59
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@charts/streaming-hub/values.yaml` around lines 218 - 219, Separate the two
extraEnvVars contracts: in charts/streaming-hub/values.yaml lines 218-219,
rename the ConfigMap map key or otherwise give the pod-level list a distinct
key, preserving each intended value shape. In
charts/streaming-hub/README.params.md lines 56-59, remove the duplicate entry
and document each retained key with its actual shape.
Source: Linters/SAST tools
guimoreirar
force-pushed
the
feat/streaming-hub
branch
from
August 4, 2026 20:14
d4e1965 to
e4e220b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@charts/streaming-hub/values.schema.json`:
- Around line 297-299: Require every tolerations array entry to be an object by
adding an items.type object schema at all four tolerations locations:
charts/streaming-hub/values.schema.json lines 297-299, 849-851, 957-959, and
1065-1067.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a459dfb8-56bd-4071-b8a3-61b4709878ce
📒 Files selected for processing (6)
charts/lerian-common/templates/_ingress.tplcharts/streaming-hub/templates/_deployment.tplcharts/streaming-hub/templates/ingress.yamlcharts/streaming-hub/templates/serviceaccount.yamlcharts/streaming-hub/values.schema.jsoncharts/streaming-hub/values.yaml
guimoreirar
force-pushed
the
feat/streaming-hub
branch
from
August 4, 2026 20:27
e4e220b to
9260ca3
Compare
…type
Two operator-input correctness fixes surfaced while validating resource templates:
- values.schema.json closed the per-role blocks (all/ingest/delivery) and their
operational sub-blocks (autoscaling/pdb/resources/...) with additionalProperties:false
and type-locked their scalar leaves, rejecting valid input such as pdb.maxUnavailable
(int-or-string) or resources.limits. Strictness belongs only on the finite cfgValue
config groups (key-typo protection there); k8s operational passthrough stays open.
Config-group typo rejection and the mode enum are preserved.
- The scheduling `tolerations` default was {} (a map), which typed the schema as an
object and rejected the correct list form. Changed to [] at all four sites
(streamingHub + the three roles); empty [] renders identically (with skips it).
…-hub First real consumer of lerian-common's shared resource templates (they shipped dormant — every chart hand-rolled these). Delegate Service/HPA/PDB/ServiceAccount to lerian-common.service/.hpa/.pdb/.serviceAccount, the Ingress to lerian-common.ingress, and the Deployment pod-spec fragments to lerian-common.imagePullSecrets/.httpProbe. Byte-identical render across all three roles (all/ingest/delivery) in both mode=all and mode=split. lerian-common.ingress gains an optional `backendName` (defaults to `name`) so a chart whose Ingress routes to a Service named differently from the Ingress resource can still use it — streaming-hub's backend is the active role's component Service (streaming-hub-<role>), not the chart fullname. Backward-compatible. Kept inline (deliberately): deploymentStrategy (inline toYaml emits keys alphabetically; the lib partial emits type-first) and scheduling (lerian-common.scheduling emits a leading newline that becomes trailing whitespace under `| nindent`).
guimoreirar
force-pushed
the
feat/streaming-hub
branch
from
August 4, 2026 20:28
9260ca3 to
c4a8ecf
Compare
- extraEnvVars: remove duplicate list declaration + broken _deployment.tpl range; single map->ConfigMap passthrough contract (matches lerian-common) - remove stray kebab-case streaming-hub: values key - values.schema.json: require tolerations entries to be objects (4 sites) - _helpers.tpl componentFullname: reserve suffix space before truncating - migrations.yaml: call streaming-hub.defaultTag helper - bootstrap-postgres: activeDeadlineSeconds; escape SQL identifiers; reconcile role password on re-run; securityContext + scheduling parity with migrations Job - README/TOPOLOGY: secret.yaml->secrets.yaml, role wording, ghcr-credential note - regenerate README.params.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
feat(charts): add streaming-hub chart
New chart promoted from develop as its own PR to main (no changes to other charts).