Copilot commented on code in PR #39:
URL:
https://github.com/apache/skywalking-banyandb-helm/pull/39#discussion_r2508486175
##########
chart/templates/_helpers.tpl:
##########
@@ -98,4 +98,27 @@ EtcdEndpoints
{{- end }}
- name: BYDB_ETCD_ENDPOINTS
value: "{{- $endpoints | join "," -}}"
-{{- end }}
\ No newline at end of file
+{{- end }}
+
+{{- define "banyandb.hasDataNodeListValue" -}}
+{{- $dataNodeList := include "banyandb.dataNodeListValue" . }}
+{{- if ne $dataNodeList "" }}true{{- end }}
Review Comment:
The helper function checks if the data node list is non-empty but doesn't
verify if the feature is enabled via `cluster.liaison.dataNodeListEnv.enabled`.
This means the environment variable will be set even when `enabled: false` in
values.yaml, as long as there are data nodes with the "hot" role. The condition
should also check `.Values.cluster.liaison.dataNodeListEnv.enabled` before
returning true.
```suggestion
{{- if and .Values.cluster.liaison.dataNodeListEnv.enabled (ne $dataNodeList
"") }}true{{- end }}
```
##########
chart/templates/_helpers.tpl:
##########
@@ -98,4 +98,27 @@ EtcdEndpoints
{{- end }}
- name: BYDB_ETCD_ENDPOINTS
value: "{{- $endpoints | join "," -}}"
-{{- end }}
\ No newline at end of file
+{{- end }}
+
+{{- define "banyandb.hasDataNodeListValue" -}}
+{{- $dataNodeList := include "banyandb.dataNodeListValue" . }}
+{{- if ne $dataNodeList "" }}true{{- end }}
+{{- end }}
+
+{{/*
+Generate data node names list for "hot" role only
+*/}}
+{{- define "banyandb.dataNodeListValue" -}}
+{{- $dataNodes := list }}
+{{- $fullname := include "banyandb.fullname" . }}
+{{- range $roleName, $roleConfig := .Values.cluster.data.roles }}
+ {{- if eq $roleName "hot" }}
+ {{- $replicas := $roleConfig.replicas | default
$.Values.cluster.data.nodeTemplate.replicas }}
+ {{- range $i := until (int $replicas) }}
+ {{- $nodeName := printf "%s-data-%s-%d" $fullname $roleName $i }}
+ {{- $dataNodes = append $dataNodes $nodeName }}
+ {{- end }}
+ {{- end }}
+{{- end }}
+{{- $dataNodes | join "," -}}
+{{- end }}
Review Comment:
The implementation hardcodes filtering for the "hot" role only (line 115),
but the configuration in values.yaml provides a
`cluster.liaison.dataNodeListEnv.roles` field to specify which roles to
include. The implementation should respect the configured roles list. When
`roles` is empty, it should include all roles; otherwise, it should only
include the specified roles. The current implementation ignores this
configuration entirely.
##########
chart/templates/_helpers.tpl:
##########
@@ -98,4 +98,27 @@ EtcdEndpoints
{{- end }}
- name: BYDB_ETCD_ENDPOINTS
value: "{{- $endpoints | join "," -}}"
-{{- end }}
\ No newline at end of file
+{{- end }}
+
+{{- define "banyandb.hasDataNodeListValue" -}}
+{{- $dataNodeList := include "banyandb.dataNodeListValue" . }}
+{{- if ne $dataNodeList "" }}true{{- end }}
+{{- end }}
+
+{{/*
+Generate data node names list for "hot" role only
Review Comment:
The comment states this generates data node names for "hot" role only, which
is inconsistent with the values.yaml documentation that suggests the `roles`
field should control which roles are included. The comment should be updated to
reflect the intended behavior where configurable roles determine which data
nodes are included.
```suggestion
Generate data node names list for all configured roles as specified in the
`roles` field
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
