ci: add helm image collection - #501
Conversation
Matthiator
left a comment
There was a problem hiding this comment.
Nice — a transparent image inventory in the release notes + as a CI artifact is a great idea 🙌 (I understand Trivy scanning is being handled separately, so I've kept that out of scope here and only flagged the leftover scaffolding as cleanup.)
A few things before merge, mainly two: the collection currently doesn't actually write its output file (so the job is green but produces nothing), and the release header step has a few path issues. Details inline.
Minor nits I didn't inline: typo helm template for for '$chart' (double "for"), the # pipefail that pipes break comment reads a bit confusingly, SCRIPT_DIR looks unused, and the velero "12.1.0" → 12.1.0 change is a no-op (still a YAML string). The reloader → template-library 0.2.0 bump looks correct 👍
| - name: Extract container images | ||
| run: | | ||
| mkdir -p reports | ||
| OUTPUT_FILE="$PWD/reports/images.txt" .scripts/image-version.sh |
There was a problem hiding this comment.
This passes OUTPUT_FILE, but the script now reads IMAGE_OUTPUT_FILE — so reports/images.txt is never written. The current run confirms it: the summary stays empty and the upload logs ##[warning]No files were found with the provided path: reports/images.txt. The job is green but produces nothing. Renaming the env var to IMAGE_OUTPUT_FILE (and HELM_IMAGE_OUTPUT_FILE if you also want the dependency list) fixes it.
|
|
||
| - name: Extract images and set goreleaser header | ||
| run: | | ||
| MANAGED=/tmp/gen/managed-service-catalog/helm \ |
There was a problem hiding this comment.
Three path issues in this step:
MANAGEDpoints tomanaged-service-catalog/helm, butgeneratenow producesplatform-components/helm.CONFIG_FILE/CONFIGSare unset → they default relative to the repo root, but the generatedconfig.yamllives under/tmp/gen.- same
OUTPUT_FILEvsIMAGE_OUTPUT_FILEmismatch as in pr-checks →/tmp/images.txtis never created.
As written the script exits before GoReleaser, so the header wouldn't be filled. Something like:
MANAGED=/tmp/gen/platform-components/helm \
CONFIG_FILE=/tmp/gen/config.yaml \
CONFIGS=/tmp/gen/platform-configs/<cluster>/helm \
IMAGE_OUTPUT_FILE=/tmp/images.txt \
"$GITHUB_WORKSPACE"/.scripts/image-version.sh
(Not caught by PR CI since release only runs on tag push.)
There was a problem hiding this comment.
This part will be rewritten as the image extraction now lives in another repo.
|
|
||
|
|
||
| render_dir="$(mktemp -d)"; trap 'rm -rf "$render_dir"' EXIT | ||
| FAILED=() |
There was a problem hiding this comment.
Failures get collected in FAILED, but the array is never checked afterwards — a chart that fails dependency update/template is silently skipped and the job still passes, so its images quietly disappear from the list. Could we fail (or emit a visible ::warning::) when FAILED is non-empty?
Minor: on the helm dependency update line below, dep_out=$(… >/dev/null 2>&1) discards all output, so the later echo "$dep_out" never prints anything.
|
|
||
| echo "$IMAGES" | ||
| echo "Extracting Helm Dependencies" | ||
| HELM_IMAGES="$(find . -name Chart.yaml -exec yq '.dependencies[] | select(.name != "template-library") | .name + ": " + .version' {} \;)" |
There was a problem hiding this comment.
A few things on the dependency collection:
find .isn't scoped to$MANAGED— in the release job (full repo checkout) this scans the entire built-in catalog, not just the rendered set, so it's inconsistent between pr-checks and release.find "$MANAGED" …would scope it.- these are chart
name: versionpairs (Helm chart versions, not container images), so theHELM_IMAGESnaming is a bit misleading. HELM_IMAGE_OUTPUT_FILEis never set by either workflow, so this list is computed but never persisted anywhere.
| HELM_IMAGES="$(find . -name Chart.yaml -exec yq '.dependencies[] | select(.name != "template-library") | .name + ": " + .version' {} \;)" | ||
| echo "$HELM_IMAGES" | ||
|
|
||
| [[ -n "$HELM_IMAGES" ]] || { echo "::warning::No helm image references found"; exit 0; } |
There was a problem hiding this comment.
This exit 0 on empty helm-deps runs before the IMAGE_OUTPUT_FILE write below — so a perfectly valid container-image list wouldn't be written if no dependencies are found. Worth reordering so the image file is written first.
|
|
||
| IMAGES="$( | ||
| cat "$render_dir"/*.yaml | | ||
| grep -E '^[[:space:]]*image:' | |
There was a problem hiding this comment.
Coverage note (not a blocker): grep image: catches normal + init + sidecar containers (good!), but by design misses runtime-injected images (mutating webhooks, Kyverno, operator-set images via flags), image refs passed via args/env, and - image: list-item lines (the regex only matches image: after whitespace, not after - ). It also only renders one values set, so disabled sub-features are missed. Might be worth a comment noting this is best-effort, so "collected" isn't read as "complete."
|
|
||
| echo "Done Rendering!" | ||
|
|
||
| [[ -n "$IMAGES" ]] || { echo "::warning::No image references found"; exit 0; } |
There was a problem hiding this comment.
With set -euo pipefail, if the grep above matches nothing the pipeline exits 1 and the script dies in the IMAGES="$( … )" assignment — before ever reaching this friendly ::warning::No image references found / exit 0. A || true on the grep would make the empty-case handling actually reachable.
|
|
||
| MANAGED="${MANAGED:-${PWD}/platform-components/helm}" | ||
| CONFIG_FILE="${CONFIG_FILE:-config.yaml}" | ||
| CLUSTER_NAME="$(yq -r '.clusters[0].name' "$CONFIG_FILE")" |
There was a problem hiding this comment.
Tiny thing: yq reads $CONFIG_FILE here before the [[ -f … ]] / command -v checks below, so a missing file or missing yq produces a cryptic command-substitution error instead of your nice diagnostic. Moving the checks above this line would surface the friendly message.
| # metalb, loki and velero need custom configs | ||
| yq eval '.clusters[0].services.metallb.config.publicLoadBalancerIPs = "203.0.113.10"' -i "$CFG" | ||
| yq eval '.clusters[0].services.metallb.config.loadBalancerAddressPool = ["203.0.113.0/24"]' -i "$CFG" | ||
| yq eval '.clusters[0].storage.bucketNames.chunks = "loki"' -i "$CFG" |
There was a problem hiding this comment.
This writes .clusters[0].storage.bucketNames.chunks, but Cluster has no top-level storage field, so mapstructure drops it before schema validation → silently ineffective. The intended Loki path is likely under services.loki / the generated values. Worth double-checking this actually has the intended effect.
| with: | ||
| sparse-checkout: | | ||
| .scripts | ||
| .github/helm-profiles |
There was a problem hiding this comment.
Small cleanup: .github/helm-profiles is sparse-checked-out here but doesn't seem to be used anywhere in this job — can probably be dropped from the checkout.
|
I think we need to move and adapt this to the new repository |
efc8217 to
c95fcbb
Compare
|
This PR has changed its scope. Instead of doing both image scanning for helm images, it will pull the extracted image versions, generated by another pipeline in the catalog repository and put it into the goreleaser pipeline. Other PR can be viewed here. |
7313f42 to
54c6ea4
Compare
📝 Summary
This PR adds a step to the pipeline, that when a release happens, it fetches the image versions from the catalog and displays.
Currently wip
🧩 Type of change
🧪 Testing
🔗 Related Issues / Tickets
✅ Checklist
📎 Additional Context (optional)