Merge pull request #240 from jk-cluster/ci/exclude-charts-from-install-matrix

ci: ausgeschlossene Charts aus der Install-Test-Matrix filtern

.github/workflows/validate-charts.yml

@@ -11,6 +11,8 @@ jobs:
       any-chart-changed: ${{ steps.detect.outputs.any-chart-changed }}
       scan-charts: ${{ steps.detect.outputs.scan-charts }}
       any-scan: ${{ steps.detect.outputs.any-scan }}
+      install-charts: ${{ steps.detect.outputs.install-charts }}
+      any-install: ${{ steps.detect.outputs.any-install }}
     steps:
       - name: "Checkout Repository"
         uses: actions/checkout@v7
@@ -54,6 +56,55 @@ jobs:
             echo "any-scan=true" >> "$GITHUB_OUTPUT"
           fi
 
+          # --- The install-test matrix: changed charts minus the excluded ones
+          # (issue #231). A chart in ci/excluded-charts.txt used to get a job
+          # that ran the script, printed SKIP and exited 0 - and GitHub knows
+          # only the exit code, so the check list showed a green tick for a
+          # chart nothing was ever attempted on. Dropping it here means no job
+          # exists for it and nothing claims it passed.
+          #
+          # The list is NOT parsed here. ci/run-install-test.sh --list-excluded
+          # emits "<chart><TAB><reason>", and that script is the one parser;
+          # re-implementing the format in YAML would be a second chance to
+          # select different charts, silently. The comparison below is jq's
+          # "!=" on whole strings, so a chart name is never read as a pattern.
+          INSTALL_CHARTS="$CHARTS"
+          SUMMARY_ROWS=""
+          while IFS=$'\t' read -r EX_NAME EX_REASON; do
+            [ -n "$EX_NAME" ] || continue
+            if [ "$(echo "$INSTALL_CHARTS" | jq -r --arg c "$EX_NAME" 'index($c) != null')" = "true" ]; then
+              # A "|" in the justification would break out of the table cell.
+              SUMMARY_ROWS="${SUMMARY_ROWS}| \`$EX_NAME\` | ${EX_REASON//|/\\|} |"$'\n'
+              echo "Excluded from the install-test: $EX_NAME - $EX_REASON"
+            fi
+            INSTALL_CHARTS="$(echo "$INSTALL_CHARTS" | jq -c --arg c "$EX_NAME" 'map(select(. != $c))')"
+          done <<< "$(ci/run-install-test.sh --list-excluded)"
+          echo "install-charts=$INSTALL_CHARTS" >> "$GITHUB_OUTPUT"
+          if [ "$INSTALL_CHARTS" = "[]" ]; then
+            echo "any-install=false" >> "$GITHUB_OUTPUT"
+          else
+            echo "any-install=true" >> "$GITHUB_OUTPUT"
+          fi
+
+          # An absent job says "not run" but never why, and an absence is easy
+          # to miss entirely. So every excluded chart of this PR is named here
+          # WITH its documented justification - which is the question a review
+          # actually asks. This summary is the visible half of the change; the
+          # dropped matrix entry is only the half that stops the lie.
+          if [ -n "$SUMMARY_ROWS" ]; then
+            {
+              echo "### Charts excluded from the install-test"
+              echo
+              echo "These charts are changed by this PR but listed in \`ci/excluded-charts.txt\`, so they get **no** install-test job. There is no green tick for them, because nothing was attempted (issue #231)."
+              echo
+              echo "| Chart | Documented reason |"
+              echo "| --- | --- |"
+              printf '%s' "$SUMMARY_ROWS"
+              echo
+            } >> "$GITHUB_STEP_SUMMARY"
+          fi
+          echo "Install-test matrix: $INSTALL_CHARTS"
+
   build-chart:
     name: "Build ${{ matrix.chart }}"
     needs: detect-changed-charts
@@ -162,7 +213,7 @@ jobs:
   install-test:
     name: "Install-test ${{ matrix.chart }}"
     needs: [detect-changed-charts, build-chart]
