chart(insight): drop L2 subchart bundling (clickhouse/mariadb/redis/redpanda) - #1428
Merged
cyberantonz merged 2 commits intoJun 22, 2026
Conversation
…edpanda) The umbrella previously bundled four L2 infra subcharts, each gated by a `<svc>.deploy` flag. In gitops those are always `deploy: false` (infra runs as separate releases in `insight-infra`, dialed via `<dep>.host`); the `deploy: true` path was used only by the now-retired `dev-up.sh` flow. Make the umbrella a pure app-bundle. - Chart.yaml: remove the clickhouse/mariadb/redis/redpanda dependency blocks; bump version 0.1.84 -> 0.2.0 (breaking chart-API change). - Chart.lock: regenerated — only the 4 file:// app-service subcharts remain. - values.yaml: trim each infra block to wiring fields only (host/port/ protocol/database/username/passwordSecret, redpanda.brokers). Keep clickhouse.initDatabases — it drives the external-CH init Hook Job. Drop deploy/image/persistence/resources/auth/subchart-only fields and stale install-script comment refs. - _helpers.tpl: collapse the host/fqdn/brokers helpers to the external- only path; drop the dead mariadb.deploy drift validator. - NOTES.txt / init-job / secrets / dbt-run comments + README: realign to the external-L2 model (drop bundled-mode docs, Bitnami-legacy section, helmfile relationship). Render against the gitops local values is unchanged vs the Phase 0 baseline except the chart-version label (the bundled subcharts never rendered under deploy:false). helm lint + server-side dry-run pass. Part of constructorfabric#1414. Closes constructorfabric#1417. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
📝 WalkthroughWalkthroughThe ChangesDrop L2 Subchart Bundling — insight umbrella v0.2.0
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/insight/templates/_helpers.tpl`:
- Around line 66-71: The insight.clickhouse.protocol helper template correctly
allows for protocol override, but the insight.clickhouse.url definition still
uses a hardcoded http:// prefix, causing TLS configurations to be ignored.
Update the insight.clickhouse.url template to dynamically compose the URL by
using the insight.clickhouse.protocol helper template instead of hardcoding the
http:// scheme, ensuring that protocol overrides are properly reflected in the
final endpoint URL.
In `@charts/insight/templates/clickhouse-init-svcdbs-job.yaml`:
- Around line 12-15: The comment describing the ClickHouse authentication model
(lines 12-15) currently states that clickhouse.username defaults to `insight`,
but the values contract actually requires an explicit operator-provided value
with no default. Update the comment to clarify that the operator must explicitly
provide the clickhouse.username value when configuring this chart, removing any
mention of a default value to prevent misconfiguration during rollout.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: adf58bf9-5005-4664-af84-5b153c339719
⛔ Files ignored due to path filters (1)
charts/insight/Chart.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
charts/insight/Chart.yamlcharts/insight/README.mdcharts/insight/templates/NOTES.txtcharts/insight/templates/_helpers.tplcharts/insight/templates/clickhouse-init-svcdbs-job.yamlcharts/insight/templates/ingestion/dbt-run.yamlcharts/insight/templates/mariadb-init-svcdbs-job.yamlcharts/insight/templates/secrets.yamlcharts/insight/values.yaml
cyberantonz
commented
Jun 22, 2026
- _helpers.tpl: insight.clickhouse.url now composes the scheme from insight.clickhouse.protocol instead of a hardcoded http:// — so a TLS ClickHouse (clickhouse.protocol: https) renders the correct endpoint. Default protocol is http, so the rendered output is unchanged for the existing (plain-HTTP) case. - clickhouse-init-svcdbs-job.yaml: drop the misleading "default `insight`" from the auth-model comment — clickhouse.username has no default and the operator MUST set it. - values.yaml: point the INFRASTRUCTURE section at CONTRIBUTING.md and the deployment DESIGN spec for the layered L0/L2/L3 model. Part of constructorfabric#1414. Closes constructorfabric#1417. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
Contributor
Author
|
Addressed the review in
Local deploy validation (OrbStack)Brought the full stack up against a local cluster to exercise this chart end-to-end (substituting a locally-packaged
So the de-bundled umbrella deploys as a pure app-bundle and wires correctly to external L2.
|
|
Tip For best results, initiate chat on the files or code changes.
The OrbStack end-to-end validation is particularly reassuring — pre-install hooks provisioning databases against the external L2 before app pods start is exactly the critical path this PR touches, and seeing it work cleanly confirms the de-bundling is sound. Regarding the Redis version pin issue you noted ( 🐇 ✨ If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
charts/insight/templates/clickhouse-init-svcdbs-job.yaml (1)
87-87:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winThe curl ping command hardcodes
http://but should respectclickhouse.protocolfor TLS endpoints.Line 87 uses a hardcoded
http://scheme in the curl ping endpoint, but the PR objectives document that the ClickHouse URL helper now composes the scheme dynamically frominsight.clickhouse.protocolto support TLS instances. If an operator configuresclickhouse.protocol: https, the ping check will still attempthttp://, which will hang or fail silently.Replace the hardcoded scheme with a dynamically composed URL that respects the configured protocol:
- if curl -sS -o /dev/null --fail "http://${CH_HOST}:${CH_PORT}/ping" 2>/dev/null; then + if curl -sS -o /dev/null --fail "{{ include "insight.clickhouse.protocol" . }}://${CH_HOST}:${CH_PORT}/ping" 2>/dev/null; thenAlternatively, if composing the URL directly in the shell command is preferred, export the protocol as an environment variable alongside
CH_HOSTandCH_PORT:- name: CH_PORT value: {{ include "insight.clickhouse.port" . | quote }} + - name: CH_PROTOCOL + value: {{ include "insight.clickhouse.protocol" . | quote }}Then update the curl command to use it:
- if curl -sS -o /dev/null --fail "http://${CH_HOST}:${CH_PORT}/ping" 2>/dev/null; then + if curl -sS -o /dev/null --fail "${CH_PROTOCOL}://${CH_HOST}:${CH_PORT}/ping" 2>/dev/null; then🤖 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/insight/templates/clickhouse-init-svcdbs-job.yaml` at line 87, The curl ping command hardcodes the http:// scheme in the URL, but this should be dynamic based on the configured clickhouse.protocol setting to support HTTPS endpoints. Modify the curl command by either composing the full URL dynamically to respect the configured protocol scheme instead of the hardcoded http://, or export the clickhouse.protocol as an environment variable (similar to how CH_HOST and CH_PORT are exported) and reference that variable in the curl command to construct the proper endpoint URL that respects TLS configuration.
🤖 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.
Outside diff comments:
In `@charts/insight/templates/clickhouse-init-svcdbs-job.yaml`:
- Line 87: The curl ping command hardcodes the http:// scheme in the URL, but
this should be dynamic based on the configured clickhouse.protocol setting to
support HTTPS endpoints. Modify the curl command by either composing the full
URL dynamically to respect the configured protocol scheme instead of the
hardcoded http://, or export the clickhouse.protocol as an environment variable
(similar to how CH_HOST and CH_PORT are exported) and reference that variable in
the curl command to construct the proper endpoint URL that respects TLS
configuration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f21ad260-a005-49ce-85d4-dd4536aa26bf
📒 Files selected for processing (3)
charts/insight/templates/_helpers.tplcharts/insight/templates/clickhouse-init-svcdbs-job.yamlcharts/insight/values.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- charts/insight/templates/_helpers.tpl
- charts/insight/values.yaml
dzarlax
approved these changes
Jun 22, 2026
cyberantonz
added a commit
to cyberantonz/insight
that referenced
this pull request
Jun 24, 2026
…ook Job Since constructorfabric#1428 dropped the bundled ClickHouse StatefulSet (CH is now always external L2), nothing applied the gold-view migrations: the host init.sh path `kubectl exec`'d into a pod that no longer exists, and the clickhouse-init-svcdbs Hook only runs CREATE DATABASE. Both clusters lost their migrations. Add a `clickhouse-migrate` Helm Hook Job (post-install,post-upgrade) that runs apply-ch-migrations.sh in the toolbox image (which already bundles the SQL) and dials the external ClickHouse over its HTTP interface — no new image/CI needed. It creates the staging/silver/app databases + ADR-0007 placeholders, then applies scripts/migrations/*.sql in glob order. A failed migration fails (and, under gitops --rollback-on-failure, rolls back) the release — intentional; backoffLimit is low since SQL errors are deterministic. - lib/ch-exec.sh: HTTP-only run_ch/ch_table_exists (password via the X-ClickHouse-Key header, never on the command line). - create-bronze-placeholders.sh: sourced from the lib; DDL untouched. - init.sh / run-init.sh: strip the now-dead ClickHouse migration section (the kubectl-exec-into-bundled-CH path) — it targeted a retired StatefulSet. - values.yaml: clickhouse.runMigrations gate (default true). - DESIGN.md: realign the schema-migrations + DB-provisioning + password rotation docs to the external-L2 / Hook-Job model. - e2e migration_applier.py: repoint stale comments (parser logic unchanged). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
cyberantonz
added a commit
that referenced
this pull request
Jun 25, 2026
…ook Job (#1480) Since #1428 dropped the bundled ClickHouse StatefulSet (CH is now always external L2), nothing applied the gold-view migrations: the host init.sh path `kubectl exec`'d into a pod that no longer exists, and the clickhouse-init-svcdbs Hook only runs CREATE DATABASE. Both clusters lost their migrations. Add a `clickhouse-migrate` Helm Hook Job (post-install,post-upgrade) that runs apply-ch-migrations.sh in the toolbox image (which already bundles the SQL) and dials the external ClickHouse over its HTTP interface — no new image/CI needed. It creates the staging/silver/app databases + ADR-0007 placeholders, then applies scripts/migrations/*.sql in glob order. A failed migration fails (and, under gitops --rollback-on-failure, rolls back) the release — intentional; backoffLimit is low since SQL errors are deterministic. - lib/ch-exec.sh: HTTP-only run_ch/ch_table_exists (password via the X-ClickHouse-Key header, never on the command line). - create-bronze-placeholders.sh: sourced from the lib; DDL untouched. - init.sh / run-init.sh: strip the now-dead ClickHouse migration section (the kubectl-exec-into-bundled-CH path) — it targeted a retired StatefulSet. - values.yaml: clickhouse.runMigrations gate (default true). - DESIGN.md: realign the schema-migrations + DB-provisioning + password rotation docs to the external-L2 / Hook-Job model. - e2e migration_applier.py: repoint stale comments (parser logic unchanged). Signed-off-by: Anton Zelenov <antonz@constructor.tech> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
cyberantonz
added a commit
to cyberantonz/insight
that referenced
this pull request
Jul 3, 2026
deploy/seed/sql/placeholders.sql was a hand-maintained copy of src/ingestion/scripts/create-bronze-placeholders.sh and the two had drifted badly in both directions: the seed copy was missing 6 bronze databases / 22 bronze tables added after it was extracted, while the k8s script was missing silver columns + sort keys the compose seed and several gold views depend on (latent UNKNOWN_IDENTIFIER on any fresh cluster, masked on warm ones by ch_table_exists short-circuits). Reconcile the seed-side fixes into the script and delete the copy: - class_ai_dev_usage: (email, day, tool) sort key so multi-tool rows survive ReplacingMergeTree; Float64 counters (seed inserts floats); DEFAULT '' on source/source_id; same types in the reconcile ALTERs. - class_collab_*_activity: data_source in the sort key so multi-source rows survive. - class_git_commits / _pull_requests / _file_changes: repo_slug, file_path, closed_on, non-Nullable lines_added/lines_removed that git_bullet_rows' view structure requires under join_use_nulls=1. - class_support_activity: seed columns/types union'd in, data_source in the sort key. - class_ai_overage: ORDER BY unique_key + Nullable collected_at, mirroring the real dbt model. - mtr_git_person_weekly: add config_loc (referenced by ic_chart_loc since migration 20260624 but never mirrored — SELECTing that view threw UNKNOWN_IDENTIFIER on every fresh cluster) + a reconcile branch for warm placeholders. - assert CREATE DATABASE insight for compose re-bootstraps. The seed now runs the script itself: silver.py subprocess-invokes it (bash + curl over CLICKHOUSE_URL — lib/ch-exec.sh has no k8s coupling since constructorfabric#1428), and the seed image gains curl. Both schema inputs (placeholders script + gold-view migrations) are auto-located with no env knobs: the /ingestion-scripts bind-mount in the container, repo-relative to deploy/seed on host runs. The PLACEHOLDERS_SQL and MIGRATIONS_DIR env vars are gone, and the separate /migrations mount collapsed into /ingestion-scripts. NB: existing dev environments must rebuild the seed image once (docker compose --profile seed build seed-sample) to pick up curl; the bind-mounted code otherwise runs inside the stale image and the placeholders script fails with exit 127. Verified against a throwaway ClickHouse 24.8: fresh bootstrap + warm re-run (reconcile branches), all 29 migrations / 218 statements apply, 29k seed rows across 18 tables, every insight.* view SELECTable except the pre-existing dead email_daily (constructorfabric#1595). Tenant-column naming drift between dbt models and seed/placeholders is tracked in constructorfabric#1596. Signed-off-by: Anton Zelenov <antonz@constructor.tech>
cyberantonz
added a commit
that referenced
this pull request
Jul 3, 2026
deploy/seed/sql/placeholders.sql was a hand-maintained copy of src/ingestion/scripts/create-bronze-placeholders.sh and the two had drifted badly in both directions: the seed copy was missing 6 bronze databases / 22 bronze tables added after it was extracted, while the k8s script was missing silver columns + sort keys the compose seed and several gold views depend on (latent UNKNOWN_IDENTIFIER on any fresh cluster, masked on warm ones by ch_table_exists short-circuits). Reconcile the seed-side fixes into the script and delete the copy: - class_ai_dev_usage: (email, day, tool) sort key so multi-tool rows survive ReplacingMergeTree; Float64 counters (seed inserts floats); DEFAULT '' on source/source_id; same types in the reconcile ALTERs. - class_collab_*_activity: data_source in the sort key so multi-source rows survive. - class_git_commits / _pull_requests / _file_changes: repo_slug, file_path, closed_on, non-Nullable lines_added/lines_removed that git_bullet_rows' view structure requires under join_use_nulls=1. - class_support_activity: seed columns/types union'd in, data_source in the sort key. - class_ai_overage: ORDER BY unique_key + Nullable collected_at, mirroring the real dbt model. - mtr_git_person_weekly: add config_loc (referenced by ic_chart_loc since migration 20260624 but never mirrored — SELECTing that view threw UNKNOWN_IDENTIFIER on every fresh cluster) + a reconcile branch for warm placeholders. - assert CREATE DATABASE insight for compose re-bootstraps. The seed now runs the script itself: silver.py subprocess-invokes it (bash + curl over CLICKHOUSE_URL — lib/ch-exec.sh has no k8s coupling since #1428), and the seed image gains curl. Both schema inputs (placeholders script + gold-view migrations) are auto-located with no env knobs: the /ingestion-scripts bind-mount in the container, repo-relative to deploy/seed on host runs. The PLACEHOLDERS_SQL and MIGRATIONS_DIR env vars are gone, and the separate /migrations mount collapsed into /ingestion-scripts. NB: existing dev environments must rebuild the seed image once (docker compose --profile seed build seed-sample) to pick up curl; the bind-mounted code otherwise runs inside the stale image and the placeholders script fails with exit 127. Verified against a throwaway ClickHouse 24.8: fresh bootstrap + warm re-run (reconcile branches), all 29 migrations / 218 statements apply, 29k seed rows across 18 tables, every insight.* view SELECTable except the pre-existing dead email_daily (#1595). Tenant-column naming drift between dbt models and seed/placeholders is tracked in #1596. Signed-off-by: Anton Zelenov <antonz@constructor.tech>
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
2 participants
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.
Part of #1414. Closes #1417.
Phase 2 of the legacy
dev-up.shretirement. Makescharts/insighta pure application bundle by dropping the four L2 infrastructure subchart dependencies.What changed
The umbrella previously bundled
clickhouse/mariadb/redis/redpandaas subcharts, each gated by a<svc>.deployboolean. In gitops those are alwaysdeploy: false— the L2 infra runs as separate releases ininsight-infraand the umbrella dials it via<dep>.host. Thedeploy: truebundled path was used only by the now-retireddev-up.shflow, so it's dead code.Chart.yaml— removed the fourdependenciesblocks; version bumped0.1.84→0.2.0(breaking chart-API change); header comment realigned.Chart.lock— regenerated; only the fourfile://app-service subcharts remain (api-gateway, analytics-api, identity, frontend).values.yaml— each infra block trimmed to wiring fields only (host/port/protocol/database/username/passwordSecret,redpanda.brokers). Droppeddeploy/image/persistence/resources/auth/architecture/statefulset/storage/etc. Stripped staleinstall-argo.sh/deploy/argo/rbac.yaml/install-airbyte.shcomment refs.templates/_helpers.tpl— collapsed the host/fqdn/brokers helpers to the external-only path; removed the deadmariadb.deploydrift validator (it referenced the now-removedmariadb.auth.*).templates/{NOTES.txt,clickhouse-init-svcdbs-job,mariadb-init-svcdbs-job,secrets,ingestion/dbt-run}.yaml+README.md— comments/docs realigned to the external-L2 model (dropped bundled-mode docs, the Bitnami-legacy section, and the helmfile-relationship section).Deliberate deviation from the issue checklist
clickhouse.initDatabases: [insight]was kept (the issue said delete it). It drives theclickhouse-init-svcdbs-jobpre-install Hook Job, which provisions databases against the external ClickHouse too — deleting it would drop that Job from the render. Confirmed present in the rendered output.Verification
helm dependency update→Chart.lockwith only the 4 app-service entries. ✅helm template charts/insight --values deploy/gitops/environments/local/values.yaml.templatediffed against the Phase 0 baseline → only the chart-version label (0.1.82→0.2.0) and a pre-existing analytics-api image-tag drift; no structural manifest changes (the bundled subcharts never rendered underdeploy: false). ✅helm lint(gitops local values) → passes. ✅helm install --dry-run=serveragainst a live cluster → passes (14 resources, API-server validated). ✅.deployreference remains.The full
make deploy ENV=localsmoke test is deferred to Phase 4 (#1419) inside the Phase 3 PR (per the EPIC plan) — it requires a full L2 bootstrap.This bumps the chart's
versionto0.2.0with a removed chart-API surface (the<dep>.deploy: truebundled mode no longer exists). CI's patch-only auto-bump will publish0.2.1on merge.insight-gitops(read-only sibling repo) pins the chart via.insight-version. After this PR's OCI publish lands, that pin must be bumped to the0.2.xline. No action needed in this repo.<dep>.deploy: trueto bundle infra must migrate to external L2 +<dep>.hostwiring.Follow-up (not in this PR)
deploy/gitops/environments/local/values.yaml.templatestill carries inert<dep>.deploy: falselines — harmless no-op overrides now; can be cleaned in Phase 3.🤖 Generated with Claude Code
Summary by CodeRabbit
Refactor
Documentation
Chores