Skip to content

refactor(build): install Helm from CGW mirror instead of vendoring source - #284

Merged
Fortune-Ndlovu merged 14 commits into
mainfrom
fix/RHIDP-16046-helm-cgw-binary
Aug 13, 2026
Merged

refactor(build): install Helm from CGW mirror instead of vendoring source#284
Fortune-Ndlovu merged 14 commits into
mainfrom
fix/RHIDP-16046-helm-cgw-binary

Conversation

@nickboldt

@nickboldt nickboldt commented Aug 12, 2026

Copy link
Copy Markdown
Member

Summary

Build (Helm CGW binary)

  • Remove vendor/helm/ source tree (~50k lines) and the go-toolset compile stage
  • Install Helm v4.2.3 from mirror.openshift.com/pub/cgw/helm/ via artifacts.lock.yaml + Hermeto generic fetcher (Konflux) or curl by TARGETARCH (upstream Containerfile)
  • Add hack/update-helm-lockfile.sh, hack/check-helm-binary-available.sh, hack/install-helm-binary.sh, and make helm-lockfile-update for version bumps
  • CGW helm-builder as Containerfile Stage 2a; commented Stage 2b retains vendored go-toolset fallback when CGW lacks a release
  • Fix CGW flat tarball extraction (helm-linux-amd64 at archive root, not linux-amd64/helm)
  • Fix Makefile OS/ARCH ordering before HELM_ARCHIVE_DIR (broken E2E local-setup path on CI)

Supersedes the source-vendoring approach in draft PR #282.

Downstream

E2E fixes (Helm 4 + RHDH 2.x chart on Kind)

GitHub E2E failed after the Helm 4 upgrade for reasons unrelated to the CGW lockfile itself. Fixes on this branch:

  • Resolve RHDH chart from oci://quay.io/rhdh/chart on all branches (auto-pick latest X.Y-N-CI via sort -V; drop flaky GitHub Releases tarball fetch)
  • Branch Helm values on resolved chart major (1.x vs 2.x), not git branch name
  • Wait for install-dynamic-plugins init to finish, then check backstage-backend for expected CreateContainerConfigError (not containerStatuses[0] during PodInitializing)
  • Standalone deploy: quay.io/fedora/postgresql-15 on Kind (no registry.redhat.io pull secret)
  • Helm 4 OCI pull status (Pulled: / Digest: on stdout): add helm_template_yaml() so helm template | kubectl apply validates
  • Unit tests for chart version sort helpers (tests/e2e-chart-version.bats)

Related

Test plan

  • Konflux hermetic build succeeds with generic prefetch (downstream !697)
  • make test passes (unit + ShellCheck)
  • GitHub Actions: Build PR Container Image
  • GitHub Actions: E2E Tests (local mode)
  • GitHub Actions: E2E Tests (container image)
  • make image-build locally (optional; CI image job covers this)

Ref: https://redhat.atlassian.net/browse/RHIDP-16046

Generated-by: cursor

rhdh-bot and others added 2 commits August 12, 2026 09:21
…urce

Replace vendor/helm source tree and go-toolset compile stage with
prebuilt Helm v4.2.3 Linux binaries from mirror.openshift.com/pub/cgw/helm/.
Hermetic Konflux builds prefetch tarballs via artifacts.lock.yaml and the
Hermeto generic fetcher; the upstream Containerfile downloads by TARGETARCH.

Adds hack/update-helm-lockfile.sh for version bumps and points local-setup
helm downloads at the same CGW mirror.

https://redhat.atlassian.net/browse/RHIDP-16046

Generated-by: cursor
Co-authored-by: Cursor <cursoragent@cursor.com>
https://redhat.atlassian.net/browse/RHIDP-16046

Generated-by: cursor
Co-authored-by: Cursor <cursoragent@cursor.com>
@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Aug 12, 2026
Extract CGW helm install into a dedicated helm-builder stage (swappable
with go-toolset + vendor/helm if CGW lacks a newer release). Add
hack/install-helm-cgw-binary.sh for shared platform detection from
TARGETPLATFORM or uname; container builds require linux binaries.

Makefile local-setup downloads helm-$(OS)-$(ARCH) from CGW so darwin/arm64
dev machines get the correct binary.

https://redhat.atlassian.net/browse/RHIDP-16046

Generated-by: cursor
Co-authored-by: Cursor <cursoragent@cursor.com>
@nickboldt
nickboldt marked this pull request as ready for review August 12, 2026 12:36
@openshift-ci openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Aug 12, 2026
rhdh-bot and others added 4 commits August 12, 2026 09:42
Add helm-cgw-available.sh to probe mirror tarballs (per-platform or
linux-amd64/arm64 for lockfiles). install-helm-local.sh downloads from
CGW when available, otherwise builds from vendor/helm via go.

