Copilot commented on code in PR #53:
URL:
https://github.com/apache/skywalking-banyandb-helm/pull/53#discussion_r2785606079
##########
test/e2e/values.lifecycle.yaml:
##########
@@ -18,13 +18,16 @@
# Declare variables to be passed into your templates.
image:
- repository: ghcr.io/apache/skywalking-banyandb
- tag: 46083529398b73504e9ca929ef367cd1776aef82
+ repository: mrproliu/skywalking-banyandb
+ tag: property-schema2
Review Comment:
The lifecycle E2E values also switch the BanyanDB image to
`mrproliu/skywalking-banyandb:property-schema2`. If this image isn’t guaranteed
to exist and be accessible from CI, the workflow will become flaky; it also
bypasses the normal trusted release artifacts. Please revert to the Apache GHCR
image (or pin a digest in a trusted registry) and keep any feature-branch
images out of committed E2E configs.
```suggestion
repository: ghcr.io/apache/skywalking-banyandb
tag: latest
```
##########
test/e2e/values.dns.registry.yaml:
##########
@@ -19,12 +19,16 @@
image:
repository: ghcr.io/apache/skywalking-banyandb
- tag: 8a880f51df7a9f843803b84ca8b6e9ff9d2acfca
+ tag: 67cb3eaa71d1b2ba9a56dddc17050fd69da78593
pullPolicy: IfNotPresent
cluster:
enabled: true
- etcdEndpoints: []
+ schemaStorage:
+ mode: "property"
+ property:
+ cron: "* 2 * * *"
+ syncInterval: "20s"
Review Comment:
`cluster.schemaStorage.property` uses keys `cron` and `syncInterval` here,
but the chart values/docs introduced in this PR use `serverRepairCron` and
`clientSyncInterval` (see `chart/values.yaml`). As written, these settings will
be ignored by the chart. Please rename these fields to the chart-supported
parameter names (or update the chart/docs if the intended names are
`cron`/`syncInterval`).
```suggestion
serverRepairCron: "* 2 * * *"
clientSyncInterval: "20s"
```
##########
test/e2e/values.standalone.yaml:
##########
@@ -20,12 +20,9 @@
cluster:
enabled: false
-etcd:
- enabled: false
-
image:
- repository: ghcr.io/apache/skywalking-banyandb
- tag: 46083529398b73504e9ca929ef367cd1776aef82
+ repository: mrproliu/skywalking-banyandb
+ tag: property-schema2
Review Comment:
The E2E standalone values switch the BanyanDB image to
`mrproliu/skywalking-banyandb:property-schema2`. This is a non-official image
reference and may not be publicly available or reproducible in CI; it also
introduces a supply-chain risk compared to the Apache GHCR image used
elsewhere. Please use the project’s published image repository (e.g.,
`ghcr.io/apache/skywalking-banyandb`) with a pinned tag/digest, or
document/parameterize why an alternate repo is required for these tests.
```suggestion
repository: ghcr.io/apache/skywalking-banyandb
tag: "0.7.0"
```
##########
chart/templates/_helpers.tpl:
##########
@@ -88,16 +88,73 @@ Create the name of the service account to use
EtcdEndpoints
*/}}
{{- define "banyandb.etcdEndpoints" -}}
-{{- $endpoints := list }}
-{{- $replicaCount := int .Values.etcd.replicaCount }}
-{{- $releaseName := .Release.Name }}
-{{- $namespace := .Release.Namespace }}
-{{- range $i := until $replicaCount }}
- {{- $endpoint := printf "%s-etcd-%d.%s-etcd-headless.%s:2379" $releaseName
$i $releaseName $namespace }}
- {{- $endpoints = append $endpoints $endpoint }}
-{{- end }}
+{{- $etcdClient := index .Values "etcd-client" | default dict }}
+{{- if $etcdClient.endpoints }}
- name: BYDB_ETCD_ENDPOINTS
Review Comment:
When schema storage or node discovery mode is `etcd`, the chart no longer
injects any default `BYDB_ETCD_ENDPOINTS` unless `etcd-client.endpoints` is
explicitly set. This is a behavior change from the previous in-chart etcd
dependency and can lead to a rendered manifest that starts BanyanDB in etcd
mode without endpoints configured. Consider enforcing `etcd-client.endpoints`
with `required` (or emitting a Helm template error) whenever
`cluster.schemaStorage.mode` or `cluster.nodeDiscovery.mode` is `etcd`.
```suggestion
{{- $schemaMode := ((.Values.cluster.schemaStorage).mode) | default
"property" }}
{{- $nodeDiscoveryMode := ((.Values.cluster.nodeDiscovery).mode) | default
"dns" }}
{{- $etcdClient := index .Values "etcd-client" | default dict }}
{{- if or (eq $schemaMode "etcd") (eq $nodeDiscoveryMode "etcd") }}
{{- $endpoints := required "When cluster.schemaStorage.mode or
cluster.nodeDiscovery.mode is 'etcd', you must set 'etcd-client.endpoints'."
$etcdClient.endpoints }}
- name: BYDB_ETCD_ENDPOINTS
value: "{{- $endpoints | join "," -}}"
{{- else if $etcdClient.endpoints }}
- name: BYDB_ETCD_ENDPOINTS
```
##########
chart/templates/_helpers.tpl:
##########
@@ -88,16 +88,73 @@ Create the name of the service account to use
EtcdEndpoints
*/}}
{{- define "banyandb.etcdEndpoints" -}}
-{{- $endpoints := list }}
-{{- $replicaCount := int .Values.etcd.replicaCount }}
-{{- $releaseName := .Release.Name }}
-{{- $namespace := .Release.Namespace }}
-{{- range $i := until $replicaCount }}
- {{- $endpoint := printf "%s-etcd-%d.%s-etcd-headless.%s:2379" $releaseName
$i $releaseName $namespace }}
- {{- $endpoints = append $endpoints $endpoint }}
-{{- end }}
+{{- $etcdClient := index .Values "etcd-client" | default dict }}
+{{- if $etcdClient.endpoints }}
- name: BYDB_ETCD_ENDPOINTS
- value: "{{- $endpoints | join "," -}}"
+ value: "{{- $etcdClient.endpoints | join "," -}}"
+{{- end }}
+{{- end }}
+
+{{/*
+EtcdEnv - injects all etcd-related env vars (endpoints, auth, TLS)
+Only rendered when schema storage mode is "etcd" or node discovery mode is
"etcd"
+*/}}
+{{- define "banyandb.etcdEnv" -}}
+{{- $schemaMode := (.Values.cluster.schemaStorage).mode | default "property" }}
+{{- $nodeDiscoveryMode := ((.Values.cluster.nodeDiscovery).mode) | default
"dns" }}
+{{- if or (eq $schemaMode "etcd") (eq $nodeDiscoveryMode "etcd") }}
+{{- $etcdClient := index .Values "etcd-client" | default dict }}
+{{- $auth := $etcdClient.auth | default dict }}
+{{- $tls := $auth.tls | default dict }}
+{{- include "banyandb.etcdEndpoints" . }}
+{{- if $auth.username }}
+- name: BYDB_ETCD_USERNAME
+ value: {{ $auth.username | quote }}
+{{- end }}
+{{- if $auth.password }}
+- name: BYDB_ETCD_PASSWORD
+ value: {{ $auth.password | quote }}
+{{- end }}
+{{- if $tls.enabled }}
+{{- if $tls.secretName }}
+- name: BYDB_ETCD_TLS_CA_FILE
+ value: "/etc/tls/{{ $tls.secretName }}/{{ $tls.caFilename | default "ca.crt"
}}"
+- name: BYDB_ETCD_TLS_CERT_FILE
+ value: "/etc/tls/{{ $tls.secretName }}/{{ $tls.certFilename | default
"tls.crt" }}"
+- name: BYDB_ETCD_TLS_KEY_FILE
+ value: "/etc/tls/{{ $tls.secretName }}/{{ $tls.keyFilename | default
"tls.key" }}"
Review Comment:
`banyandb.etcdEnv` sets `BYDB_ETCD_TLS_*_FILE` paths under `/etc/tls/{{
$tls.secretName }}` based on `etcd-client.auth.tls.secretName`, but none of the
workload templates mount that secretName (they mount component-specific
`...tls.etcdSecretName` instead). Enabling `etcd-client.auth.tls.enabled=true`
will therefore produce file paths that don’t exist in the container. Please
either (a) mount `etcd-client.auth.tls.secretName` into the relevant pods, or
(b) remove this global `secretName` and keep using the existing per-component
`tls.etcdSecretName` values when constructing the env vars.
```suggestion
- name: BYDB_ETCD_TLS_CA_FILE
value: "/etc/tls/{{ $tls.caFilename | default "ca.crt" }}"
- name: BYDB_ETCD_TLS_CERT_FILE
value: "/etc/tls/{{ $tls.certFilename | default "tls.crt" }}"
- name: BYDB_ETCD_TLS_KEY_FILE
value: "/etc/tls/{{ $tls.keyFilename | default "tls.key" }}"
```
--
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]