-    if: needs.detect-changed-charts.outputs.any-chart-changed == 'true'
+    if: needs.detect-changed-charts.outputs.any-install == 'true'
     runs-on: ubuntu-latest
     # Blocking since Renovate automerges patch/minor updates: a chart that no
     # longer starts must stop the PR before it reaches main and publishes. This
@@ -171,8 +222,22 @@ jobs:
     timeout-minutes: 30
     strategy:
       fail-fast: false
+      # "install-charts", not "charts": the changed charts minus the ones in
+      # ci/excluded-charts.txt, filtered in detect-changed-charts. Excluded
+      # charts are named with their reason in that job's summary (issue #231).
+      #
+      # DO NOT try to move that decision into an "if" on this job, e.g.
+      #   if: ... && !contains(fromJSON(needs.detect-changed-charts.outputs.excluded-charts), matrix.chart)
+      # to get a visible "Skipped" per chart instead of an absent entry.
+      # GitHub rejects it - jobs.<id>.if is evaluated BEFORE the matrix is
+      # expanded, so the matrix context does not exist there:
+      #   context "matrix" is not allowed here.
+      #   available contexts are "github", "inputs", "needs", "vars".
+      # Measured on run 33650724109 (PR #233, closed): the whole workflow ended
+      # in failure with zero jobs created. The per-chart value has to be gone
+      # from the matrix by the time this job is reached.
       matrix:
-        chart: ${{ fromJSON(needs.detect-changed-charts.outputs.charts) }}
+        chart: ${{ fromJSON(needs.detect-changed-charts.outputs.install-charts) }}
     env:
       K3D_VERSION: v5.9.0
       K3S_IMAGE: docker.io/rancher/k3s:v1.35.5-k3s1
@@ -227,11 +292,11 @@ jobs:
         run: curl -sSfL https://raw.githubusercontent.com/k3d-io/k3d/main/install.sh | TAG="$K3D_VERSION" bash
       - name: "Create k3d cluster"
         run: k3d cluster create install-test --image "$K3S_IMAGE" --wait --timeout 180s
-      # Excluded charts (see ci/excluded-charts.txt) are skipped inside the
-      # script with their documented reason. The variables below are empty for
-      # every other chart and are read only by the chart that declares them in
-      # ci/<chart>/settings.env (REQUIRED_ENV); the script fails loudly if a
-      # declared one is missing, it never skips over it.
+      # Every chart reaching this step is one the exclusion filter let through,
+      # so the script's SKIP branch is never taken here - it exists for local
+      # runs. The variables below are empty for every chart but the one that
+      # declares them in ci/<chart>/settings.env (REQUIRED_ENV); the script
+      # fails loudly if a declared one is missing, it never skips over it.
       - name: "Install chart and wait for readiness"
         env:
           FREESAILARR_WG_KEY: ${{ steps.load-freesailarr-vpn.outputs.FREESAILARR_WG_KEY }}

README.md

@@ -49,7 +49,7 @@ Collection of some self-made helm-charts. Each top-level directory is a standalo
   Note: the registry **accepts a re-upload of an already published version** (HTTP 201) and replaces the stored artifact, so a republish of an existing version is still silent — see #160.
 - `change-app-version.yml` — manually dispatched app-version bump; the chart must be listed in its `chart-name` choice list.
 - `ci/helm-dependencies.sh <chart-dir>` — not a workflow but the step they all share: fetches a chart's declared dependencies into `charts/`, and returns silently for the charts that declare none (all but one). It runs in **five** places, because every helm command this repo builds on exits 1 while a declared dependency is missing: before lint and package in `validate-charts.yml`, before package in `publish-helm-chart.yml`, and inside `ci/run-install-test.sh`, `ci/run-quantity-render-test.sh` and `ci/run-trivy-config.sh`. See "Chart dependencies" in the style guidelines for what that costs.