make vendor refreshes artifacts.lock.yaml or vendors helm source;
helm-lockfile-update errors with guidance when CGW is missing.
Restore helm support in update-vendor.sh for the source path.

https://redhat.atlassian.net/browse/RHIDP-16046

Generated-by: cursor
Co-authored-by: Cursor <cursoragent@cursor.com>
- Rename helm-cgw-available.sh to check-helm-binary-available.sh
- Rename install-helm-cgw-binary.sh to install-helm-binary.sh
- Label CGW helm-builder as Stage 2a; add commented Stage 2b for
  vendored go-toolset + vendor/helm when CGW lacks a release
- Clarify update-vendor.sh helm fallback comment

https://redhat.atlassian.net/browse/RHIDP-16046

Generated-by: cursor
Co-authored-by: Cursor <cursoragent@cursor.com>
CGW helm tarballs contain helm-linux-amd64 at the archive root, not
linux-amd64/helm. Fixes Konflux prefetch --prefetch install and local CGW
download.

https://redhat.atlassian.net/browse/RHIDP-16046

Generated-by: cursor
Co-authored-by: Cursor <cursoragent@cursor.com>
HELM_ARCHIVE_DIR used $(OS)/$(ARCH) before those variables were assigned,
producing bin/helm-4.2.3--/helm and breaking E2E local-setup on CI.
Also accept normalized amd64 in install-helm-local.sh.

https://redhat.atlassian.net/browse/RHIDP-16046

Generated-by: cursor
Co-authored-by: Cursor <cursoragent@cursor.com>
@nickboldt

Copy link
Copy Markdown
Member Author

@github-actions

Copy link
Copy Markdown
Contributor

PR images are available (for 1 week):

  1. quay.io/rhdh-community/rhdh-must-gather:pr-284
  2. quay.io/rhdh-community/rhdh-must-gather:pr-284-bb2e0e811

E2E standalone deploy on main still fetched the chart tarball from
GitHub Releases, which failed intermittently with EOF in CI. Unify
native Helm install and standalone helm template to use
oci://quay.io/rhdh/chart with an auto-resolved X.Y-N-CI tag from
Quay (sort -V so build numbers compare correctly).

https://redhat.atlassian.net/browse/RHIDP-16046

Generated-by: cursor
Co-authored-by: Cursor <cursoragent@cursor.com>
Comment thread tests/e2e/lib/test-utils.sh Fixed
rhdh-bot and others added 3 commits August 13, 2026 09:43
ShellCheck SC2034 flagged HELM_CHART_OCI_REF as unused because it was
only referenced from run-e2e-tests.sh after sourcing. Derive
HELM_CHART_SKOPEO_REF from it for skopeo list-tags calls so the
constant is used in-file and chart URLs stay in one place.

https://redhat.atlassian.net/browse/RHIDP-16046

Generated-by: cursor
Co-authored-by: Cursor <cursoragent@cursor.com>
Branch E2E values on resolved chart major (1.x vs 2.x) instead of
git branch name. Wait for install-dynamic-plugins init to finish, then
check backstage-backend for CreateContainerConfigError (not container [0]
during PodInitializing). Standalone deploys use quay.io/fedora/postgresql-15
on Kind without registry.redhat.io pull secrets.

https://redhat.atlassian.net/browse/RHIDP-16046

Generated-by: cursor
Co-authored-by: Cursor <cursoragent@cursor.com>
Helm 4 prints "Pulled:" and "Digest:" to stdout when templating
oci:// charts. Standalone E2E deploy pipes helm template output
directly to kubectl apply, which then fails validation with
"apiVersion not set, kind not set". Add helm_template_yaml() to
filter those status lines before apply.

https://redhat.atlassian.net/browse/RHIDP-16046

Generated-by: cursor
Co-authored-by: Cursor <cursoragent@cursor.com>
@github-actions

Copy link
Copy Markdown
Contributor

PR images are available (for 1 week):

  1. quay.io/rhdh-community/rhdh-must-gather:pr-284
  2. quay.io/rhdh-community/rhdh-must-gather:pr-284-bbd0fa414

@Fortune-Ndlovu

Copy link
Copy Markdown
Member

/agentic_review

@rhdh-qodo-merge

rhdh-qodo-merge Bot commented Aug 13, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (1) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Action required

1. Helm download unverified ✓ Resolved 🐞 Bug ⛨ Security
Description
Helm tarballs are downloaded and extracted in hack/install-helm-binary.sh (curl mode) and
hack/install-helm-local.sh without verifying the expected sha256, so a tampered archive could
install a malicious helm binary. This bypasses the checksum material already tracked in
artifacts.lock.yaml/sha256sum.txt.
Code

hack/install-helm-binary.sh[R68-71]

+    curl -fsSL "https://mirror.openshift.com/pub/cgw/helm/${HELM_VERSION}/${TARBALL}" \
+        -o "/tmp/${TARBALL}"
+    extract_helm "/tmp/${TARBALL}"
+    rm -f "/tmp/${TARBALL}"
Relevance

●●● Strong

Supply-chain hardening; repo already tracks Helm sha256s via lockfiles, so checksum verification
likely welcomed.

PR-#137

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The installer scripts perform curl + tar extraction with no checksum check, despite the repo
adding a lockfile containing sha256s and a helper that fetches sha256sum.txt.

hack/install-helm-binary.sh[50-72]
hack/install-helm-local.sh[35-44]
artifacts.lock.yaml[4-10]
hack/update-helm-lockfile.sh[37-48]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`hack/install-helm-binary.sh` and `hack/install-helm-local.sh` download `helm-<os>-<arch>.tar.gz` and extract it without validating the archive digest. The repo already pins digests for Linux artifacts in `artifacts.lock.yaml` and the lockfile updater reads `sha256sum.txt`, but the installer paths never check them.

### Issue Context
- Konflux prefetch path relies on `artifacts.lock.yaml` checksums, but curl/local paths currently do not.
- Prefer verifying against a *pinned* checksum (lockfile) where possible (linux/amd64, linux/arm64), rather than downloading a checksum from the same origin.

### Fix Focus Areas
- hack/install-helm-binary.sh[50-75]
- hack/install-helm-local.sh[35-64]
- artifacts.lock.yaml[1-10]
- Containerfile[15-27]

### Implementation notes
- Add checksum verification before `extract_helm`/`tar xzf`.
- For container curl-mode installs: copy `artifacts.lock.yaml` into the helm-builder stage and have the script read the expected checksum for `${TARBALL}` from it; then run `sha256sum -c` on the downloaded file.
- For local installs: if `${BUILD_OS}-${BUILD_ARCH}` is present in `artifacts.lock.yaml`, verify against it; otherwise either (a) fetch `sha256sum.txt` and verify, or (b) require an explicit expected checksum input for non-locked platforms.
- Ensure `sha256sum` is available in the image (install `coreutils` if needed on UBI minimal).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Missing tests/test_helper.bash source 📘 Rule violation ▣ Testability
Description
tests/e2e-chart-version.bats uses load 'test_helper' and does not source
tests/test_helper.bash as required, which can break shared test setup/fixtures. This violates the
requirement that all BATS tests source the common helper explicitly.
Code

tests/e2e-chart-version.bats[4]

+load 'test_helper'
Relevance

●●● Strong

Simple, deterministic compliance fix; teams typically accept standardizing test scaffolding/headers.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
PR Compliance ID 1826 requires every .bats file to include a non-commented source/. statement
that references tests/test_helper.bash. In tests/e2e-chart-version.bats, line 4 uses `load
'test_helper' and there is no source tests/test_helper.bash` statement in the file header.

Rule 1826: BATS tests must source common test_helper.bash
tests/e2e-chart-version.bats[1-5]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new BATS test `tests/e2e-chart-version.bats` does not source `tests/test_helper.bash` (it currently uses `load 'test_helper'`). The compliance rule requires an explicit `source`/`.` statement that includes `tests/test_helper.bash`.

## Issue Context
Repository contains the shared helper at `tests/test_helper.bash`; this file should be sourced near the top of every `.bats` file.

## Fix Focus Areas
- tests/e2e-chart-version.bats[1-6]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. E2E poll aborts early ✓ Resolved 🐞 Bug ☼ Reliability
Description
wait_for_helm_misconfigured_backstage_pods() uses kubectl | wc / kubectl | jq pipelines inside
assignments without tolerating non-zero kubectl exits, but run-e2e-tests.sh runs with `set -euo
pipefail`. A transient API error (or brief not-found during rollout) will therefore terminate the
E2E run instead of continuing to poll.
Code

tests/e2e/lib/test-utils.sh[R402-405]

+    while [ "$SECONDS" -lt "$deadline" ]; do
+        local count
+        count=$(kubectl -n "$namespace" get pods -l "$selector" -o jsonpath='{.items[*].metadata.name}' 2>/dev/null | wc -w)
+        if [ "$count" -ge "$expected_count" ]; then
Relevance

●● Moderate

