Allow to configure labels for service #31
Conversation
|
Hi @azatsafin, can you please take a look at this PR. Thank you very much! |
|
@azatsafin don't you mind to check it? |
|
@azatsafin, when you find the time can you review this PR. Thank you. |
There was a problem hiding this comment.
Pull request overview
This PR adds support for configuring custom labels on the Kafka-UI service resource via the service.labels parameter in values.yaml. The implementation follows the existing pattern used for ingress labels in the same chart.
Key Changes:
- Added
service.labelsparameter to values.yaml with an empty object as the default - Updated service.yaml template to merge custom labels with default labels
- Documented the new parameter in README.md
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| charts/kafka-ui/values.yaml | Adds service.labels parameter with documentation comment |
| charts/kafka-ui/templates/service.yaml | Updates metadata section to include custom labels from values |
| charts/kafka-ui/README.md | Documents the new service.labels parameter in the Traffic Exposure Parameters table |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {{- if .Values.service.labels }} | ||
| {{- toYaml .Values.service.labels | nindent 4 }} | ||
| {{- end }} | ||
| {{- if .Values.service.annotations }} | ||
| annotations: | ||
| {{ toYaml .Values.service.annotations | nindent 4 }} |
There was a problem hiding this comment.
The conditional logic here uses if with explicit variable checking, but the ingress template uses the with directive for the same pattern. For consistency with the existing codebase pattern (as seen in ingress.yaml lines 19-21), consider using with instead. The with directive is more idiomatic in Helm templates for this use case as it provides automatic empty-value checking and scopes the context.
| {{- if .Values.service.labels }} | |
| {{- toYaml .Values.service.labels | nindent 4 }} | |
| {{- end }} | |
| {{- if .Values.service.annotations }} | |
| annotations: | |
| {{ toYaml .Values.service.annotations | nindent 4 }} | |
| {{- with .Values.service.labels }} | |
| {{- toYaml . | nindent 4 }} | |
| {{- end }} | |
| {{- with .Values.service.annotations }} | |
| annotations: | |
| {{ toYaml . | nindent 4 }} |
| | -------------------------- | -------------------------------------------------------------------------------------------------------------------------------- | ----------- | | ||
| | `service.type` | Kafka-UI service type | `ClusterIP` | | ||
| | `service.port` | Kafka-UI pod port number | `80` | | ||
| | `service.labels` | Kafka-UI service Labels | `{}` | |
There was a problem hiding this comment.
Capitalization inconsistency: "Labels" should be lowercase "labels" to match the capitalization pattern used for other service and ingress parameters in the same table (see lines 67, 68, 70-72 where all other parameter descriptions use lowercase for common words).
| | `service.labels` | Kafka-UI service Labels | `{}` | | |
| | `service.labels` | Kafka-UI service labels | `{}` | |
| # loadBalancerIP: 10.11.12.13 | ||
| # if you want to force a specific nodePort. Must be use with service.type=NodePort | ||
| # nodePort: | ||
| ## @param service.labels kafka-UI service labels |
There was a problem hiding this comment.
Capitalization inconsistency: "kafka-UI" should be "Kafka-UI" (with capital K) to match the capitalization pattern used consistently throughout the file (see lines 186, 190, 192, and the ingress.labels comment on line 210).
| ## @param service.labels kafka-UI service labels | |
| ## @param service.labels Kafka-UI service labels |
3 participants
We want to make it possible to add service Labels from values.yaml under service.labels