[Helm] Update configuration to allow use of an external clickhouse instance #8048
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe recent updates primarily focus on enhancing the analytics functionality by making ClickHouse configuration more flexible. The changes involve replacing hardcoded values with dynamic environment variables and updating the Helm chart version to ensure compatibility. This improves maintainability and scalability of the system. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- components/analytics/vector/vector.toml (1 hunks)
- helm-chart/Chart.yaml (1 hunks)
- helm-chart/templates/_helpers.tpl (2 hunks)
- helm-chart/templates/analytics/cvat-analytics-secret.yml (1 hunks)
- helm-chart/templates/cvat_backend/server/deployment.yml (1 hunks)
- helm-chart/templates/cvat_backend/worker_analyticsreports/deployment.yml (1 hunks)
- helm-chart/templates/cvat_backend/worker_export/deployment.yml (1 hunks)
- helm-chart/values.yaml (3 hunks)
Files skipped from review due to trivial changes (2)
- components/analytics/vector/vector.toml
- helm-chart/Chart.yaml
Additional context used
yamllint
helm-chart/templates/analytics/cvat-analytics-secret.yml
[error] 1-1: syntax error: expected the node content, but found '-' (syntax)
helm-chart/templates/cvat_backend/worker_analyticsreports/deployment.yml
[error] 1-1: syntax error: expected the node content, but found '-' (syntax)
helm-chart/templates/cvat_backend/server/deployment.yml
[error] 1-1: syntax error: expected the node content, but found '-' (syntax)
helm-chart/templates/cvat_backend/worker_export/deployment.yml
[error] 1-1: syntax error: expected the node content, but found '-' (syntax)
Additional comments not posted (7)
helm-chart/templates/analytics/cvat-analytics-secret.yml (1)
20-20: The addition ofCLICKHOUSE_PORTto the Kubernetes secret is a good practice for dynamic configuration management.helm-chart/templates/cvat_backend/worker_analyticsreports/deployment.yml (1)
60-60: Including shared ClickHouse environment variables enhances configuration management and reduces the risk of configuration drift across different components.helm-chart/templates/_helpers.tpl (1)
143-171: Defining ClickHouse environment variables in a shared template is an excellent practice for maintaining consistency and reducing duplication across deployments.helm-chart/templates/cvat_backend/server/deployment.yml (1)
62-62: The inclusion of shared ClickHouse environment variables in the server deployment is crucial for integrating external database services and ensures consistent configuration across deployments.helm-chart/templates/cvat_backend/worker_export/deployment.yml (1)
60-60: Ensure that the ClickHouse environment variables are correctly included and match the expected values from the secret.helm-chart/values.yaml (2)
329-329: Ensure the ClickHouse port is correctly set and reflects the port exposed by the ClickHouse service.
364-364: Review the Grafana datasource configuration to ensure it correctly references the ClickHouse environment variables. Verify that thetlsSkipVerifysetting aligns with your security policies.Also applies to: 373-378, 380-380
Verification successful
The Grafana datasource configuration correctly references the ClickHouse environment variables, and the
tlsSkipVerifysetting is set tofalse, which aligns with stricter security policies.
helm-chart/values.yaml:
- Line 364:
envFromSecret: cvat-analytics-secrettlsSkipVerify: falseScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify Grafana configuration for ClickHouse datasource. # Test: Search for 'tlsSkipVerify' setting in Grafana configuration. Expect: Only occurrences where it's set to false. rg --type yaml 'tlsSkipVerify: false' helm-chart/values.yamlLength of output: 93
Script:
#!/bin/bash # Description: Verify that Grafana datasource configuration references ClickHouse environment variables. # Test: Search for ClickHouse environment variables in Grafana configuration. rg --type yaml 'ClickHouse' helm-chart/values.yamlLength of output: 77
helm-chart/templates/_helpers.tpl
Outdated
| @@ -141,3 +139,33 @@ The name of the service account to use for backend pods | |||
| value: "{{ .Release.Namespace }}" | |||
| {{- end }} | |||
| {{- end }} | |||
|
|
|||
| {{- define "cvat.sharedClickhouseEnv" }} | |||
| {{- if .Values.analytics.enabled}} | |||
There was a problem hiding this comment.
So how do you actually switch to an external instance? In Chart.yml, the Clickhouse subchart is enabled if analytics.enabled is true, so there's no way to disable the internal instance without disabling analytics altogether.
|
Please add a changelog entry. |
azhavoro
force-pushed
the
az/update_helm_clickhouse_config
branch
from
0751da3
to
4767a08
Compare
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #8048 +/- ##
==========================================
Coverage ? 83.61%
==========================================
Files ? 383
Lines ? 40440
Branches ? 3815
==========================================
Hits ? 33813
Misses ? 6627
Partials ? 0
|
Motivation and context
How has this been tested?
Checklist
developbranch(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
New Features
Improvements
0.13.1to0.13.2.Configuration Changes