Copilot commented on code in PR #68:
URL:
https://github.com/apache/skywalking-banyandb-helm/pull/68#discussion_r3594450120
##########
chart/templates/_helpers.tpl:
##########
@@ -147,6 +147,69 @@ StatefulSet names must leave room for the
controller-revision-hash suffix
{{- end }}
{{- end }}
+{{/*
+Trace-pipeline plugin helpers
+*/}}
+
+{{/*
+Return the data-node image to use when plugins are enabled.
+This is the -plugins host image tag derived from the main image tag.
+*/}}
+{{- define "banyandb.pluginsHostImage" -}}
+{{- $repo := .Values.image.repository -}}
+{{- $tag := required "banyandb.image.tag is required when plugins are enabled"
.Values.image.tag -}}
+{{- printf "%s:%s-plugins" $repo $tag -}}
+{{- end -}}
+
+{{/*
+Return the plugin carrier image reference.
+The carrier tag defaults to <main-tag>-plugins-carrier to preserve lockstep
parity.
+*/}}
+{{- define "banyandb.pluginsCarrierImage" -}}
+{{- $plugins := .Values.plugins | default dict -}}
+{{- $pluginsImage := $plugins.image | default dict -}}
+{{- $repo := $pluginsImage.repository | default .Values.image.repository -}}
+{{- $mainTag := required "banyandb.image.tag is required when plugins are
enabled" .Values.image.tag -}}
+{{- $tag := $pluginsImage.tag | default (printf "%s-plugins-carrier" $mainTag)
-}}
+{{- printf "%s:%s" $repo $tag -}}
+{{- end -}}
+
+{{/*
+Return the third-party plugin image reference, if configured.
+*/}}
+{{- define "banyandb.pluginsThirdPartyImage" -}}
+{{- $thirdParty := (default dict .Values.plugins).thirdParty | default dict -}}
+{{- $image := $thirdParty.image | default dict -}}
+{{- if and $image.repository $image.tag -}}
+{{- printf "%s:%s" $image.repository $image.tag -}}
+{{- end -}}
+{{- end -}}
Review Comment:
`banyandb.pluginsThirdPartyImage` reads
`plugins.thirdParty.image.repository/tag`, but `values.yaml` and
`doc/parameters.md` define `plugins.thirdParty.repository/tag/pullPolicy` at
the top level. This makes the helper return an empty string even when
third-party plugins are configured, so the third-party initContainer never gets
rendered.
##########
chart/values.yaml:
##########
@@ -37,13 +37,67 @@ image:
## @param image.repository Docker repository for SkyWalking BanyanDB
##
repository: docker.io/apache/skywalking-banyandb
- ## @param image.tag Image tag/version (empty for latest)
+ ## @param image.tag Image tag/version (empty means latest at runtime)
Review Comment:
The comment says `image.tag` can be empty for latest, but multiple templates
use `required "banyandb.image.tag is required" .Values.image.tag` which makes
an empty tag invalid. The parameter description should reflect that `image.tag`
must be non-empty.
##########
chart/templates/cluster_data_statefulset.yaml:
##########
@@ -137,9 +137,61 @@ spec:
{{- toYaml . | nindent 12 }}
{{- end }}
{{- end }}
+ {{- $plugins := $.Values.plugins | default dict -}}
+ {{- $pluginsMountMode := $plugins.mountMode | default "initContainer"
-}}
+ {{- $pluginsImage := $plugins.image | default dict -}}
+ {{- $pluginsInitContainer := $plugins.initContainer | default dict -}}
+ {{- if and $plugins.enabled (eq $pluginsMountMode "initContainer") }}
+ - name: plugin-carrier
+ image: {{ include "banyandb.pluginsCarrierImage" $ }}
+ imagePullPolicy: {{ $pluginsImage.pullPolicy | default
$.Values.image.pullPolicy }}
+ command:
+ - sh
+ - -c
+ - |
+ set -euo pipefail
+ mkdir -p /plugins-shared /plugins-shared/thirdparty
+ if [ -d /plugins ] && [ "$(ls -A /plugins)" ]; then
+ cp /plugins/*.so /plugins-shared/ 2>/dev/null || true
+ fi
+ volumeMounts:
+ - name: plugins-volume
+ mountPath: /plugins-shared
+ {{- with $pluginsInitContainer.resources }}
+ resources:
+ {{- toYaml . | nindent 12 }}
+ {{- end }}
+ {{- end }}
+ {{- if and $plugins.enabled (eq $pluginsMountMode "initContainer")
(include "banyandb.pluginsThirdPartyImage" $) }}
+ {{- $thirdParty := $plugins.thirdParty | default dict -}}
+ {{- $thirdPartyImage := $thirdParty.image | default dict -}}
+ - name: plugin-thirdparty
+ image: {{ include "banyandb.pluginsThirdPartyImage" $ }}
+ imagePullPolicy: {{ $thirdPartyImage.pullPolicy | default
"IfNotPresent" }}
+ command:
Review Comment:
The third-party initContainer pull policy is read from
`$plugins.thirdParty.image.pullPolicy`, but `values.yaml` defines
`plugins.thirdParty.pullPolicy` (no `.image`). As written, user-provided
`plugins.thirdParty.pullPolicy` is ignored, and combined with the helper
mismatch it prevents third-party plugin delivery.
##########
doc/parameters.md:
##########
@@ -13,11 +13,25 @@ The content of this document describes the parameters that
can be configured in
### Container image configuration
-| Name | Description | Value
|
-| ------------------ | ----------------------------------------- |
-------------------------------------- |
-| `image.repository` | Docker repository for SkyWalking BanyanDB |
`docker.io/apache/skywalking-banyandb` |
-| `image.tag` | Image tag/version (empty for latest) | `""`
|
-| `image.pullPolicy` | Image pull policy (e.g. IfNotPresent) |
`IfNotPresent` |
+| Name | Description |
Value |
+| ------------------ | ------------------------------------------------- |
------------------------------------------ |
+| `image.repository` | Docker repository for SkyWalking BanyanDB |
`docker.io/apache/skywalking-banyandb` |
+| `image.tag` | Image tag/version (empty means latest at runtime) |
`32055eb1a069a8c6ab93b5e7de5bb72e7bf7dec0` |
Review Comment:
The `image.tag` description says empty means latest, but the chart templates
use Helm `required` on `.Values.image.tag`, so an empty tag will fail
rendering. Update the docs to reflect that `image.tag` must be non-empty (or
update templates to truly support empty tags).
##########
.github/workflows/e2e.ci.yaml:
##########
@@ -45,28 +45,48 @@ jobs:
config: test/e2e/e2e-fodc-proxy-service.yaml
- name: Run Skywalking E2E Test (BanyanDB DNS cluster state)
config: test/e2e/e2e-dns-cluster-state.yaml
+ - name: Run Skywalking E2E Test (BanyanDB trace-pipeline plugins)
+ config: test/e2e/e2e-plugins.yaml
name: ${{ matrix.test.name }}
env:
OAP_TAG: 65137fc27fd765bb44007bad7fcbe59a185fa617
OAP_REPO: ghcr.io/apache/skywalking/oap
UI_TAG: 65137fc27fd765bb44007bad7fcbe59a185fa617
UI_REPO: ghcr.io/apache/skywalking/ui
steps:
- - uses: actions/checkout@v2
+ - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 #
v4.2.2
- name: Login to ghcr
- uses: docker/login-action@c94ce9fb468520275223c153574b00df6fe4bcc9
+ uses: docker/login-action@4907a6ddec9925e35a0a9e82d7399ccc52663121 #
v4.1.0
with:
registry: ghcr.io
username: ${{ github.repository_owner }}
password: ${{ secrets.GITHUB_TOKEN }}
+ - name: Clone skywalking-infra-e2e
+ run: |
+ git clone https://github.com/apache/skywalking-infra-e2e.git
skywalking-infra-e2e-src
+ git -C skywalking-infra-e2e-src checkout
7e4b5b68716fdb7b79b21fa8908f9db497e1b115
- name: Setup go
- uses: actions/setup-go@v4
+ uses: actions/setup-go@0aaccfd150d50ccaeb58ebd88d36e91967a5f35b #
v5.4.0
with:
go-version: '1.24'
+ - name: Install docker-compose
+ shell: bash
+ run: |
+ if ! command docker-compose 2>&1 > /dev/null; then
+ echo "Installing docker-compose"
+ sudo curl -L
"https://github.com/docker/compose/releases/download/1.29.2/docker-compose-$(uname
-s)-$(uname -m)" -o /usr/local/bin/docker-compose
+ sudo chmod +x /usr/local/bin/docker-compose
+ fi
Review Comment:
The docker-compose presence check runs `command docker-compose` (which
attempts to execute docker-compose) and uses redirections in a way that doesn’t
reliably silence output. This can mis-detect availability depending on
docker-compose’s exit code. Use `command -v docker-compose >/dev/null 2>&1` to
check for the binary.
##########
chart/values-lifecycle.yaml:
##########
@@ -33,9 +33,9 @@ image:
## @param image.repository Docker repository for SkyWalking BanyanDB
##
repository: docker.io/apache/skywalking-banyandb
- ## @param image.tag Image tag/version (empty for latest)
+ ## @param image.tag Image tag/version (empty means latest at runtime)
Review Comment:
The comment says `image.tag` can be empty for latest, but templates require
a non-empty `image.tag` via Helm `required`. Update the parameter description
to avoid suggesting an empty tag is supported.
--
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]
