chore(clickhouse): support custom labels and pod labels #550
Conversation
WalkthroughThe changes in this pull request enhance the configuration files for ClickHouse installations by introducing conditional blocks for labels, annotations, and volume configurations in the Changes
Possibly related PRs
Suggested reviewers
Warning Tool Failures:Tool Failure Count:📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Signed-off-by: Prashant Shahi <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
charts/clickhouse/templates/clickhouse-operator/deployment.yaml (1)
8-10: Consider adding validation and documentation for labels.While the implementation is solid, consider:
- Adding validation for label values to ensure they follow Kubernetes label syntax (alphanumeric, '-', '_', or '.' characters)
- Including examples in the values.yaml documentation for both
clickhouseOperator.labelsandclickhouseOperator.podLabelsExample documentation for values.yaml:
clickhouseOperator: # -- Additional labels for the ClickHouse operator deployment # @default -- {} labels: {} # app.kubernetes.io/part-of: monitoring # environment: prod # -- Additional labels for the ClickHouse operator pods # @default -- {} podLabels: {} # app.kubernetes.io/component: database # tier: storageAlso applies to: 33-35
🧰 Tools
🪛 yamllint
[warning] 8-8: wrong indentation: expected 2 but found 4
(indentation)
[warning] 9-9: wrong indentation: expected 2 but found 4
(indentation)
[warning] 10-10: wrong indentation: expected 2 but found 4
(indentation)
charts/clickhouse/templates/clickhouse-instance/clickhouse-instance.yaml (1)
137-139: LGTM! Consider documenting label restrictions.The pod labels implementation looks good. However, it would be helpful to document Kubernetes label restrictions in the values.yaml file to guide users.
Add comments in values.yaml to specify:
- Label key format: prefix/name (prefix optional)
- Key length: ≤ 253 characters
- Value length: ≤ 63 characters
- Allowed characters: alphanumeric,
-,_,.charts/clickhouse/values.yaml (2)
181-184: Enhance documentation for label fields.While the implementation is correct, consider improving the documentation by:
- Adding descriptions using the
--prefix to maintain consistency with other fields- Including examples of common use cases
-# ClickHouse instance labels -labels: {} -# ClickHouse pod labels -podLabels: {} +# -- Labels to be added to the ClickHouse instance +# Example: +# labels: +# environment: production +# team: data-platform +labels: {} +# -- Labels to be added to the ClickHouse pods +# Example: +# podLabels: +# monitoring: enabled +# sidecar.istio.io/inject: "true" +podLabels: {}
493-496: Enhance documentation for operator label fields.Similar to the instance labels, consider improving the documentation for operator labels:
- # ClickHouse operator deployment labels - labels: {} - # ClickHouse operator pod labels - podLabels: {} + # -- Labels to be added to the ClickHouse operator deployment + # Example: + # labels: + # app.kubernetes.io/component: operator + # app.kubernetes.io/part-of: clickhouse + labels: {} + # -- Labels to be added to the ClickHouse operator pods + # Example: + # podLabels: + # monitoring: enabled + # sidecar.istio.io/inject: "true" + podLabels: {}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
charts/clickhouse/templates/clickhouse-instance/clickhouse-instance.yaml(2 hunks)charts/clickhouse/templates/clickhouse-operator/deployment.yaml(2 hunks)charts/clickhouse/values.yaml(2 hunks)
🧰 Additional context used
🪛 yamllint
charts/clickhouse/templates/clickhouse-instance/clickhouse-instance.yaml
[warning] 8-8: wrong indentation: expected 2 but found 4
(indentation)
[warning] 9-9: wrong indentation: expected 2 but found 4
(indentation)
[warning] 10-10: wrong indentation: expected 2 but found 4
(indentation)
charts/clickhouse/templates/clickhouse-operator/deployment.yaml
[warning] 8-8: wrong indentation: expected 2 but found 4
(indentation)
[warning] 9-9: wrong indentation: expected 2 but found 4
(indentation)
[warning] 10-10: wrong indentation: expected 2 but found 4
(indentation)
🔇 Additional comments (5)
charts/clickhouse/templates/clickhouse-operator/deployment.yaml (2)
8-10: LGTM! Well-structured implementation of custom labels.
The conditional block for custom labels is properly implemented using Helm templating. The toYaml function with nindent 4 ensures correct YAML formatting when rendered.
🧰 Tools
🪛 yamllint
[warning] 8-8: wrong indentation: expected 2 but found 4
(indentation)
[warning] 9-9: wrong indentation: expected 2 but found 4
(indentation)
[warning] 10-10: wrong indentation: expected 2 but found 4
(indentation)
33-35: LGTM! Correctly implemented pod-level labels.
The pod labels implementation follows the same consistent pattern and is properly scoped to the pod template level, allowing for granular label management.
charts/clickhouse/templates/clickhouse-instance/clickhouse-instance.yaml (2)
Line range hint 91-103: LGTM! Well-structured Zookeeper configuration.
The implementation properly handles both external and internal Zookeeper configurations with clean conditional logic.
🧰 Tools
🪛 yamllint
[warning] 8-8: wrong indentation: expected 2 but found 4
(indentation)
[warning] 9-9: wrong indentation: expected 2 but found 4
(indentation)
[warning] 10-10: wrong indentation: expected 2 but found 4
(indentation)
[error] 7-7: syntax error: expected the node content, but found '-'
(syntax)
Line range hint 1-266: Verify label support across related resources.
The implementation looks good overall. Let's verify if similar label support is needed in related resources.
✅ Verification successful
Based on the shell script results, I can now provide a final verification response:
Label support is consistently implemented across resources
The verification shows that label support is properly implemented across all Kubernetes resources in the chart:
- All resources use appropriate label templates (
clickhouse.labelsorclickhouseOperator.labels) - Custom label support exists where needed (e.g., pod labels, service labels)
- Label inheritance and selector labels are correctly configured in deployments and services
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if other resources in the chart might need similar label support
echo "Checking for other Kubernetes resources that might need label support..."
fd -e yaml -e tpl . charts/clickhouse/templates -x rg -l 'kind:|Kind:'
# Check for existing label implementations
echo -e "\nChecking existing label implementations..."
fd -e yaml -e tpl . charts/clickhouse/templates -x rg -C2 'labels:|Labels:'
Length of output: 5474
🧰 Tools
🪛 yamllint
[warning] 8-8: wrong indentation: expected 2 but found 4
(indentation)
[warning] 9-9: wrong indentation: expected 2 but found 4
(indentation)
[warning] 10-10: wrong indentation: expected 2 but found 4
(indentation)
[error] 7-7: syntax error: expected the node content, but found '-'
(syntax)
charts/clickhouse/values.yaml (1)
181-184: Implementation looks good!
The changes successfully add support for custom labels and pod labels for both ClickHouse instance and operator. The implementation:
- Follows Kubernetes conventions
- Maintains consistency between instance and operator configurations
- Uses appropriate default values
- Is placed logically within the configuration structure
Also applies to: 493-496
charts/clickhouse/templates/clickhouse-instance/clickhouse-instance.yaml
Show resolved
Hide resolved
Signed-off-by: Prashant Shahi <[email protected]>
Chores
Summary by CodeRabbit