Manage instance like zookeeper and operator #618
Conversation
Signed-off-by: Nicolas Lamirault <[email protected]>
WalkthroughRefactors ClickHouse Helm charts to nest instance settings under .Values.clickhouseInstance and gate ClickHouse resources behind clickhouseInstance.enabled; introduces a separate clickhouseOperator section with conditional operator templates; adds a ConfigMap for custom functions and updates helpers to use instance-scoped value paths. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User values.yaml
participant H as Helm Template Engine
participant CI as clickhouse-instance templates
participant CO as clickhouse-operator templates
U->>H: Provide values
alt clickhouseInstance.enabled == true
H->>CI: Render ClickHouseInstallation + related resources
else
H--xCI: Skip ClickHouse instance resources
end
alt clickhouseOperator.enabled == true
H->>CO: Render Operator (NS/RBAC/SA/Secret/Deploy)
else
H--xCO: Skip Operator resources
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration 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: 3
🔭 Outside diff range comments (1)
charts/clickhouse/templates/clickhouse-instance/scripts-configmap.yaml (1)
21-32: 🛠️ Refactor suggestionAdd timeout to prevent infinite loop.
The script runs in an infinite loop without a timeout, which could lead to resource exhaustion if ClickHouse never becomes ready.
Apply this diff to add a timeout:
+ # Maximum number of attempts (10 minutes with 10s delay) + MAX_ATTEMPTS=60 + attempt=0 + while true; do + if [ $attempt -ge $MAX_ATTEMPTS ]; then + echo "Timeout waiting for Clickhouse to become ready" + exit 1 + fi + response=$(wget --spider -S "http://${HOST}:${PORT}/ping" 2>&1) exit_code=$? if [ $exit_code -eq 0 ]; then echo "Clickhouse is ready!" exit 0 else echo "Clickhouse is not ready yet. Retrying in ${DELAY} seconds..." + attempt=$((attempt + 1)) sleep $DELAY fi done
🧹 Nitpick comments (2)
charts/clickhouse/templates/clickhouse-instance/storage-class.yaml (1)
10-10: Consider making storage class names configurable.The storage class names
gce-resizableandgp2-resizableare hardcoded. Consider making them configurable through values to support different naming conventions across environments.metadata: - name: gce-resizable + name: {{ .Values.clickhouseInstance.storageClass.gcp.name | default "gce-resizable" }}metadata: - name: gp2-resizable + name: {{ .Values.clickhouseInstance.storageClass.aws.name | default "gp2-resizable" }}Also applies to: 26-26
charts/clickhouse/templates/clickhouse-instance/scripts-configmap.yaml (1)
16-17: Make host and port configurable.The script uses hardcoded values for host and port. Consider making these configurable through environment variables or template values for better flexibility.
Apply this diff to make the values configurable:
- HOST="localhost" - PORT="8123" + HOST="${HOST:-localhost}" + PORT="${PORT:-8123}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
charts/clickhouse/templates/_helper.tpl(7 hunks)charts/clickhouse/templates/clickhouse-instance/clickhouse-instance.yaml(13 hunks)charts/clickhouse/templates/clickhouse-instance/configmap.yaml(2 hunks)charts/clickhouse/templates/clickhouse-instance/exporter-configmap.yaml(2 hunks)charts/clickhouse/templates/clickhouse-instance/scripts-configmap.yaml(3 hunks)charts/clickhouse/templates/clickhouse-instance/serviceaccount.yaml(2 hunks)charts/clickhouse/templates/clickhouse-instance/storage-class.yaml(2 hunks)charts/clickhouse/values.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
charts/clickhouse/templates/clickhouse-instance/clickhouse-instance.yaml (1)
Learnt from: prashant-shahi
PR: SigNoz/charts#565
File: charts/clickhouse/values.yaml:261-264
Timestamp: 2024-12-16T16:15:01.460Z
Learning: In `charts/clickhouse/values.yaml`, the `customStartupProbe`, `customLivenessProbe`, and `customReadinessProbe` configurations are provided for advanced users to specify custom probe types such as HTTP, Exec, TCP, and gRPC.
🪛 YAMLlint (1.35.1)
charts/clickhouse/templates/clickhouse-instance/scripts-configmap.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/clickhouse/templates/clickhouse-instance/exporter-configmap.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/clickhouse/templates/clickhouse-instance/configmap.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/clickhouse/templates/clickhouse-instance/serviceaccount.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/clickhouse/templates/clickhouse-instance/clickhouse-instance.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 10-10: wrong indentation: expected 2 but found 4
(indentation)
[warning] 11-11: wrong indentation: expected 2 but found 4
(indentation)
charts/clickhouse/templates/clickhouse-instance/storage-class.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🪛 Gitleaks (8.21.2)
charts/clickhouse/values.yaml
158-158: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (8)
charts/clickhouse/templates/_helper.tpl (2)
60-64: LGTM! Improved configuration organization.The refactoring to move service account and cold storage configurations under
.Values.clickhouseInstanceenhances clarity and maintainability by grouping related settings together.Also applies to: 186-191
105-107: LGTM! Consistent image configuration structure.The image configuration for init containers, UDF, ClickHouse, and logs has been consistently moved under
.Values.clickhouseInstance, making the configuration more organized and explicit.Also applies to: 119-121, 133-135, 147-149
charts/clickhouse/templates/clickhouse-instance/exporter-configmap.yaml (1)
1-2: LGTM! Enhanced conditional logic and configuration structure.The changes improve the template by:
- Only creating the ConfigMap when the ClickHouse instance is enabled
- Maintaining consistent configuration structure under
.Values.clickhouseInstanceAlso applies to: 12-14
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/clickhouse/templates/clickhouse-instance/serviceaccount.yaml (1)
1-2: LGTM! Improved resource creation control.The changes enhance the template by:
- Only creating the ServiceAccount when the ClickHouse instance is enabled
- Maintaining consistent configuration structure for cold storage and service account settings
Also applies to: 13-13
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/clickhouse/templates/clickhouse-instance/storage-class.yaml (1)
1-2: LGTM! Enhanced resource creation control.The changes improve the template by only creating the StorageClass when the ClickHouse instance is enabled.
Also applies to: 34-34
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/clickhouse/templates/clickhouse-instance/configmap.yaml (1)
29-29: Verify the executable path.The command uses a relative path
./histogramQuantilewhich assumes the executable is in the current directory. Consider using an absolute path to ensure reliability.Run this script to verify the executable path:
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Description: Check if the histogramQuantile executable exists at the expected location # Find all histogramQuantile files fd histogramQuantile # Check if the file is executable find . -name histogramQuantile -type f -executableLength of output: 75
Executable Not Found – Manual Verification Required
The search commands did not return any executable for “histogramQuantile” in the repository. This could indicate that the binary is either built during the container image build, generated at runtime, or placed in a non-standard location. Please verify manually (or via an alternate verification method) that the executable is located where expected during container runtime.
charts/clickhouse/templates/clickhouse-instance/clickhouse-instance.yaml (1)
165-167: LGTM! Good security practices.The security context is properly configured with non-root user and group IDs, along with appropriate file system permissions.
charts/clickhouse/values.yaml (1)
273-278: LGTM! Well-defined resource requests.The resource requests are properly defined with reasonable defaults, and the limits are commented out to allow users to set them based on their needs.
| - name: {{ . }} | ||
| {{- end }} | ||
| {{- range .Values.imagePullSecrets }} | ||
| {{- range .Values.clickhouseInstance.imagePullSecrets }} |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify image pull secrets configuration.
Ensure that existing deployments using global image pull secrets continue to work after moving instance-specific secrets under .Values.clickhouseInstance.
🏁 Script executed:
#!/bin/bash
# Search for any references to the old imagePullSecrets configuration
rg -l "imagePullSecrets" | grep -v "_helper.tpl"Length of output: 1460
Global imagePullSecrets fallback missing might break legacy deployments
The current change in the _helper.tpl file directly loops over .Values.clickhouseInstance.imagePullSecrets without checking or falling back to any global configuration that might be defined (for example, in .Values.imagePullSecrets). This could cause deployments that have not migrated to the instance‐specific configuration to lose their image pull secrets settings. Please check whether you want to either move all global settings to the instance-specific field or add fallback logic in the template so that deployments using global settings continue working.
- File to inspect:
charts/clickhouse/templates/_helper.tpl(line 175) - Action needed: Either migrate global image pull secrets to
.Values.clickhouseInstance.imagePullSecretsor modify the template (e.g., using adefaultfunction) to fall back to the global configuration if instance-specific values are not provided.
| <access_key_id>{{ .Values.clickhouseInstance.coldStorage.accessKey }}</access_key_id> | ||
| <secret_access_key>{{ .Values.clickhouseInstance.coldStorage.secretAccess }}</secret_access_key> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use Kubernetes secrets for sensitive credentials.
Storing access keys and secrets in plain text within the configuration is a security risk. Consider using Kubernetes secrets to manage these credentials.
- Create a secret for the cold storage credentials:
apiVersion: v1
kind: Secret
metadata:
name: {{ include "clickhouse.fullname" . }}-cold-storage
type: Opaque
data:
access_key: {{ .Values.clickhouseInstance.coldStorage.accessKey | b64enc }}
secret_key: {{ .Values.clickhouseInstance.coldStorage.secretAccess | b64enc }}- Update the cold storage configuration to use the secret:
- <access_key_id>{{ .Values.clickhouseInstance.coldStorage.accessKey }}</access_key_id>
- <secret_access_key>{{ .Values.clickhouseInstance.coldStorage.secretAccess }}</secret_access_key>
+ <access_key_id>{{ .Values.clickhouseInstance.coldStorage.existingSecret.accessKey }}</access_key_id>
+ <secret_access_key>{{ .Values.clickhouseInstance.coldStorage.existingSecret.secretKey }}</secret_access_key>| # -- Clickhouse user | ||
| user: admin | ||
| # -- Clickhouse password | ||
| password: 27ff0399-0d3a-4bd8-919d-17c2181e6fb9 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace hardcoded password with a generated value.
Using a hardcoded password in the values file is a security risk. Consider generating a random password during installation or requiring the user to provide one.
Update the password field to use a generated value:
password: {{ randAlphaNum 32 | quote }}Or add a validation to ensure the user provides a password:
password: # -- Required: Provide a secure password for the ClickHouse user🧰 Tools
🪛 Gitleaks (8.21.2)
158-158: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
|
I understand that we are trying to disable Clickhouse itself from being deployed. Then what would be the use of this chart? Wouldn't it be simpler to have |
|
It's to use this chars in 2 separate Argo-CD applications: One which deploy only the Clickhouse operator, and one which manage Clickhouse instance |
Signed-off-by: Nicolas Lamirault <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
charts/clickhouse/values.yaml (1)
157-158:⚠️ Potential issueReplace hardcoded password with a generated value.
Using a hardcoded password in the values file is a security risk. Consider generating a random password during installation or requiring the user to provide one.
Update the password field to use a generated value:
- password: 27ff0399-0d3a-4bd8-919d-17c2181e6fb9 + password: # -- Required: Provide a secure password for the ClickHouse user
🧹 Nitpick comments (3)
charts/clickhouse/templates/clickhouse-operator/secret.yaml (1)
3-12: Consider adding annotations for secret rotation.While the secret implementation is correct, consider enhancing it with annotations to support secret rotation strategies.
Add annotations to enable secret rotation:
metadata: name: {{ include "clickhouseOperator.fullname" . }} namespace: {{ .Release.Namespace }} labels: {{ include "clickhouseOperator.labels" . | nindent 4 }} + {{- if .Values.clickhouseOperator.secret.annotations }} + annotations: + {{- toYaml .Values.clickhouseOperator.secret.annotations | nindent 4 }} + {{- end }}Let's verify if similar patterns are used in other secrets within the chart:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for secret annotations in other templates rg -l 'kind:\s*Secret' charts/clickhouse/templates/ | xargs rg 'annotations:'Length of output: 77
Secret rotation annotations not currently implemented – consider adding them.
After re-running the search for "annotations:" in all Secret templates under the charts/clickhouse/templates/ directory (the initial search returned no output), it appears that none of the other secret definitions currently include annotations for secret rotation. Thus, the proposed addition of a conditional annotations block is a valid enhancement to support secret rotation strategies.
charts/clickhouse/templates/clickhouse-operator/service.yaml (1)
16-18: Add targetPort specification for metrics port.The port configuration should include a targetPort to explicitly specify which port the service should forward to on the pods.
ports: - port: {{ .port }} name: {{ include "clickhouseOperator.fullname" $ }}-metrics + targetPort: metricscharts/clickhouse/templates/clickhouse-operator/role.yaml (1)
72-81: Enhance deployment permissions for operator self-management.The deployment permissions are currently limited to get, patch, update, and delete. Consider adding 'create' permission to allow the operator to manage its own deployment completely.
verbs: - get - patch - update - delete + - create
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
charts/clickhouse/templates/clickhouse-operator/deployment.yaml(2 hunks)charts/clickhouse/templates/clickhouse-operator/namespace.yaml(1 hunks)charts/clickhouse/templates/clickhouse-operator/role.yaml(2 hunks)charts/clickhouse/templates/clickhouse-operator/rolebinding.yaml(2 hunks)charts/clickhouse/templates/clickhouse-operator/secret.yaml(2 hunks)charts/clickhouse/templates/clickhouse-operator/service.yaml(2 hunks)charts/clickhouse/templates/clickhouse-operator/serviceaccount.yaml(2 hunks)charts/clickhouse/values.yaml(2 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
charts/clickhouse/templates/clickhouse-operator/namespace.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/clickhouse/templates/clickhouse-operator/role.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/clickhouse/templates/clickhouse-operator/serviceaccount.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/clickhouse/templates/clickhouse-operator/deployment.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/clickhouse/templates/clickhouse-operator/rolebinding.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/clickhouse/templates/clickhouse-operator/secret.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/clickhouse/templates/clickhouse-operator/service.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🔇 Additional comments (6)
charts/clickhouse/templates/clickhouse-operator/secret.yaml (1)
1-2: LGTM! Conditional blocks enhance deployment flexibility.The nested conditional blocks align well with the PR objectives by allowing separate deployment of the ClickHouse operator and its components through Argo-CD applications.
Also applies to: 13-14
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/clickhouse/templates/clickhouse-operator/namespace.yaml (1)
1-8: LGTM! Well-structured conditional namespace creation.The conditional logic ensures the namespace is only created when:
- The ClickHouse operator is enabled
- A custom namespace is specified that differs from the release namespace
This aligns well with the separation of operator and instance deployments.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/clickhouse/templates/clickhouse-operator/serviceaccount.yaml (1)
1-16: LGTM! Well-structured ServiceAccount configuration.The ServiceAccount is properly scoped to the operator with appropriate conditional creation logic, labels, annotations, and image pull secrets support.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/clickhouse/templates/clickhouse-operator/rolebinding.yaml (1)
1-22: LGTM! Well-structured RoleBinding configuration.The RoleBinding correctly associates the Role with the ServiceAccount, maintaining proper namespace scoping and conditional creation logic.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/clickhouse/templates/clickhouse-operator/deployment.yaml (1)
1-121: LGTM! The deployment configuration is well-structured and aligns with the PR objectives.The deployment template correctly implements the separation of concerns by:
- Using conditional deployment based on
.Values.clickhouseOperator.enabled- Properly configuring volumes and mounts for operator configuration
- Including both operator and metrics-exporter containers with appropriate settings
This implementation supports the PR's goal of managing the ClickHouse operator independently through Argo-CD.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 10-10: wrong indentation: expected 2 but found 4
(indentation)
[warning] 11-11: wrong indentation: expected 2 but found 4
(indentation)
charts/clickhouse/values.yaml (1)
133-493: LGTM! The consolidation of ClickHouse settings is well-structured.The reorganization of settings under
clickhouseInstanceimproves clarity and supports the PR's goal of managing the ClickHouse instance independently through Argo-CD. The configuration includes comprehensive settings for:
- Basic instance configuration (name, namespace, etc.)
- Service and networking
- Storage and persistence
- Security and resource limits
- Monitoring and logging
| accessKey: <access_key_id> | ||
| # -- Secret Access Key for S3 or GCS | ||
| secretAccess: <secret_access_key> |
There was a problem hiding this comment.
Secure cold storage credentials.
Storing AWS/GCS access keys in plain text within the values file is a security risk. Consider using Kubernetes secrets or cloud provider's native IAM roles.
For AWS, you can use IAM roles (already supported as shown in the role section below). For GCS, consider using Workload Identity.
Update the fields to use references to Kubernetes secrets:
- accessKey: <access_key_id>
- secretAccess: <secret_access_key>
+ existingSecret: "" # -- Name of existing secret containing cold storage credentials
+ accessKeyKey: "access-key" # -- Key in the secret containing the access key
+ secretAccessKey: "secret-access" # -- Key in the secret containing the secret access keyCommittable suggestion skipped: line range outside the PR's diff.
|
We're having a discussion on this internally. It'll take some time for the discussion to wrap up. Let me get back to you! Appreciate the effort here :) |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
charts/clickhouse/values.yaml (2)
147-158:⚠️ Potential issueDatabase and Credential Configuration.
The cluster settings and database names (for metrics, traces, and logs) are explicitly set. However, the hardcoded ClickHouse password at line 158 poses a security risk. Recommendation: Replace this with a generated secret or require the user to provide one, as highlighted in previous reviews.
🧰 Tools
🪛 Gitleaks (8.21.2)
158-158: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
453-456:⚠️ Potential issueCold Storage Credentials.
The access and secret keys for cold storage are hardcoded, which can expose sensitive information. Recommendation: Use Kubernetes secrets or another secure mechanism to manage these credentials.
🧹 Nitpick comments (2)
charts/clickhouse/values.yaml (2)
266-279: Resource Request Configuration.CPU and memory requests are defined, though resource limits remain commented out. For production workloads, consider uncommenting and fine‑tuning the limits to prevent resource contention.
344-370: Init Containers Configuration.The init container for installing the histogramQuantile UDF and the secondary init container waiting for ClickHouse readiness are well scripted. Consider adding more robust error handling (e.g., failure detection for the wget command) to prevent the container from stalling on failure.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
charts/clickhouse/values.yaml(2 hunks)
🧰 Additional context used
🪛 Gitleaks (8.21.2)
charts/clickhouse/values.yaml
158-158: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test-chart
🔇 Additional comments (29)
charts/clickhouse/values.yaml (29)
133-136: Encapsulate ClickHouse Instance Configurations.The new
clickhouseInstanceblock provides a clearer, instance-specific grouping of settings, improving clarity and maintainability. Ensure that all templates and scripts referencing the old global settings are updated accordingly.
137-145: Instance Identification Settings.The configuration for the instance name, namespace, and override values is clearly defined. Double-check that these values align with your naming conventions and deployment practices.
160-163: Cluster Scaling Parameters.The
replicasCountandshardsCountparameters are set to 1. Verify these defaults meet your anticipated workload and scalability requirements.
165-177: ClickHouse Image Configuration.Image settings—including registry, repository, tag, and pull policy—are clearly provided. Confirm that the selected version (
24.1.2-alpine) is compatible with your environment.
178-198: Resource Meta and Service Account Configuration.Settings for image pull secrets, labels, and the service account are established under the instance. Leaving the service account name empty allows for auto-generation, which is acceptable.
199-209: ClickHouse Service Setup.The service configuration (annotations, type, and ports) is straightforward. Ensure that using a
ClusterIPservice meets your external access and networking requirements.
210-218: TLS and External Zookeeper Configuration.Disabling TLS (
secure: false) and certificate verification (verify: false) may be acceptable for internal deployments, but review these settings against your security policies. Also, sinceexternalZookeeperis left empty, confirm that the necessary configuration will be applied when integrating with an external Zookeeper.
220-230: Pod Scheduling and Priority Settings.Pod-specific settings such as
priorityClassName,nodeSelector,tolerations,affinity, andtopologySpreadConstraintsare defined with default/empty values. Tailor these as needed for your cluster’s scheduling constraints.
231-243: Probe Configuration.The startup, liveness, and readiness probes are configured with standard intervals and thresholds. Verify that these timeouts and delays suit your application's actual startup and health response characteristics.
262-265: Custom Probes.The custom probe configurations are provided as empty objects. This setup offers flexibility for future customization; ensure you populate these if custom health checks become necessary.
280-287: Security Context Settings.The security context establishes the user, group, and file system group IDs, which is a good practice to maintain container security. No changes needed here.
288-303: Allowed Network IPs.The list of allowed network IPs is comprehensive for common private ranges. Adjust this list if additional access boundaries are required.
304-327: Persistence Configuration.Persistence settings—including the enable flag, existing claim, storage class, access modes, and volume size—are clearly outlined. Confirm that setting
storageClasstonullcorrectly triggers the default provisioner in your cluster.
328-337: User Profiles Configuration.The
profilessection is ready for any overrides you might need, while default user profiles remain unmodified. This approach offers flexibility without impacting default behavior.
338-343: Default Profiles.The default profiles for experimental window functions and nondeterministic mutations are enabled. Maintain these settings unless you have a specific reason to override these defaults, as noted in the documentation.
386-393: Cluster Layout Settings.The layout configuration sets both
shardsCountandreplicasCountto 1. Validate these settings against your scalability and high-availability requirements.
394-405: Custom ClickHouse Settings.The
settingsblock is available for overriding particular ClickHouse options, such as enabling the built-in Prometheus HTTP endpoint by uncommenting the provided lines. This flexibility is useful for fine-tuning.
406-412: Default Settings.Default paths for schema, user scripts, and custom functions are set and should typically not be overridden to ensure operational consistency.
413-420: Pod Annotations.Pod-level annotations are kept empty by default, allowing for later customization (e.g., enabling scraping for Prometheus). Uncomment and adjust these when needed.
421-432: Pod Distribution.The pod distribution settings are left as an empty array, giving you the option to explicitly define topology spread constraints if necessary.
433-437: Additional Volumes Configuration.Both
additionalVolumesandadditionalVolumeMountsare empty, which is acceptable if no extra storage is needed. Update these if additional volumes become part of your deployment.
438-451: Cold Storage Endpoint Configuration.Cold storage is configured for S3 with a placeholder endpoint. Make sure to update the endpoint with your actual bucket details and region.
457-464: AWS Role Configuration.The AWS IAM role settings are provided, with annotations indicating the role ARN (masked here). Confirm that the final values are securely managed and not exposed in plaintext.
465-482: ClickHouse Configuration Files.The
filessection is a placeholder for any custom configuration files you may want to supply to ClickHouse. Populate this mapping when custom configurations are needed.
487-491: Volume Claim Templates.The custom volume claim templates are defined but left empty, which is acceptable if additional PVC customization is not required.
492-494: Extra Containers.No extra containers have been specified. Extend this section if additional sidecar containers are necessary for your deployment.
495-554: Logs and Telemetry Configuration.The logs configuration is detailed, covering system logs, opentelemetry collector settings, and exporter configurations. Ensure that the use of environment variables (e.g.,
${env:K8S_HOST_IP}) is supported in your environment.
560-564: Custom Storage Class Installation.The
installCustomStorageClassflag is set to false by default. Enable this flag if you require dynamic provisioning with volume expansion capabilities on supported clouds such as AWS or GCP.
572-574: ClickHouse Operator Enablement.The
clickhouseOperatorsection now clearly separates operator settings from the instance settings. Withenabled: true, it supports deployment as a separate Argo‑CD application, aligning with the PR objectives. Verify that all dependencies and templates are adjusted accordingly.
|
any news @grandwizard28 ? |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
charts/clickhouse/templates/clickhouse-instance/clickhouse-instance.yaml (1)
84-85: Use Kubernetes Secrets for cold storage credentials
EmbeddingaccessKeyandsecretAccessin plain values is insecure. Please switch to mounting these from a KubernetesSecret.Also applies to: 93-94
🧹 Nitpick comments (1)
charts/clickhouse/templates/clickhouse-instance/clickhouse-instance.yaml (1)
349-349: Remove trailing whitespace
Line 349 contains trailing spaces. Cleaning this up will satisfy the linter and avoid noisy diffs.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 349-349: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
charts/clickhouse/templates/clickhouse-instance/clickhouse-instance.yaml(13 hunks)
🧰 Additional context used
🧠 Learnings (1)
charts/clickhouse/templates/clickhouse-instance/clickhouse-instance.yaml (1)
Learnt from: prashant-shahi
PR: SigNoz/charts#565
File: charts/clickhouse/values.yaml:261-264
Timestamp: 2024-12-16T16:15:01.460Z
Learning: In `charts/clickhouse/values.yaml`, the `customStartupProbe`, `customLivenessProbe`, and `customReadinessProbe` configurations are provided for advanced users to specify custom probe types such as HTTP, Exec, TCP, and gRPC.
🪛 YAMLlint (1.37.1)
charts/clickhouse/templates/clickhouse-instance/clickhouse-instance.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 10-10: wrong indentation: expected 2 but found 4
(indentation)
[warning] 11-11: wrong indentation: expected 2 but found 4
(indentation)
[error] 349-349: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test-chart
🔇 Additional comments (9)
charts/clickhouse/templates/clickhouse-instance/clickhouse-instance.yaml (9)
1-1: Conditional rendering scope added
Wrapping the ClickHouseInstallation manifest underif .Values.clickhouseInstance.enabledensures instance resources are only created when explicitly enabled, supporting the operator/instance separation use case.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
9-11: The YAMLlint indentation warnings here are false positives in the Helm context; the nestedlabelsblock correctly uses 4 spaces undermetadata.🧰 Tools
🪛 YAMLlint (1.37.1)
[warning] 10-10: wrong indentation: expected 2 but found 4
(indentation)
[warning] 11-11: wrong indentation: expected 2 but found 4
(indentation)
12-15: Approve annotations block
Conditionally rendering.Values.clickhouseInstance.annotationslets users add arbitrary annotations per instance cleanly.
19-22: Approve default volume claim template logic
The guard onpersistence.enabledand!persistence.existingClaimto renderdataVolumeClaimTemplateonly when needed is correct.
27-34: Approve user configuration mapping
Scoping user credentials and network settings under.Values.clickhouseInstance.userisolates per-instance configs as intended.
36-36: Approve profiles merge
MergingprofileswithdefaultProfilesmaintains sensible defaults while allowing overrides.
39-43: Approve cluster layout configuration
Parameterizing cluster name and layout under the newclickhouseInstancekey preserves grouping consistency.
46-46: Approve settings merge
The pattern of mergingsettingswithdefaultSettingsmirrors the profiles approach and keeps defaults intact.
61-61: Approve cold storage enable guard
Theif .Values.clickhouseInstance.coldStorage.enabledcheck correctly skips the entire cold storage sub-block when disabled.
charts/clickhouse/templates/clickhouse-instance/clickhouse-instance.yaml
Outdated
Show resolved
Hide resolved
charts/clickhouse/templates/clickhouse-instance/clickhouse-instance.yaml
Show resolved
Hide resolved
| {{- else if eq .Values.clickhouseInstance.coldStorage.type "gcs" }} | ||
| <gcs> | ||
| <support_batch_delete>false</support_batch_delete> | ||
| <type>s3</type> | ||
| <endpoint>{{ .Values.coldStorage.endpoint }}</endpoint> | ||
| <access_key_id>{{ .Values.coldStorage.accessKey }}</access_key_id> | ||
| <secret_access_key>{{ .Values.coldStorage.secretAccess }}</secret_access_key> | ||
| <endpoint>{{ .Values.clickhouseInstance.coldStorage.endpoint }}</endpoint> |
There was a problem hiding this comment.
Fix GCS storage <type> tag
The GCS branch erroneously emits <type>s3</type>. It must be <type>gcs</type> to match the storage type.
- <gcs>
- <support_batch_delete>false</support_batch_delete>
- <type>s3</type>
+ <gcs>
+ <support_batch_delete>false</support_batch_delete>
+ <type>gcs</type>📝 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.
| {{- else if eq .Values.clickhouseInstance.coldStorage.type "gcs" }} | |
| <gcs> | |
| <support_batch_delete>false</support_batch_delete> | |
| <type>s3</type> | |
| <endpoint>{{ .Values.coldStorage.endpoint }}</endpoint> | |
| <access_key_id>{{ .Values.coldStorage.accessKey }}</access_key_id> | |
| <secret_access_key>{{ .Values.coldStorage.secretAccess }}</secret_access_key> | |
| <endpoint>{{ .Values.clickhouseInstance.coldStorage.endpoint }}</endpoint> | |
| {{- else if eq .Values.clickhouseInstance.coldStorage.type "gcs" }} | |
| <gcs> | |
| <support_batch_delete>false</support_batch_delete> | |
| <type>gcs</type> | |
| <endpoint>{{ .Values.clickhouseInstance.coldStorage.endpoint }}</endpoint> |
🤖 Prompt for AI Agents
In charts/clickhouse/templates/clickhouse-instance/clickhouse-instance.yaml
around lines 88 to 92, the GCS storage configuration incorrectly sets the <type>
tag to "s3". Change the <type> tag value from "s3" to "gcs" to correctly reflect
the Google Cloud Storage type.
Signed-off-by: Nicolas Lamirault <[email protected]>
Signed-off-by: Nicolas Lamirault <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
charts/clickhouse/values.yaml (1)
627-633: Don’t ship a default operator DB user password.Require a Secret or generate one at install; empty by default to force override.
secret: # -- Specifies whether a secret should be created create: true # -- User name for Clickhouse Operator username: clickhouse_operator # -- User password for Clickhouse Operator - password: clickhouse_operator_password + password: "" # REQUIRED: set via values or provide `existingSecret` + # existingSecret: "" # Optional: Secret name with keys: username, password
♻️ Duplicate comments (2)
charts/clickhouse/values.yaml (2)
157-160: Remove hardcoded ClickHouse password.Do not ship a default static password. Require user input or an existing Secret.
Apply:
- user: admin - password: 27ff0399-0d3a-4bd8-919d-17c2181e6fb9 + user: admin + password: "" # REQUIRED: provide via values or Secret + # existingSecret: "" # Optional: name of a Secret with keys: username, password
439-466: Move cold storage credentials to a Secret.Avoid plaintext creds in values; support existing Secret and key names.
- accessKey: <access_key_id> - secretAccess: <secret_access_key> + existingSecret: "" # Name of Secret with object store creds + accessKeyKey: access-key # Key holding access key + secretAccessKey: secret-access # Key holding secret keyAlso ensure templates keep backward compat (deprecate old fields with warnings).
🧹 Nitpick comments (5)
charts/clickhouse/values.yaml (5)
201-206: Fix service type comment for accuracy.ClusterIP is the default here; include all valid types in the comment.
- # -- Service Type: LoadBalancer (allows external access) or NodePort (more secure, no extra cost) + # -- Service type: ClusterIP (default), LoadBalancer, or NodePort
268-277: Resource defaults are very light; confirm they’re realistic.100m CPU/200Mi RAM may be too low for CH even in dev. Consider higher sane defaults or document clearly.
314-321: Honor global storageClass by default.Align with ZK and reuse the global anchor so users get consistent defaults.
- storageClass: null + storageClass: *GLOBAL_SC
561-564: Document cloud/storage-class interplay.When installCustomStorageClass is true, call out required RBAC and CSI support in README/UPGRADE notes.
573-575: Defaulting operator.enabled=true: call out upgrade impact.Splitting operator vs instance can change what gets installed in existing clusters. Add clear release notes and example split installs (operator-only vs instance-only).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
charts/clickhouse/values.yaml(2 hunks)
🧰 Additional context used
🪛 Gitleaks (8.27.2)
charts/clickhouse/values.yaml
159-159: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test-chart
🔇 Additional comments (1)
charts/clickhouse/values.yaml (1)
496-555: K8S_HOST_IP injection confirmed
Env var is already set via valueFrom.fieldRef(status.podIP) in the clickhouse-instance template; no changes required.
| clickhouseInstance: | ||
| # -- Whether to install clickhouse. If false, `clickhouse.host` must be set | ||
| enabled: true | ||
|
|
||
| # -- Name of the clickhouse component | ||
| name: clickhouse | ||
|
|
||
| # -- Which namespace to install clickhouse and `clickhouse-operator` to (defaults to namespace chart is installed to) | ||
| namespace: "" | ||
| # -- Name override for clickhouse | ||
| nameOverride: "" | ||
| # -- Fullname override for clickhouse | ||
| fullnameOverride: "" | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Clarify enable/disable semantics and verify templating guards.
Ensure all ClickHouse instance resources are gated behind .Values.clickhouseInstance.enabled and operator resources behind .Values.clickhouseOperator.enabled. Also document migration from old keys to the new nested structure to avoid breaking upgrades.
Run to confirm conditional guards in templates:
🏁 Script executed:
#!/bin/bash
# Expect: all clickhouse-instance templates guarded by `if .Values.clickhouseInstance.enabled`
rg -n --glob 'charts/**/templates/**' -C2 -e '\{\{\s*if\s+\.Values\.clickhouseInstance\.enabled'
# Expect: all clickhouse-operator templates guarded by `if .Values.clickhouseOperator.enabled`
rg -n --glob 'charts/**/templates/**' -C2 -e '\{\{\s*if\s+\.Values\.clickhouseOperator\.enabled'Length of output: 197
Add conditional guards for ClickHouse instance and operator templates, and document migration
- No occurrences of
.Values.clickhouseInstance.enabledor.Values.clickhouseOperator.enabledfound incharts/clickhouse/templates; wrap all instance resources inand operator resources in{{- if .Values.clickhouseInstance.enabled }} …resources… {{- end }}
{{- if .Values.clickhouseOperator.enabled }} …resources… {{- end }}
- In
charts/clickhouse/values.yaml, add a brief upgrade note mapping legacy top-level keys (e.g.enabled,name) to the new nestedclickhouseInstance/clickhouseOperatorstructure to avoid breaking changes during upgrade.
🤖 Prompt for AI Agents
In charts/clickhouse/values.yaml around lines 134-147 and in all files under
charts/clickhouse/templates, add conditional guards and a migration note: wrap
every resource manifest that belongs to the ClickHouse instance with {{- if
.Values.clickhouseInstance.enabled }} ... {{- end }} and wrap operator manifests
with {{- if .Values.clickhouseOperator.enabled }} ... {{- end }} (ensure correct
Helm indentation and that any helper templates used by both are guarded or
tolerate being absent), and update charts/clickhouse/values.yaml to include a
brief upgrade note/comment mapping legacy top-level keys (e.g. enabled, name) to
the new nested clickhouseInstance/clickhouseOperator keys so upgrades don’t
break (document which old keys map to which new nested fields).
| allowedNetworkIps: | ||
| - "10.0.0.0/8" | ||
| - "100.64.0.0/10" | ||
| - "172.16.0.0/12" | ||
| - "192.0.0.0/24" | ||
| - "198.18.0.0/15" | ||
| - "192.168.0.0/16" | ||
|
|
There was a problem hiding this comment.
Correct reserved IPv4 block (likely typo).
192.0.0.0/24 isn’t the common “TEST-NET” block; you probably intended 192.0.2.0/24.
- - "192.0.0.0/24"
+ - "192.0.2.0/24"Optionally also include other doc-only ranges 198.51.100.0/24 and 203.0.113.0/24.
📝 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.
| allowedNetworkIps: | |
| - "10.0.0.0/8" | |
| - "100.64.0.0/10" | |
| - "172.16.0.0/12" | |
| - "192.0.0.0/24" | |
| - "198.18.0.0/15" | |
| - "192.168.0.0/16" | |
| allowedNetworkIps: | |
| - "10.0.0.0/8" | |
| - "100.64.0.0/10" | |
| - "172.16.0.0/12" | |
| - "192.0.2.0/24" | |
| - "198.18.0.0/15" | |
| - "192.168.0.0/16" |
🤖 Prompt for AI Agents
In charts/clickhouse/values.yaml around lines 297 to 304, the allowedNetworkIps
list contains 192.0.0.0/24 which is likely a typo; replace 192.0.0.0/24 with the
intended documentation/test-net block 192.0.2.0/24, and optionally add the other
documentation-only ranges 198.51.100.0/24 and 203.0.113.0/24 to the list if you
want full coverage of reserved doc-only IPv4 blocks.
| image: | ||
| registry: docker.io | ||
| repository: alpine | ||
| tag: 3.18.2 | ||
| pullPolicy: IfNotPresent | ||
| command: | ||
| - sh | ||
| - -c | ||
| - | | ||
| set -e | ||
| version="v0.0.1" | ||
| node_os=$(uname -s | tr '[:upper:]' '[:lower:]') | ||
| node_arch=$(uname -m | sed s/aarch64/arm64/ | sed s/x86_64/amd64/) | ||
| echo "Fetching histogram-binary for ${node_os}/${node_arch}" | ||
| cd /tmp | ||
| wget -O histogram-quantile.tar.gz "https://github.com/SigNoz/signoz/releases/download/histogram-quantile%2F${version}/histogram-quantile_${node_os}_${node_arch}.tar.gz" | ||
| tar -xzf histogram-quantile.tar.gz | ||
| chmod +x histogram-quantile | ||
| mv histogram-quantile /var/lib/clickhouse/user_scripts/histogramQuantile | ||
| echo "histogram-quantile installed successfully" | ||
| init: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid runtime curl/wget in init; pin and verify artifact.
Downloading binaries at install time is a supply-chain and airgap risk. Prefer baking the UDF into an image; minimally, verify checksum.
Example tweak (checksum verification step):
cd /tmp
- wget -O histogram-quantile.tar.gz "https://github.com/SigNoz/signoz/releases/download/histogram-quantile%2F${version}/histogram-quantile_${node_os}_${node_arch}.tar.gz"
+ wget -O histogram-quantile.tar.gz "https://github.com/SigNoz/signoz/releases/download/histogram-quantile%2F${version}/histogram-quantile_${node_os}_${node_arch}.tar.gz"
+ # TODO: set EXPECTED_SHA256 via values and validate
+ echo "${EXPECTED_SHA256} histogram-quantile.tar.gz" | sha256sum -c -If you want, I can propose an image-based approach with no network at runtime.
📝 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.
| image: | |
| registry: docker.io | |
| repository: alpine | |
| tag: 3.18.2 | |
| pullPolicy: IfNotPresent | |
| command: | |
| - sh | |
| - -c | |
| - | | |
| set -e | |
| version="v0.0.1" | |
| node_os=$(uname -s | tr '[:upper:]' '[:lower:]') | |
| node_arch=$(uname -m | sed s/aarch64/arm64/ | sed s/x86_64/amd64/) | |
| echo "Fetching histogram-binary for ${node_os}/${node_arch}" | |
| cd /tmp | |
| wget -O histogram-quantile.tar.gz "https://github.com/SigNoz/signoz/releases/download/histogram-quantile%2F${version}/histogram-quantile_${node_os}_${node_arch}.tar.gz" | |
| tar -xzf histogram-quantile.tar.gz | |
| chmod +x histogram-quantile | |
| mv histogram-quantile /var/lib/clickhouse/user_scripts/histogramQuantile | |
| echo "histogram-quantile installed successfully" | |
| init: | |
| image: | |
| registry: docker.io | |
| repository: alpine | |
| tag: 3.18.2 | |
| pullPolicy: IfNotPresent | |
| command: | |
| - sh | |
| - -c | |
| - | | |
| set -e | |
| version="v0.0.1" | |
| node_os=$(uname -s | tr '[:upper:]' '[:lower:]') | |
| node_arch=$(uname -m | sed s/aarch64/arm64/ | sed s/x86_64/amd64/) | |
| echo "Fetching histogram-binary for ${node_os}/${node_arch}" | |
| cd /tmp | |
| wget -O histogram-quantile.tar.gz "https://github.com/SigNoz/signoz/releases/download/histogram-quantile%2F${version}/histogram-quantile_${node_os}_${node_arch}.tar.gz" | |
| # TODO: set EXPECTED_SHA256 via values and validate | |
| echo "${EXPECTED_SHA256} histogram-quantile.tar.gz" | sha256sum -c - | |
| tar -xzf histogram-quantile.tar.gz | |
| chmod +x histogram-quantile | |
| mv histogram-quantile /var/lib/clickhouse/user_scripts/histogramQuantile | |
| echo "histogram-quantile installed successfully" | |
| init: |
🤖 Prompt for AI Agents
In charts/clickhouse/values.yaml around lines 351-371 the init container
downloads the histogram-quantile binary at runtime via wget which is a
supply-chain and airgap risk; replace this by either baking the pinned
histogram-quantile binary into the container image (preferred) and updating the
image.repository/tag to a new image that already contains
/var/lib/clickhouse/user_scripts/histogramQuantile, or if you must download at
init, pin the exact artifact URL and add checksum verification (fetch a
known-good checksum file or hardcode the expected sha256, verify the downloaded
tar.gz before extraction and exit non-zero on mismatch) and fail fast on any
network or verification error.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
charts/clickhouse/templates/clickhouse-instance/clickhouse-instance.yaml (1)
350-352: Scope extraContainers under clickhouseInstance (keep backward-compat via coalesce)Current path
.Values.extraContainersis inconsistent with the new scoping and may break values.- {{- if .Values.extraContainers }} - {{- toYaml .Values.extraContainers | nindent 12 }} - {{- end }} + {{- $extra := coalesce .Values.clickhouseInstance.extraContainers .Values.extraContainers }} + {{- if $extra }} + {{- toYaml $extra | nindent 12 }} + {{- end }}
♻️ Duplicate comments (4)
charts/clickhouse/templates/clickhouse-instance/clickhouse-instance.yaml (4)
120-122: External Zookeeper path now correctly scoped — good catch
396-409: volumeClaimTemplates path typo fixed and correctly scoped — nice
84-86: Avoid embedding access keys in plain text; prefer Kubernetes Secrets / workload identityCredentials are templated directly into the CR. Use Secrets (and/or ADC/workload identity for GCP) as previously suggested.
I can provide a Secret-based variant for both S3 and GCS if desired.
Also applies to: 93-95
89-95: GCS block sets s3 — must be gcsThis misconfig breaks GCS disks.
<gcs> <support_batch_delete>false</support_batch_delete> - <type>s3</type> + <type>gcs</type> <endpoint>{{ .Values.clickhouseInstance.coldStorage.endpoint }}</endpoint> <access_key_id>{{ .Values.clickhouseInstance.coldStorage.accessKey }}</access_key_id> <secret_access_key>{{ .Values.clickhouseInstance.coldStorage.secretAccess }}</secret_access_key>
🧹 Nitpick comments (3)
charts/clickhouse/templates/clickhouse-instance/clickhouse-instance.yaml (3)
75-76: Provide a safe default for keep_free_space_bytesPrevents "" rendering when not set.
- <keep_free_space_bytes>{{ .Values.clickhouseInstance.coldStorage.defaultKeepFreeSpaceBytes }}</keep_free_space_bytes> + <keep_free_space_bytes>{{ default "10737418240" .Values.clickhouseInstance.coldStorage.defaultKeepFreeSpaceBytes }}</keep_free_space_bytes>
200-201: Default imagePullPolicy to IfNotPresent to avoid nil lookupsIf unset, Helm will fail with a nil error.
- imagePullPolicy: {{ .Values.clickhouseInstance.initContainers.udf.image.pullPolicy }} + imagePullPolicy: {{ default "IfNotPresent" .Values.clickhouseInstance.initContainers.udf.image.pullPolicy }}- imagePullPolicy: {{ .Values.clickhouseInstance.initContainers.init.image.pullPolicy }} + imagePullPolicy: {{ default "IfNotPresent" .Values.clickhouseInstance.initContainers.init.image.pullPolicy }}- imagePullPolicy: {{ .Values.clickhouseInstance.image.pullPolicy }} + imagePullPolicy: {{ default "IfNotPresent" .Values.clickhouseInstance.image.pullPolicy }}Also applies to: 210-211, 220-220
1-1: Yamllint false positives on templating directivesThe reported yamllint errors stem from Go templating at the top of the file. Consider a yamllint rule override for Helm templates (or use helm-lint) to avoid noise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
charts/clickhouse/templates/clickhouse-instance/clickhouse-instance.yaml(14 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-12-16T16:15:01.460Z
Learnt from: prashant-shahi
PR: SigNoz/charts#565
File: charts/clickhouse/values.yaml:261-264
Timestamp: 2024-12-16T16:15:01.460Z
Learning: In `charts/clickhouse/values.yaml`, the `customStartupProbe`, `customLivenessProbe`, and `customReadinessProbe` configurations are provided for advanced users to specify custom probe types such as HTTP, Exec, TCP, and gRPC.
Applied to files:
charts/clickhouse/templates/clickhouse-instance/clickhouse-instance.yaml
🪛 YAMLlint (1.37.1)
charts/clickhouse/templates/clickhouse-instance/clickhouse-instance.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 10-10: wrong indentation: expected 2 but found 4
(indentation)
[warning] 11-11: wrong indentation: expected 2 but found 4
(indentation)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test-chart
🔇 Additional comments (1)
charts/clickhouse/templates/clickhouse-instance/clickhouse-instance.yaml (1)
233-274: Probes look consistent with custom probe overrides — LGTM
|
|
||
| profiles: | ||
| {{- merge dict .Values.profiles .Values.defaultProfiles | toYaml | nindent 6 }} | ||
| {{- merge dict .Values.clickhouseInstance.profiles .Values.clickhouseInstance.defaultProfiles | toYaml | nindent 6 }} |
There was a problem hiding this comment.
Fix merge precedence: user values currently get overridden by defaults
In Sprig, later maps win. The current order makes defaults override user-provided profiles/settings.
Apply:
- {{- merge dict .Values.clickhouseInstance.profiles .Values.clickhouseInstance.defaultProfiles | toYaml | nindent 6 }}
+ {{- merge (dict) .Values.clickhouseInstance.defaultProfiles .Values.clickhouseInstance.profiles | toYaml | nindent 6 }}- {{- merge dict .Values.clickhouseInstance.settings .Values.clickhouseInstance.defaultSettings | toYaml | nindent 6 }}
+ {{- merge (dict) .Values.clickhouseInstance.defaultSettings .Values.clickhouseInstance.settings | toYaml | nindent 6 }}Also applies to: 46-46
🤖 Prompt for AI Agents
In charts/clickhouse/templates/clickhouse-instance/clickhouse-instance.yaml
around line 36 (and also at line 46), the merge dict call passes user profiles
before defaults so defaults currently win; swap the merge arguments so defaults
are merged first and user-provided .Values.clickhouseInstance.profiles come last
(e.g. merge dict .Values.clickhouseInstance.defaultProfiles
.Values.clickhouseInstance.profiles) to ensure user values override defaults.
|
there is a new one for the Operator part: #730 |
2 participants
Fix #427
Summary by CodeRabbit
New Features
Refactor