-- `validate-charts.yml` — PR pipeline: detects which charts a PR touches and, for exactly those, runs `helm lint --strict`, `helm package`, and a chart-version bump check (`template-chart` is exempt from the version check). PRs without chart changes skip the build matrix. After the build stage, an **install-test** job (same changed-charts matrix) installs each changed chart into an ephemeral [k3d](https://k3d.io) cluster on the runner and requires every workload to become Ready — see below. Both matrices follow the changed chart directories, so a PR that only changes the harness under `ci/` exercises nothing by itself; the Trivy scan has a fixed-sample fallback for that case, the install-test has none — bump the affected chart's patch version to pull it into the matrix. The **resource-quantity render test** is the exception to that rule: it runs over every chart on every PR — see below.
+- `validate-charts.yml` — PR pipeline: detects which charts a PR touches and, for exactly those, runs `helm lint --strict`, `helm package`, and a chart-version bump check (`template-chart` is exempt from the version check). PRs without chart changes skip the build matrix. After the build stage, an **install-test** job (the same changed-charts matrix, minus the charts listed in `ci/excluded-charts.txt`) installs each changed chart into an ephemeral [k3d](https://k3d.io) cluster on the runner and requires every workload to become Ready — see below. Both matrices follow the changed chart directories, so a PR that only changes the harness under `ci/` exercises nothing by itself; the Trivy scan has a fixed-sample fallback for that case, the install-test has none — bump the affected chart's patch version to pull it into the matrix. The **resource-quantity render test** is the exception to that rule: it runs over every chart on every PR — see below.
 
 ### Resource-quantity render test (all charts, every PR)
 
@@ -80,10 +80,12 @@ Fixtures live at the **repo root** under `ci/` (deliberately *not* inside the ch
 - `ci/<chart>/fixtures.yaml` — dummy secrets with in-cluster connection data, throwaway infrastructure (postgres/redis/dex), static hostPath PVs/StorageClasses where the chart pins a storage class or needs RWX, and seed jobs (e.g. `synapse generate`)
 - `ci/<chart>/fixtures.env.yaml` — fixtures that need a value which must not be committed. The file holds `${VARIABLE}` placeholders and nothing else; `ci/run-install-test.sh` renders it with `envsubst` and pipes the result straight into `kubectl` (never printed). Substitution is restricted to the names from `REQUIRED_ENV`, so an undeclared variable is never filled in and no unrelated `$…` in the manifest is touched
 - `ci/<chart>/settings.env` — optional overrides: `INSTALL_TIMEOUT` for slow starters, and `REQUIRED_ENV="VAR_A VAR_B"` to declare the variables `fixtures.env.yaml` needs. **A missing or empty declared variable fails the install-test loudly, naming the variable — it never skips the chart.** Skipping would restore exactly the green-but-meaningless signal this mechanism exists to remove; the one legitimate case of absent secrets, a PR from a fork, belongs in an `if` on the workflow job rather than in the script
-- `ci/excluded-charts.txt` — charts that cannot run in CI, each with a justification. Currently: `satisfactory` (multi-GB image + 10GB+ Steam download), `template-chart` (scaffold with placeholder image)
+- `ci/excluded-charts.txt` — charts that cannot run in CI, one per line as `<chart> <justification>`. Currently: `satisfactory` (multi-GB image + 10GB+ Steam download), `template-chart` (scaffold with placeholder image). The justification is mandatory: a line carrying only a chart name is deliberately **not** an exclusion, because a bare name would drop a chart out of CI with nothing to review. **An excluded chart gets no install-test job at all** — see below
 
 `freesailarr` is the only chart using `fixtures.env.yaml` today: gluetun needs a real Wireguard key, or it never completes a handshake and no app container behind its kill switch becomes Ready. The install-test job loads the key from 1Password Connect for that chart only, using the same pattern as `publish-helm-chart.yml` — `steps.<id>.outputs.*` into the step's own `env:`, quoted, with `:?` guards that name the missing `op://` field before the cluster is even created. The key in that item is a **CI-only Mullvad key**, deliberately not the production one: Mullvad ties an address to a key and routes the tunnel to the endpoint it saw last, so a shared key would make every PR run pull the tunnel out from under the running stack — with the symptom in the cluster and the cause in a pull request.
 
+**An excluded chart is dropped from the matrix, not skipped inside its job (#231).** It used to get a job that ran the script, printed `SKIP` and exited 0 — and GitHub knows only the exit code, so the check list showed `Install-test satisfactory ✓` for a chart whose image was never pulled, indistinguishable from one that really came up in the cluster. `detect-changed-charts` now removes the excluded charts before the matrix is built, so no job exists for them and nothing claims they passed. Because an absence on its own says nothing, that job writes every excluded chart of the PR **with its documented justification** into the job summary — a `Skipped` would only say "not run", and why is the question a review actually asks. The list is parsed in exactly one place: `ci/run-install-test.sh --list-excluded` emits `<chart><TAB><reason>` and the workflow calls it instead of re-implementing the format in YAML, since a second parser would be a second chance to select different charts — silently, and in either direction. The script keeps its own `SKIP` branch for local runs. What cannot be done instead: deciding per chart in an `if` on the install-test job, to get a visible `Skipped` entry rather than an absent one. GitHub rejects it with `context "matrix" is not allowed here` — `jobs.<id>.if` is evaluated **before** the matrix is expanded, so the `matrix` context does not exist there, and the whole workflow ends in failure with zero jobs created (measured on the closed PR #233).
+
 Fixture images avoid Docker Hub where possible (`public.ecr.aws`/`ghcr.io`) to dodge runner rate limits. The job **blocks the PR** on failure — the stabilization phase of #22 ended when Renovate started automerging patch/minor updates, since a chart that no longer starts must not reach `main` and publish unattended. When adding a new chart, add its `ci/<chart>/` fixtures (or an entry in `ci/excluded-charts.txt`) — the install-test fails if both are missing. The script also runs locally against any existing cluster: `k3d cluster create t && ci/run-install-test.sh <chart> && k3d cluster delete t`.
 
 ## Dependency updates (Renovate)
@@ -182,6 +184,7 @@ These guidelines are **binding for every current and future chart** in this repo
 
 - The main app image tag comes from `{{ .Chart.AppVersion }}` — never hardcoded.
 - **Documented exception — freesailarr**: the chart bundles nine independently versioned upstreams, so none of them is "the" app version. **No** tag comes from `appVersion`; all nine images are pinned as `<app>.image.repository` / `<app>.image.tag` in `values.yaml`, which is also what puts every one of them under the built-in `helm-values` manager. Its `appVersion` is the chart's own version marker, freely chosen and tied to no upstream — raised by hand when a new edition of the chart is marked as a whole. Consequently the chart has **no** block in `renovate.json5` and is **not** in the `change-app-version.yml` choice list.
+- **Documented exception — minecraft-server**: the only chart whose main image tag can be overridden, through `image.tagOverride` (#158, empty by default — the tag still comes from `appVersion`). `appVersion` is the tag and therefore fixes the JVM, and since chart 5.0.0 only the **newest** Java line is maintained out of this one directory; the older lines stay published as frozen chart versions (java8 and java17 at 4.0.0, java21 at 4.1.0) and never receive another change. Without the override, running an older JVM would mean installing a frozen chart and giving up every later fix — so the line is a decision at rollout time rather than a property of the chart. The override is named `tagOverride`, not `tag`, because it decouples the running image from the chart version, and it is deliberately outside Renovate's reach (`renovate.json5` tracks the `appVersion` in `Chart.yaml` and nothing else). The chart checks the pairing itself: the Java line of the **effective** tag against `minecraft.version`, failing the render with both values named — a mismatch used to install silently and only break the container afterwards.
 - Auxiliary images (sidecars, init containers, secondary workloads — e.g. the delegation nginx in matrix-synapse, guacd in patchmon, busybox init/consume images in paperless-ngx) are pinned via values instead of being hardcoded in templates, and their pinned versions are reviewed alongside every app-version update round.
 
 ### 4. Ingress

ci/run-install-test.sh

@@ -4,6 +4,7 @@
 # fixtures under ci/<chart>/ and requires every workload to become Ready.
 #
 # Usage: ci/run-install-test.sh <chart-name>
+#        ci/run-install-test.sh --list-excluded
 #
 # Layout (repo root):
 #   ci/common/            fixtures applied for every chart (OnePasswordItem stub CRD)
@@ -14,12 +15,48 @@
 #   ci/excluded-charts.txt        charts that cannot run in CI, with reasons
 set -euo pipefail
 
-CHART="${1:?usage: $0 <chart-name>}"
 NAMESPACE="default"
 RELEASE="ci"
 ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)"
 CI_DIR="$ROOT/ci"
 
+# --- The exclusion list, parsed in exactly one place ------------------------
+# Two consumers read ci/excluded-charts.txt: this script, and the install-test
+# matrix in .github/workflows/validate-charts.yml, which drops the excluded
+# charts before the matrix is built (issue #231). Two parsers would be two
+# chances to select different charts - and the failure would be silent in the
+# worst direction, a chart the workflow believes excluded and the script
+# believes testable, or the reverse. So there is one parser, here, and the
+# workflow calls it through "--list-excluded" instead of re-implementing it.
+#
+# The format is one chart per line: first whitespace-separated field the chart
+# name, the rest of the line its mandatory justification. Blank lines and lines
+# starting with "#" are comments. A line carrying only a name and no reason is
+# deliberately NOT an exclusion (the "NF > 1" below): the justification is what
+# the README requires, and a bare name would silently drop a chart out of CI
+# with nothing to review.
+list_excluded() {
+  [ -f "$CI_DIR/excluded-charts.txt" ] || return 0
+  { grep -vE '^[[:space:]]*(#|$)' "$CI_DIR/excluded-charts.txt" || true; } \
+    | awk 'NF > 1 { name = $1; $1 = ""; sub(/^[[:space:]]+/, ""); print name "\t" $0 }'
+}
+
+# The reason a chart is excluded, empty if it is not. Exact string match on the
+# name, not a regex, so a chart name is never read as a pattern.
+excluded_reason() {
+  list_excluded | awk -F'\t' -v chart="$1" '$1 == chart { print $2; exit }'
+}
+
+# Emits "<chart><TAB><reason>" for every excluded chart. Read by the detect job
+# of validate-charts.yml before any chart is picked, which is why this runs
+# before the argument is required.
+if [ "${1:-}" = "--list-excluded" ]; then
+  list_excluded
+  exit 0
+fi
+
+CHART="${1:?usage: $0 <chart-name> | --list-excluded}"
+
 # Per-chart override via ci/<chart>/settings.env (INSTALL_TIMEOUT in seconds).
 # REQUIRED_ENV is a space-separated list of environment variables the chart's
 # fixtures need; see the check below.
@@ -32,12 +69,16 @@ if [ -f "$CI_DIR/$CHART/settings.env" ]; then
 fi
 
 # --- Exclusion list ---------------------------------------------------------
-if [ -f "$CI_DIR/excluded-charts.txt" ]; then
-  REASON="$(grep -E "^$CHART([[:space:]]|$)" "$CI_DIR/excluded-charts.txt" | sed -E "s/^$CHART[[:space:]]*//" || true)"
-  if [ -n "$REASON" ]; then
+# Through excluded_reason(), i.e. the same parser --list-excluded uses, so a
+# local run and the workflow matrix can never disagree about which charts are
+# excluded. In CI this branch is no longer reached: the detect job drops the
+# excluded charts before the matrix is built, so no install-test job exists for
+# them (#231). It stays for local runs, where "ci/run-install-test.sh
+# satisfactory" should say why it does nothing instead of trying.
+REASON="$(excluded_reason "$CHART")"
+if [ -n "$REASON" ]; then
   echo "SKIP: chart '$CHART' is excluded from the install-test: $REASON"
   exit 0
-  fi
 fi
 
 if [ ! -d "$ROOT/$CHART" ] || [ ! -f "$ROOT/$CHART/Chart.yaml" ]; then