From 1db2ddc1d931ccdcf71a1f24959759bfb7835f27 Mon Sep 17 00:00:00 2001 From: Marc LeBlanc <7050295+marcleblanc2@users.noreply.github.com> Date: Mon, 19 Oct 2026 19:48:18 -0600 Subject: [PATCH 1/4] ci: run helm unittest against all charts - scripts/ci/helm-unittest.sh only tested charts/sourcegraph; pass all four charts so the executor and migrator charts are tested too - drop -q, which is --failfast in helm-unittest and hid every failure after the first - the dind executor tests still targeted the pre-#876 chart (executor.Service.yaml, executor.enabled, registry Deployment + PVC) and had never run in CI; rewrite them for the current templates Amp-Thread-ID: https://ampcode.com/threads/T-01a156fc-33c6-7408-8f03-c6410e72484a Co-authored-by: Amp --- .../dind/tests/executor_test.yaml | 48 ++++++++++++------- .../tests/privateDockerRegistry_test.yaml | 22 +++------ scripts/ci/helm-unittest.sh | 8 +++- 3 files changed, 44 insertions(+), 34 deletions(-) diff --git a/charts/sourcegraph-executor/dind/tests/executor_test.yaml b/charts/sourcegraph-executor/dind/tests/executor_test.yaml index 8c972552c..ebc890027 100644 --- a/charts/sourcegraph-executor/dind/tests/executor_test.yaml +++ b/charts/sourcegraph-executor/dind/tests/executor_test.yaml @@ -2,32 +2,46 @@ suite: executor templates: - executor/docker-daemon.ConfigMap.yaml - executor/executor.Deployment.yaml - - executor/executor.Service.yaml tests: - - it: should render the Deployment and Service if executor is enabled + - it: should render the ConfigMap and a single Deployment named after the queue set: executor: - enabled: true + queueName: batches asserts: + - containsDocument: + kind: ConfigMap + apiVersion: v1 + name: docker-config + template: executor/docker-daemon.ConfigMap.yaml - containsDocument: kind: Deployment apiVersion: apps/v1 - name: executor + name: executor-batches + template: executor/executor.Deployment.yaml + - contains: + path: spec.template.spec.containers[0].env + content: + name: EXECUTOR_QUEUE_NAME + value: batches template: executor/executor.Deployment.yaml - - containsDocument: - kind: Service - apiVersion: v1 - name: executor - template: executor/executor.Service.yaml - - it: should not render any resources if executor is disabled + - it: should render one Deployment per entry in queues + template: executor/executor.Deployment.yaml set: - executor: - enabled: false + queues: + - name: codeintel + - name: batches + queueName: batches asserts: - hasDocuments: - count: 0 - templates: - - executor/docker-daemon.ConfigMap.yaml - - executor/executor.Deployment.yaml - - executor/executor.Service.yaml + count: 2 + - containsDocument: + kind: Deployment + apiVersion: apps/v1 + name: executor-codeintel + documentIndex: 0 + - containsDocument: + kind: Deployment + apiVersion: apps/v1 + name: executor-batches + documentIndex: 1 diff --git a/charts/sourcegraph-executor/dind/tests/privateDockerRegistry_test.yaml b/charts/sourcegraph-executor/dind/tests/privateDockerRegistry_test.yaml index ac3f71388..cbff736c7 100644 --- a/charts/sourcegraph-executor/dind/tests/privateDockerRegistry_test.yaml +++ b/charts/sourcegraph-executor/dind/tests/privateDockerRegistry_test.yaml @@ -1,29 +1,23 @@ suite: privateDockerRegistry templates: - - private-docker-registry.Deployment.yaml - - private-docker-registry.PersistentVolumeClaim.yaml - - private-docker-registry.Service.yaml + - private-docker-registry/private-docker-registry.Statefulset.yaml + - private-docker-registry/private-docker-registry.Service.yaml tests: - - it: should render the Deployment, Service and PVC if registry is enabled + - it: should render the StatefulSet and Service if registry is enabled set: privateDockerRegistry: enabled: true asserts: - containsDocument: - kind: Deployment + kind: StatefulSet apiVersion: apps/v1 name: private-docker-registry - template: private-docker-registry.Deployment.yaml + template: private-docker-registry/private-docker-registry.Statefulset.yaml - containsDocument: kind: Service apiVersion: v1 name: private-docker-registry - template: private-docker-registry.Service.yaml - - containsDocument: - kind: PersistentVolumeClaim - apiVersion: v1 - name: private-docker-registry - template: private-docker-registry.PersistentVolumeClaim.yaml + template: private-docker-registry/private-docker-registry.Service.yaml - it: should not render any resources if registry is disabled set: @@ -32,7 +26,3 @@ tests: asserts: - hasDocuments: count: 0 - templates: - - private-docker-registry.Deployment.yaml - - private-docker-registry.PersistentVolumeClaim.yaml - - private-docker-registry.Service.yaml diff --git a/scripts/ci/helm-unittest.sh b/scripts/ci/helm-unittest.sh index 3ec626947..5de3ebe5d 100755 --- a/scripts/ci/helm-unittest.sh +++ b/scripts/ci/helm-unittest.sh @@ -10,4 +10,10 @@ HELM_UNITTEST_VERSION="v1.0.2" helm plugin install https://github.com/helm-unittest/helm-unittest --version "$HELM_UNITTEST_VERSION" --verify=false ### Run the helm tests -helm unittest -q charts/sourcegraph +# helm unittest runs every chart even after a failure, prints a per-chart +# summary, and exits non-zero if any chart failed. +helm unittest \ + charts/sourcegraph \ + charts/sourcegraph-migrator \ + charts/sourcegraph-executor/k8s \ + charts/sourcegraph-executor/dind From d0506df1e7b2d0c9fafa0c23862bb3946bfec0b7 Mon Sep 17 00:00:00 2001 From: Marc LeBlanc <7050295+marcleblanc2@users.noreply.github.com> Date: Mon, 19 Oct 2026 19:56:16 -0600 Subject: [PATCH 2/4] migrator: add helm unit tests, document running tests for all charts - cover the Job name suffix, default and overridden args, database secret wiring (default and existingSecret), migrator.env, and priorityClassName precedence - TEST.md: show the all-charts helm unittest command CI uses and where each chart's tests live Amp-Thread-ID: https://ampcode.com/threads/T-01a156fc-33c6-7408-8f03-c6410e72484a Co-authored-by: Amp --- TEST.md | 10 +- .../tests/migrator_test.yaml | 129 ++++++++++++++++++ 2 files changed, 138 insertions(+), 1 deletion(-) create mode 100644 charts/sourcegraph-migrator/tests/migrator_test.yaml diff --git a/TEST.md b/TEST.md index 7b4b535bc..30fabfd8d 100644 --- a/TEST.md +++ b/TEST.md @@ -20,12 +20,20 @@ helm-unittest can be installed with: helm plugin install https://github.com/helm-unittest/helm-unittest ``` -Once the plugin is installed, you can run the unit tests using the following: +Once the plugin is installed, you can run the unit tests for a single chart: ```bash helm unittest ./charts/sourcegraph ``` +Or run every chart the same way CI does: + +```bash +helm unittest ./charts/sourcegraph ./charts/sourcegraph-migrator ./charts/sourcegraph-executor/k8s ./charts/sourcegraph-executor/dind +``` + +Each chart keeps its tests in a `tests/` directory next to its `templates/`. CI runs all of them via `scripts/ci/helm-unittest.sh`, so a new chart only needs a `tests/` directory and an entry in that script. + We currently do not have testing best practices or require unit tests for new changes, so add test cases at your best judgement if possible. You may check out our [existing test cases](https://github.com/sourcegraph/deploy-sourcegraph-helm/tree/main/charts/sourcegraph/tests) and helm-unittest [docs](https://github.com/helm-unittest/helm-unittest/blob/master/DOCUMENT.md). diff --git a/charts/sourcegraph-migrator/tests/migrator_test.yaml b/charts/sourcegraph-migrator/tests/migrator_test.yaml new file mode 100644 index 000000000..9f1f8770f --- /dev/null +++ b/charts/sourcegraph-migrator/tests/migrator_test.yaml @@ -0,0 +1,129 @@ +suite: migrator +templates: + - migrator/sourcegraph-migrator.Job.yaml +tests: + - it: should render a Job with a random name suffix and default args + asserts: + - isKind: + of: Job + - matchRegex: + path: metadata.name + pattern: "^migrator-[a-z0-9]{5}$" + - matchRegex: + path: spec.template.spec.containers[0].image + pattern: "^index.docker.io/sourcegraph/migrator:" + - equal: + path: spec.template.spec.containers[0].args + value: + - up + - -db=all + - equal: + path: spec.template.spec.restartPolicy + value: Never + + - it: should use migrator.name and migrator.args when set + set: + migrator: + name: db-migrate + args: + - upto + - -db=frontend + - -target=1648051770 + asserts: + - matchRegex: + path: metadata.name + pattern: "^db-migrate-[a-z0-9]{5}$" + - equal: + path: spec.template.spec.containers[0].args + value: + - upto + - -db=frontend + - -target=1648051770 + + - it: should read all three database credentials from the default secrets + asserts: + - contains: + path: spec.template.spec.containers[0].env + content: + name: PGHOST + valueFrom: + secretKeyRef: + key: host + name: pgsql-auth + - contains: + path: spec.template.spec.containers[0].env + content: + name: CODEINTEL_PGHOST + valueFrom: + secretKeyRef: + key: host + name: codeintel-db-auth + - contains: + path: spec.template.spec.containers[0].env + content: + name: CODEINSIGHTS_PGHOST + valueFrom: + secretKeyRef: + key: host + name: codeinsights-db-auth + + - it: should only swap the secret for the database that sets existingSecret + set: + pgsql: + auth: + existingSecret: my-pgsql-secret + asserts: + - contains: + path: spec.template.spec.containers[0].env + content: + name: PGHOST + valueFrom: + secretKeyRef: + key: host + name: my-pgsql-secret + - contains: + path: spec.template.spec.containers[0].env + content: + name: CODEINTEL_PGHOST + valueFrom: + secretKeyRef: + key: host + name: codeintel-db-auth + + - it: should append migrator.env after the database credentials + set: + migrator: + env: + SRC_LOG_LEVEL: + value: dbug + asserts: + - contains: + path: spec.template.spec.containers[0].env + content: + name: SRC_LOG_LEVEL + value: dbug + + - it: should prefer migrator.priorityClassName over the global one + set: + sourcegraph: + priorityClassName: global-class + migrator: + priorityClassName: migrator-class + asserts: + - equal: + path: spec.template.spec.priorityClassName + value: migrator-class + + - it: should fall back to the global priorityClassName + set: + sourcegraph: + priorityClassName: global-class + asserts: + - equal: + path: spec.template.spec.priorityClassName + value: global-class + + - it: should not set priorityClassName by default + asserts: + - isNull: + path: spec.template.spec.priorityClassName From edf69726af8316976780502df113f62b6d234464 Mon Sep 17 00:00:00 2001 From: Marc LeBlanc <7050295+marcleblanc2@users.noreply.github.com> Date: Mon, 19 Oct 2026 20:02:04 -0600 Subject: [PATCH 3/4] TEST.md: wrap lines at 80 columns for markdownlint Prose is reflowed and the three long shell commands use line continuations; no wording changes. Amp-Thread-ID: https://ampcode.com/threads/T-01a156fc-33c6-7408-8f03-c6410e72484a Co-authored-by: Amp --- TEST.md | 55 ++++++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 40 insertions(+), 15 deletions(-) diff --git a/TEST.md b/TEST.md index 30fabfd8d..3bb518248 100644 --- a/TEST.md +++ b/TEST.md @@ -1,6 +1,7 @@ # Sourcegraph Helm Chart Test Guide -This is a temporary test guide before we have more thorough automated integration tests. +This is a temporary test guide before we have more thorough automated +integration tests. ## Lint @@ -12,7 +13,8 @@ helm lint charts/sourcegraph/. ## Unit testing -We utilize [helm-unittest](https://github.com/helm-unittest/helm-unittest/), a BDD styled unit test framework, to validate our helm chart. +We utilize [helm-unittest](https://github.com/helm-unittest/helm-unittest/), a +BDD styled unit test framework, to validate our helm chart. helm-unittest can be installed with: @@ -29,14 +31,24 @@ helm unittest ./charts/sourcegraph Or run every chart the same way CI does: ```bash -helm unittest ./charts/sourcegraph ./charts/sourcegraph-migrator ./charts/sourcegraph-executor/k8s ./charts/sourcegraph-executor/dind +helm unittest \ + ./charts/sourcegraph \ + ./charts/sourcegraph-migrator \ + ./charts/sourcegraph-executor/k8s \ + ./charts/sourcegraph-executor/dind ``` -Each chart keeps its tests in a `tests/` directory next to its `templates/`. CI runs all of them via `scripts/ci/helm-unittest.sh`, so a new chart only needs a `tests/` directory and an entry in that script. +Each chart keeps its tests in a `tests/` directory next to its `templates/`. CI +runs all of them via `scripts/ci/helm-unittest.sh`, so a new chart only needs a +`tests/` directory and an entry in that script. -We currently do not have testing best practices or require unit tests for new changes, so add test cases at your best judgement if possible. +We currently do not have testing best practices or require unit tests for new +changes, so add test cases at your best judgement if possible. -You may check out our [existing test cases](https://github.com/sourcegraph/deploy-sourcegraph-helm/tree/main/charts/sourcegraph/tests) and helm-unittest [docs](https://github.com/helm-unittest/helm-unittest/blob/master/DOCUMENT.md). +You may check out our [existing test +cases](https://github.com/sourcegraph/deploy-sourcegraph-helm/tree/main/charts/sourcegraph/tests) +and helm-unittest +[docs](https://github.com/helm-unittest/helm-unittest/blob/master/DOCUMENT.md). ## Manual testing @@ -60,11 +72,16 @@ sourcegraph: # More values to be added in order to test your change ``` -Make sure you test both enabled and disabled toggles. For example, if you added a new values to conditional render some templates, turn it on and off in the `override.yaml` to make sure they both work. You can also include your `override.yaml` in the `Test plan` during PR review to help others understand your testing strategy. +Make sure you test both enabled and disabled toggles. For example, if you added +a new values to conditional render some templates, turn it on and off in the +`override.yaml` to make sure they both work. You can also include your +`override.yaml` in the `Test plan` during PR review to help others understand +your testing strategy. ### Update the image tag -You have two options to target specific Sourcegraph version. Add the below to your `override.yaml`: +You have two options to target specific Sourcegraph version. Add the below to +your `override.yaml`: ```yaml sourcegraph: @@ -73,11 +90,14 @@ sourcegraph: useGlobalTagAsDefault: true ``` -Alternatively, you can use `sg` to update the image tags directly in the default `values.yaml`, **DO NOT COMMIT** the changes: +Alternatively, you can use `sg` to update the image tags directly in the default +`values.yaml`, **DO NOT COMMIT** the changes: ```sh cd charts/sourcegraph -sg ops update-images -kind helm -pin-tag 6.10.0 -cr-username=$DOCKER_HUB_USERNAME -cr-password=$DOCKER_HUB_PERSONAL_ACCESS_TOKEN . +sg ops update-images -kind helm -pin-tag 6.10.0 \ + -cr-username=$DOCKER_HUB_USERNAME \ + -cr-password=$DOCKER_HUB_PERSONAL_ACCESS_TOKEN . ``` ### Inspect the entire rendered template @@ -90,20 +110,25 @@ helm template -f ./override.yaml sourcegraph charts/sourcegraph/. ### Inspect the diff -Perform a diff of the rendered helm manifests before and after your change. There're many ways to produce the diff: +Perform a diff of the rendered helm manifests before and after your change. +There're many ways to produce the diff: -- Run `helm template` before and after the change, then run `diff bundle.old.yaml bundle.new.yaml`. -- Run `helm install` before the change, then run `helm diff` to inspect the diff. +- Run `helm template` before and after the change, then run `diff + bundle.old.yaml bundle.new.yaml`. +- Run `helm install` before the change, then run `helm diff` to inspect the + diff. ### Deploy the chart You should make sure your change can be deployed ```sh -helm upgrade --install --create-namespace -n sourcegraph -f ./override.yaml sourcegraph charts/sourcegraph/. +helm upgrade --install --create-namespace -n sourcegraph \ + -f ./override.yaml sourcegraph charts/sourcegraph/. ``` -Follow the [confirm instance health] guide but use `kubectl port-forward` instead to validate instance health. +Follow the [confirm instance health] guide but use `kubectl port-forward` +instead to validate instance health. ```sh kubectl -n sourcegraph port-forward svc/sourcegraph-frontend 30080 From cb4ecbcf0462ca7546d4896a1779f22871988ab0 Mon Sep 17 00:00:00 2001 From: Marc LeBlanc <7050295+marcleblanc2@users.noreply.github.com> Date: Mon, 19 Oct 2026 20:06:51 -0600 Subject: [PATCH 4/4] migrator tests: use the same existingSecret fixture name as the main chart Checkov's secrets scanner (CKV_SECRET_6) flagged my-pgsql-secret as a high-entropy string; my-secret matches the sourcegraph chart's tests and is not flagged. Amp-Thread-ID: https://ampcode.com/threads/T-01a156fc-33c6-7408-8f03-c6410e72484a Co-authored-by: Amp --- charts/sourcegraph-migrator/tests/migrator_test.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/charts/sourcegraph-migrator/tests/migrator_test.yaml b/charts/sourcegraph-migrator/tests/migrator_test.yaml index 9f1f8770f..126d7c2a1 100644 --- a/charts/sourcegraph-migrator/tests/migrator_test.yaml +++ b/charts/sourcegraph-migrator/tests/migrator_test.yaml @@ -71,7 +71,7 @@ tests: set: pgsql: auth: - existingSecret: my-pgsql-secret + existingSecret: "my-secret" asserts: - contains: path: spec.template.spec.containers[0].env @@ -80,7 +80,7 @@ tests: valueFrom: secretKeyRef: key: host - name: my-pgsql-secret + name: "my-secret" - contains: path: spec.template.spec.containers[0].env content: