feat(intake): add intake clickhouse to nemo-platform helm - #998
Conversation
Signed-off-by: Brian Newsom <brnewsom@nvidia.com>
7d2c927 to
f459811
Compare
|
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:
📝 WalkthroughWalkthroughThe Helm chart now supports Intake with embedded or externally managed ClickHouse, including configuration values, Kubernetes resources, API connection wiring, documentation, installation notes, CI examples, and validation checks. ChangesIntake ClickHouse integration
Sequence Diagram(s)sequenceDiagram
participant HelmChart
participant IntakeAPI
participant EmbeddedClickHouse
participant ExternalClickHouse
HelmChart->>IntakeAPI: inject ClickHouse URL and credentials
IntakeAPI->>EmbeddedClickHouse: connect when embedded mode is enabled
IntakeAPI->>ExternalClickHouse: connect when embedded mode is disabled
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@k8s/helm/helm-docs-template/nemo-helm-readme.md.gotmpl`:
- Around line 41-58: Document the external ClickHouse Secret prerequisite before
the values block in k8s/helm/helm-docs-template/nemo-helm-readme.md.gotmpl,
including a concrete kubectl create secret generic command that creates
clickhouse-credentials with the configured password key; then regenerate
k8s/helm/README.md so the same prerequisite and command appear there.
In `@k8s/helm/templates/_helpers.tpl`:
- Around line 421-425: Update the clickhouse.passwordKey helper and generated
Secret handling so embedded ClickHouse without an existing Secret always uses
the same key that clickhouse-secret.yaml creates: return the literal password
key, or make the generated Secret use the helper’s configured key. Preserve
existing-secret and external ClickHouse behavior, and keep StatefulSet/API
references aligned with the generated Secret.
In `@k8s/helm/values.yaml`:
- Around line 260-270: The default auth.password in the ClickHouse auth
configuration must not contain the known “nemo” credential. Replace the shipped
default with an empty or unset value and configure validation/templates so
deployment requires either auth.existingSecret or an explicitly supplied
password before creating the ClickHouse credentials.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4147a697-5a03-421b-ae95-ba78cf196a36
📒 Files selected for processing (13)
k8s/helm/README.mdk8s/helm/ci/23-embedded-clickhouse.yamlk8s/helm/ci/24-external-clickhouse.yamlk8s/helm/helm-docs-template/nemo-helm-readme.md.gotmplk8s/helm/templates/NOTES.txtk8s/helm/templates/_helpers.tplk8s/helm/templates/api/api-deployment.yamlk8s/helm/templates/clickhouse/clickhouse-secret.yamlk8s/helm/templates/clickhouse/clickhouse-service.yamlk8s/helm/templates/clickhouse/clickhouse-serviceaccount.yamlk8s/helm/templates/clickhouse/clickhouse-statefulset.yamlk8s/helm/values.yamltools/lint/lint-helm.sh
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 `@tools/lint/lint-helm.sh`:
- Around line 48-55: Add a third Helm validation case in the lint script after
the missing-Secret check that sets externalClickhouse.existingSecret while
omitting externalClickhouse.existingSecretPasswordKey, captures the helm
template output, fails if rendering succeeds, and asserts the expected
missing-password-key error from the helper validation.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a11cec04-3ef2-4edd-ab32-64ed26c42b98
📒 Files selected for processing (13)
k8s/helm/README.mdk8s/helm/ci/23-embedded-clickhouse.yamlk8s/helm/ci/24-external-clickhouse.yamlk8s/helm/helm-docs-template/nemo-helm-readme.md.gotmplk8s/helm/templates/NOTES.txtk8s/helm/templates/_helpers.tplk8s/helm/templates/api/api-deployment.yamlk8s/helm/templates/clickhouse/clickhouse-secret.yamlk8s/helm/templates/clickhouse/clickhouse-service.yamlk8s/helm/templates/clickhouse/clickhouse-serviceaccount.yamlk8s/helm/templates/clickhouse/clickhouse-statefulset.yamlk8s/helm/values.yamltools/lint/lint-helm.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- k8s/helm/helm-docs-template/nemo-helm-readme.md.gotmpl
- k8s/helm/README.md
Signed-off-by: Brian Newsom <brnewsom@nvidia.com>
|
Signed-off-by: Brian Newsom <brnewsom@nvidia.com>
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)
tools/lint/lint-helm.sh (1)
67-79: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winScope the credential assertions to the generated Secret and password references.
These global greps cannot distinguish the encoded password from a legitimate username or unrelated manifest field, so
bmVtbw==may cause a false failure while its absence does not prove thatdata.passwordis generated. Select the embedded ClickHouse Secret and assert its password value and consumers’key: passwordreferences directly.🤖 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 `@tools/lint/lint-helm.sh` around lines 67 - 79, Update the assertions around generated_clickhouse_output to isolate the embedded ClickHouse Secret and inspect its data.password value directly, rather than globally searching for bmVtbw==. Also scope the consumer validation to ClickHouse password references and require key: password, ensuring the generated Secret and all consumers use the generated password key without matching unrelated manifest fields.
🤖 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 `@tools/lint/lint-helm.sh`:
- Around line 67-79: Update the assertions around generated_clickhouse_output to
isolate the embedded ClickHouse Secret and inspect its data.password value
directly, rather than globally searching for bmVtbw==. Also scope the consumer
validation to ClickHouse password references and require key: password, ensuring
the generated Secret and all consumers use the generated password key without
matching unrelated manifest fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c727b9cd-826c-44b7-8b5a-79b86cf5fd17
📒 Files selected for processing (1)
tools/lint/lint-helm.sh
Signed-off-by: Brian Newsom <brnewsom@nvidia.com>
Signed-off-by: Brian Newsom <brnewsom@nvidia.com>
Signed-off-by: Brian Newsom <brnewsom@nvidia.com>
Summary by CodeRabbit