From 4565758ff817ad26e7d5ac17998a0d85c842b0cd Mon Sep 17 00:00:00 2001 From: Terry Howe Date: Tue, 22 Sep 2026 08:15:38 -0600 Subject: [PATCH] test: address review feedback on e2e harness and workflow - Build the helm binary by relative package path instead of the v4 module path, so the harness also works on branches where the module carries a different major version suffix. - Scope Kubernetes release names to the run. A caller-supplied HELM_E2E_NAMESPACE may already hold releases, and a fixed name could collide with one that cleanup would then uninstall. - Fail a partially configured registry leg in the nightly workflow instead of reporting it as configured and failing later in the harness or the AWS CLI. A leg with no registry set is still skipped. Signed-off-by: Terry Howe --- .github/workflows/e2e-registries.yml | 34 ++++++++++++++++++++++++---- test/e2e/helper_test.go | 4 +++- test/e2e/oci_test.go | 16 +++++++++---- 3 files changed, 44 insertions(+), 10 deletions(-) diff --git a/.github/workflows/e2e-registries.yml b/.github/workflows/e2e-registries.yml index d60058422..130409282 100644 --- a/.github/workflows/e2e-registries.yml +++ b/.github/workflows/e2e-registries.yml @@ -9,8 +9,10 @@ name: e2e-registries # e2e.yml. Each registry is a separate matrix leg with fail-fast disabled, so # one registry being down does not hide the results of the others. # -# Required secrets, per registry. A leg whose secrets are absent is skipped -# rather than failed, so registries can be wired up one at a time. +# Required secrets, per registry. A leg whose registry secret is absent is +# skipped entirely, so registries can be wired up one at a time. A leg whose +# registry is set but is missing any of its other secrets is a misconfiguration +# and fails loudly rather than being silently skipped. # # GHCR HELM_E2E_GHCR_REGISTRY e.g. ghcr.io/helm # HELM_E2E_GHCR_USERNAME @@ -71,6 +73,9 @@ jobs: # ECR authenticates as the fixed user "AWS" with a token minted below. ECR_USERNAME: AWS ECR_PASSWORD: "" + ECR_REGION: ${{ secrets.HELM_E2E_ECR_REGION }} + ECR_ACCESS_KEY_ID: ${{ secrets.HELM_E2E_ECR_ACCESS_KEY_ID }} + ECR_SECRET_ACCESS_KEY: ${{ secrets.HELM_E2E_ECR_SECRET_ACCESS_KEY }} run: | set -euo pipefail prefix="$(printf '%s' "${MATRIX_NAME}" | tr '[:lower:]' '[:upper:]')" @@ -78,14 +83,35 @@ jobs: username="${prefix}_USERNAME" password="${prefix}_PASSWORD" - # A registry with no secrets set is skipped rather than failed, so - # registries can be wired up one at a time. + # A registry with nothing configured is skipped, so registries can be + # wired up one at a time. if [ -z "${!registry:-}" ]; then echo "no registry secret set for ${MATRIX_NAME}, skipping this leg" echo "configured=false" >> "$GITHUB_OUTPUT" exit 0 fi + # A registry that is configured but incomplete is an error. Skipping + # it would quietly report success for a registry nobody is testing, + # and letting it through only fails later inside the test harness. + missing="" + case "${MATRIX_NAME}" in + ecr) + # ECR mints its password below, but needs AWS credentials to do so. + [ -n "${ECR_REGION:-}" ] || missing="${missing} HELM_E2E_ECR_REGION" + [ -n "${ECR_ACCESS_KEY_ID:-}" ] || missing="${missing} HELM_E2E_ECR_ACCESS_KEY_ID" + [ -n "${ECR_SECRET_ACCESS_KEY:-}" ] || missing="${missing} HELM_E2E_ECR_SECRET_ACCESS_KEY" + ;; + *) + [ -n "${!username:-}" ] || missing="${missing} HELM_E2E_${prefix}_USERNAME" + [ -n "${!password:-}" ] || missing="${missing} HELM_E2E_${prefix}_TOKEN" + ;; + esac + if [ -n "${missing}" ]; then + echo "::error::${MATRIX_NAME} is partially configured; missing:${missing}" + exit 1 + fi + { echo "HELM_E2E_REGISTRY=${!registry}" echo "HELM_E2E_USERNAME=${!username:-}" diff --git a/test/e2e/helper_test.go b/test/e2e/helper_test.go index 7d0c1cf84..82770323a 100644 --- a/test/e2e/helper_test.go +++ b/test/e2e/helper_test.go @@ -136,8 +136,10 @@ func helmBinary(t *testing.T) string { return abs } + // Build by relative package path rather than module path, so this works + // on any branch regardless of the module's major version suffix. bin := filepath.Join(t.TempDir(), "helm") - build := exec.Command("go", "build", "-o", bin, "helm.sh/helm/v4/cmd/helm") + build := exec.Command("go", "build", "-o", bin, filepath.Join("..", "..", "cmd", "helm")) if out, err := build.CombinedOutput(); err != nil { t.Fatalf("building helm: %v\n%s", err, out) } diff --git a/test/e2e/oci_test.go b/test/e2e/oci_test.go index 076419cda..afd0e7daf 100644 --- a/test/e2e/oci_test.go +++ b/test/e2e/oci_test.go @@ -25,6 +25,7 @@ import ( "context" "encoding/base64" "encoding/json" + "fmt" "io" "os" "path/filepath" @@ -245,13 +246,18 @@ func TestOCIRegistryInstallToKubernetes(t *testing.T) { t.Run(tt.name, func(t *testing.T) { h.mustHelm(t, "push", chart(t, tt.chart), "oci://"+repo) + // Scope the release name to this run. A caller-supplied namespace + // may already hold releases, and a fixed name could collide with + // one, which cleanup would then uninstall. + releaseName := fmt.Sprintf("%s-%s", tt.releaseName, h.runID) + t.Cleanup(func() { - if out, err := h.helm(t, "uninstall", tt.releaseName, "--namespace", namespace, "--ignore-not-found", "--wait"); err != nil { - t.Errorf("uninstalling %s failed: %v\n%s", tt.releaseName, err, out) + if out, err := h.helm(t, "uninstall", releaseName, "--namespace", namespace, "--ignore-not-found", "--wait"); err != nil { + t.Errorf("uninstalling %s failed: %v\n%s", releaseName, err, out) } }) - h.mustHelm(t, "install", tt.releaseName, h.ref(repo, tt.chartName, ""), + h.mustHelm(t, "install", releaseName, h.ref(repo, tt.chartName, ""), "--version", tt.chartVersion, "--namespace", namespace, "--create-namespace", @@ -261,9 +267,9 @@ func TestOCIRegistryInstallToKubernetes(t *testing.T) { // Read the release back from cluster storage to confirm the // install really reached the API server. - out := h.mustHelm(t, "status", tt.releaseName, "--namespace", namespace, "--output", "json") + out := h.mustHelm(t, "status", releaseName, "--namespace", namespace, "--output", "json") if !strings.Contains(out, `"status":"deployed"`) { - t.Errorf("expected release %s to be deployed, got:\n%s", tt.releaseName, out) + t.Errorf("expected release %s to be deployed, got:\n%s", releaseName, out) } }) }