Reliability improvement but behavior change in E2E polling; no close precedent for tolerating
kubectl failures under pipefail.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The helper’s kubectl pipelines are executed in a shell with errexit+pipefail enabled (set by
the main runner). Under these options, any non-zero kubectl status will exit the script at the
assignment site.

tests/e2e/run-e2e-tests.sh[33-40]
tests/e2e/lib/test-utils.sh[396-405]
tests/e2e/lib/test-utils.sh[426-438]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The new polling helper runs under the caller's `set -euo pipefail` and uses pipelines inside command substitutions/assignments. With `pipefail`, any non-zero `kubectl` exit makes the whole assignment fail and triggers `errexit`, aborting E2E instead of retrying.

### Issue Context
`tests/e2e/run-e2e-tests.sh` enables `set -euo pipefail` before sourcing `tests/e2e/lib/test-utils.sh`, so functions inherit these options.

### Fix Focus Areas
- tests/e2e/run-e2e-tests.sh[33-40]
- tests/e2e/lib/test-utils.sh[402-449]

### Implementation notes
- Avoid `kubectl ... | wc -w` directly under pipefail. Instead:
 - `pods=$(kubectl ... 2>/dev/null || true); count=$(wc -w <<<"$pods")`
- For `kubectl ... | jq ...` queries, first capture pod JSON with `kubectl ... -o json 2>/dev/null || { all_ready=false; continue; }`, then run `jq` on the captured JSON (or add `|| true` and treat empty as not-ready).
- Ensure the polling loop continues on transient kubectl errors, but still fails fast on confirmed init pull errors (the existing describe+return path can remain).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context
⚠️ Tickets: not configured — ticket URL found in PR but could not be fetched — check ticket provider credentials
✅ Compliance rules (platform): 7 rules
✅ Cross-repo context
  Explored: repo: redhat-developer/rhdh-chart (sha: ea6b1aef)

Grey Divider

Tip of the day
💡 Did you know, you can type 'qodo, fix this' on a finding and the fix lands right on your PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread tests/e2e-chart-version.bats
Comment thread hack/install-helm-binary.sh
Comment thread tests/e2e/lib/test-utils.sh
@Fortune-Ndlovu

Fortune-Ndlovu commented Aug 13, 2026

Copy link
Copy Markdown
Member

Hey @nickboldt, just flagging the dependency chain across the PRs on this initiative (RHIDP-16046):

  1. chore(build): add RPM lockfile workflow and vendor Helm from source [RHIDP-16046] #178 (@rm3l , merged Aug 6), vendored Helm from source, similar to how websocat is handled
  2. refactor(build): install Helm from CGW mirror instead of vendoring source #284 (this PR), builds on 178 by replacing the vendored source tree with the lighter CGW binary approach
  3. feat(ci): build must-gather image hermetically in upstream repo [RHIDP-14412] #218 (mine), adds a hermetic build pipeline with Hermeto, which touches overlapping Containerfile and build infrastructure

Your PR supersedes the Helm-related parts of both 178 and 218. Once this lands, I'll rebase my 218 to reconcile with your CGW binary approach instead of the COPY bin/helm method I had. No action needed on your side, just wanted to make sure we're aware of the overlap so we don't step on each other during merge.

@Fortune-Ndlovu Fortune-Ndlovu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for pushing the PR @nickboldt added a few nits

Comment thread hack/install-helm-binary.sh Outdated
Comment thread hack/install-helm-binary.sh
Comment thread hack/install-helm-local.sh
Comment thread hack/install-helm-binary.sh Outdated
rhdh-bot and others added 2 commits August 13, 2026 12:55
Address Qodo and review nits on the CGW binary install path: verify
curl downloads against artifacts.lock.yaml (or CGW sha256sum.txt),
parse TARGETPLATFORM arch as field 2 for BuildKit variants like
linux/arm64/v8, absolutize local OUTPUT_PATH before vendor go build,
and tolerate transient kubectl failures in the misconfigured-pod wait
loop under set -o pipefail.

https://redhat.atlassian.net/browse/RHIDP-16046

Generated-by: cursor
Co-authored-by: Cursor <cursoragent@cursor.com>
UBI minimal already ships coreutils-single (provides sha256sum).
Installing coreutils conflicts with it and fails the image build.

https://redhat.atlassian.net/browse/RHIDP-16046

Generated-by: cursor
Co-authored-by: Cursor <cursoragent@cursor.com>
@github-actions

Copy link
Copy Markdown
Contributor

PR images are available (for 1 week):

  1. quay.io/rhdh-community/rhdh-must-gather:pr-284
  2. quay.io/rhdh-community/rhdh-must-gather:pr-284-8557526e2

@Fortune-Ndlovu
Fortune-Ndlovu merged commit 8584f90 into main Aug 13, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants