Skip to content

feat(ci): add disconnected OCP smoke test for Helm and Operator - #5268

Merged
openshift-merge-bot[bot] merged 26 commits into
redhat-developer:mainfrom
zdrapela:refactor/disconnected-ci-modular
Aug 27, 2026
Merged

feat(ci): add disconnected OCP smoke test for Helm and Operator#5268
openshift-merge-bot[bot] merged 26 commits into
redhat-developer:mainfrom
zdrapela:refactor/disconnected-ci-modular

Conversation

@zdrapela

@zdrapela zdrapela commented Aug 18, 2026

Copy link
Copy Markdown
Member

Jira: https://redhat.atlassian.net/browse/RHIDP-13974

Human written


AI Generated

Mirrors RHDH images, chart, operator, and dynamic plugins into an isolated mirror registry on a disconnected OCP cluster, deploys RHDH, and runs a Playwright smoke test (guest login → homepage). Covers both Helm and Operator installs, each runnable in CI (Prow bastion mirror) and Local (LOCAL_DISCONNECTED=1, cluster integrated registry).

Path selection

flowchart TD
  CI["CI: openshift-ci-tests.sh<br/>*disconnected*{helm,operator}*nightly*"] --> F
  L["Local: local-run.sh<br/>detects *disconnected*, requires oc login<br/>sets DISCONNECTED=true, LOCAL_DISCONNECTED=1"] --> F
  F["lib/disconnected.sh (facade)<br/>sources env/mirror/plugins/namespace/operator/helm"] --> D{LOCAL_DISCONNECTED=1?}
  D -- no --> CIH["CI no-op hooks<br/>external MIRROR_REGISTRY_URL"]
  D -- yes --> LO["source lib/disconnected/local.sh<br/>overrides _hook_* + integrated registry"]
  CIH --> H["handle_ocp_disconnected_helm"]
  CIH --> O["handle_ocp_disconnected_operator"]
  LO --> H
  LO --> O
Loading

Paths

Helm. Validate env + auth → pull chart (GA from charts.openshift.io, CI from OCI) → oc-mirror the hub, PostgreSQL, and catalog-index images → apply IDMS/ITMS and wait for MachineConfigPools → mirror plugins via mirror-plugins.sh and re-pin homepage/catalog-index digests → create mirror CA + registry-auth secret and plugin-mirror ConfigMap → helm upgrade -i with a post-renderer that appends disconnected volumes (registries.conf, mirror CA) to the rendered Deployment → EBS-safe hub recovery → Playwright smoke.

Operator. Validate env + auth → fetch and run prepare-restricted-environment.sh with --use-oc-mirror (mirrors operator/operand images, installs the OLM v1 CatalogSource) → wait for ClusterCatalog Serving=True and the Backstage CRD → mirror plugins via mirror-plugins.sh and re-pin digests → create mirror CA + auth + policy.json and homepage/plugin-mirror ConfigMaps → render the Backstage CR from an envsubst template (extraFiles/extraEnvs for disconnected mounts) → deploy_rhdh_operator → Playwright smoke.

CI vs Local. CI uses an external bastion mirror (MIRROR_REGISTRY_URL) and prepare's default flags plus external CA/pull-secret helpers. Local uses the cluster's integrated registry via lib/disconnected/local/: registry.sh brings it up (MIRROR_* bootstrap, amd64 skopeo shim), mirror-host.sh rewrites the push-route IDMS to the in-cluster registry service, access.sh creates push-target projects and workload/OLM pull grants, plugins.sh fully overrides plugin mirroring (digest list + imagestream tags), and hooks.sh supplies the _hook_* / cluster_mirror_host overrides.

Key decisions

  • Modular facade + hook/overridelib/disconnected.sh sources concern-based modules; CI defines no-op _hook_*, local.sh overrides them for integrated-registry behaviour. Keeps CI functions clean.
  • Reuse rhdh-operator scripts instead of reimplementing mirroring (see below).
  • oc-mirror over podman — nested Podman in the CI pod cannot init storage (newuidmap/VFS chown fail under hostUsers: false), so the operator path forces --use-oc-mirror.
  • Helm post-renderer — a static values file would clobber the chart's whole extraVolumes[]; the post-renderer appends disconnected volumes to the rendered Deployment instead.
  • Template-driven YAML — inline heredocs/yq replaced with envsubst templates (Backstage CR, homepage dynamic-plugins, post-renderer patch, skopeo shim).
  • Shared CATALOG_INDEX_IMAGE contract — CI and Local mirror and consume the same index; homepage/catalog-index re-pinned to the actually-mirrored digest.
  • EBS-safe hub recovery — rolling oc rollout restart (not oc delete pod) so the PostgreSQL StatefulSet is never disturbed (avoids ~5 min EBS re-attach).
  • TLS/auth/signatures — per-registry mirror CA mount, chart registry-auth Secret, and permissive policy.json for unsigned mirrored images.

rhdh-operator scripts

Both are fetched at runtime by disconnected::fetch_operator_repo_script from redhat-developer/rhdh-operator .rhdh/scripts/ on refs/heads/${RELEASE_BRANCH_NAME} (defaults to main locally):

Script Used by Purpose
mirror-plugins.sh Helm + Operator Mirror dynamic plugins and the catalog index into the mirror registry
prepare-restricted-environment.sh Operator only Mirror operator/operand images and install the OLM v1 CatalogSource

Companion PR

@openshift-ci

openshift-ci Bot commented Aug 18, 2026

Copy link
Copy Markdown

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@zdrapela

Copy link
Copy Markdown
Member Author

/agentic_review

@codecov

codecov Bot commented Aug 18, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.69%. Comparing base (d814f4b) to head (7ffd43c).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5268      +/-   ##
==========================================
- Coverage   63.68%   59.69%   -3.99%     
==========================================
  Files         121      109      -12     
  Lines        2288     2062     -226     
  Branches      527      500      -27     
==========================================
- Hits         1457     1231     -226     
  Misses        829      829              
  Partials        2        2              
Flag Coverage Δ
rhdh 59.69% <ø> (-3.99%) ⬇️
Components Coverage Δ
Backend plugins ∅ <ø> (∅)
Backend app 69.69% <ø> (ø)
Frontend app 58.82% <ø> (ø)
Plugin utils ∅ <ø> (∅)

Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d814f4b...7ffd43c. Read the comment docs.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@rhdh-qodo-merge

rhdh-qodo-merge Bot commented Aug 18, 2026

Copy link
Copy Markdown

Code Review by Qodo

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

Grey Divider


Action required

1. Wrong tar flags ✓ Resolved 🐞 Bug ≡ Correctness ⭐ New
Description
disconnected::resolve_homepage_plugin_package extracts index.json from a skopeo copy ... dir:
layer using tar -xzf, but the same dir: extraction flow elsewhere treats the layer as a plain
tar archive (tar -xf). This can make homepage plugin resolution fail, preventing creation of the
homepage-only dynamic plugins YAML and breaking disconnected Helm/Operator smokes.
Code

.ci/pipelines/lib/disconnected/plugins.sh[R217-220]

+  mkdir -p "${tmp_dir}/extracted"
+  tar -xzf "${tmp_dir}/oci/${layer_digest#sha256:}" -C "${tmp_dir}/extracted" index.json || {
+    log::error "Failed to extract index.json from catalog index ${index_ref}"
+    return 1
Relevance

●●● Strong

This is a deterministic archive-format bug in a new disconnected path; accepted history favors
concrete CI failure fixes.

PR-#4288
PR-#4679

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The global homepage resolver extracts from a skopeo dir: layer using tar -xzf, while the local
disconnected catalog-index extraction iterates dir: layer files and extracts them with tar -xf,
implying the layer blobs are plain tar archives in this workflow. This inconsistency means the
homepage resolver can fail when run against the same type of dir: output.

.ci/pipelines/lib/disconnected/plugins.sh[203-221]
.ci/pipelines/lib/disconnected/local/plugins.sh[122-136]

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

## Issue description
`disconnected::resolve_homepage_plugin_package` uses `tar -xzf` to extract `index.json` from a layer file produced by `skopeo copy ... dir:`. In the local disconnected implementation, those layer files are treated as *non-gzipped* tar archives (`tar -xf`). Using `-z` can cause extraction to fail and abort the disconnected flow.

## Issue Context
This function is required to compute `HOMEPAGE_PLUGIN_PACKAGE` / `HOMEPAGE_PLUGIN_FRONTEND_ID`, which are then used to render the homepage-only dynamic plugins YAML for both Helm and Operator disconnected deployments.

## Fix Focus Areas
- Update extraction to not assume gzip compression (use `tar -xf`, or attempt `-xf` first and fall back to `-xzf` only if needed).
- Keep behavior consistent with the existing `dir:` extraction logic in the LOCAL_DISCONNECTED path.

### Files/lines
- .ci/pipelines/lib/disconnected/plugins.sh[203-221]
- .ci/pipelines/lib/disconnected/local/plugins.sh[122-136]

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


2. GA chart name mismatch 🔗 Cross-repo conflict ≡ Correctness ⭐ New
Description
The disconnected Helm GA path pulls openshift-helm-charts/redhat-developer-hub, but the rhdh-chart
snapshot’s chart name is backstage (and docs instruct installing .../backstage). If
charts.openshift.io publishes the rhdh-chart content under its actual chart name, this job will fail
to pull the chart and the disconnected Helm smoke will break.
Code

.ci/pipelines/jobs/ocp-disconnected-helm.sh[R42-46]

+    helm repo add openshift-helm-charts https://charts.openshift.io 2> /dev/null || true
+    helm repo update openshift-helm-charts
+    log::info "Pulling GA chart from charts.openshift.io (version: ${RELEASE_VERSION})"
+    helm pull openshift-helm-charts/redhat-developer-hub \
+      --version "${RELEASE_VERSION}" \
Relevance

●●● Strong

Accepted history favors concrete CI correctness fixes; chart identifier mismatch would directly fail
Helm pull.

PR-#4288
PR-#4685

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The PR hardcodes helm pull openshift-helm-charts/redhat-developer-hub, while rhdh-chart’s
Chart.yaml declares name: backstage and the chart README shows installation via .../backstage,
indicating the expected chart identifier is backstage rather than redhat-developer-hub.

.ci/pipelines/jobs/ocp-disconnected-helm.sh[41-46]
External repo: redhat-developer/rhdh-chart, charts/backstage/Chart.yaml [41-44]
External repo: redhat-developer/rhdh-chart, charts/backstage/README.md [28-33]

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 disconnected Helm GA flow pulls `openshift-helm-charts/redhat-developer-hub`, but the upstream rhdh-chart chart name in this ecosystem is `backstage`. If the GA chart in `charts.openshift.io` is published using the chart’s real name, the current pull target won’t exist and the disconnected Helm job will fail early.

### Issue Context
- PR adds a GA-path chart pull from `charts.openshift.io`.
- The related repo `redhat-developer/rhdh-chart` (pinned in this review) defines the chart name as `backstage` and documents installs using `.../backstage`.

### Fix Focus Areas
- .ci/pipelines/jobs/ocp-disconnected-helm.sh[41-46]

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


3. Backstage extraFiles mountPath wrong 🔗 Cross-repo conflict ≡ Correctness
Description
The disconnected Backstage CR template sets spec.application.extraFiles[*].mountPath to
directories (e.g., /etc/containers), but rhdh-operator v1alpha5 defines mountPath as the path to
mount the file (full file path) and its integration tests validate mounts as <baseMountPath>/<key>
unless an explicit full-path mount is given. This mismatch can prevent required files
(registries.conf, policy.json, CA cert, auth.json) from being mounted correctly, breaking
operator-based disconnected plugin installation in rhdh-operator.
Code

.ci/pipelines/resources/rhdh-operator/rhdh-start-disconnected-smoke.yaml[R46-49]

+        - name: rhdh-plugin-mirror-conf
+          key: rhdh-registries.conf
+          mountPath: /etc/containers/registries.conf.d
+          containers:
Relevance

●●● Strong

This is a concrete operator-schema mismatch affecting required file mounts; no contrary repository
precedent was found.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The PR’s Backstage CR sets mountPath values to directories; rhdh-operator v1alpha5 defines mountPath
as the path to mount the object (file), and its integration tests assert mounts appear at
<baseMountPath>/<key> or at the explicit full-path mountPath when provided—indicating
directory-only mountPaths are incorrect for single-file mounts.

.ci/pipelines/resources/rhdh-operator/rhdh-start-disconnected-smoke.yaml[40-66]
External repo: redhat-developer/rhdh-operator, api/v1alpha5/backstage_types.go [209-229]
External repo: redhat-developer/rhdh-operator, integration_tests/cr-config_test.go [88-101]
External repo: redhat-developer/rhdh-operator, integration_tests/cr-config_test.go [158-166]

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 Backstage CR template for the disconnected operator smoke test uses `spec.application.extraFiles.*[].mountPath` as directory paths (e.g., `/etc/containers`). In rhdh-operator v1alpha5, `mountPath` is treated as the *file mount location* (full path), or else the operator mounts files as `<extraFiles.mountPath>/<key>` when per-item `mountPath` is not set.

As written, mounts may attempt to mount a file onto an existing directory path (e.g. `/etc/containers` or `/tmp`), which can fail at runtime and prevent `install-dynamic-plugins` from reading `registries.conf`, `policy.json`, CA cert, and `auth.json`.

### Issue Context
This CR is consumed by **redhat-developer/rhdh-operator** (Backstage CRD `rhdh.redhat.com/v1alpha5`). The operator’s API/docs and integration tests show that file mounts are expected as either:
- `<extraFiles.mountPath>/<key>` when using a base mountPath, or
- the exact `FileObjectRef.mountPath` when explicitly set (as a full file path).

### Fix Focus Areas
- .ci/pipelines/resources/rhdh-operator/rhdh-start-disconnected-smoke.yaml[40-66]

### Suggested change
Update the per-item `mountPath` values to be full file paths, e.g.:
- `rhdh-registries.conf` -> `/etc/containers/registries.conf.d/rhdh-registries.conf`
- `policy.json` -> `/etc/containers/policy.json`
- `ca.crt` -> `/etc/containers/certs.d/${MIRROR_REGISTRY_URL}/ca.crt`
- `auth.json` -> `/tmp/auth.json`

Alternatively, set `spec.application.extraFiles.mountPath` to a base dir and omit per-item `mountPath` when appropriate, but because these files must land in different directories, explicit full paths are the simplest/clearest approach.

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


View high (2)
4. Proxy comment includes user:pass ✓ Resolved 📘 Rule violation ⛨ Security
Description
e2e-tests/playwright/utils/proxy.ts includes an example proxy URL containing credential-like
user:pass in a comment, which can be interpreted as hardcoded credentials and may trigger secret
scanning. Use a redacted or placeholder format that does not resemble real credentials.
Code

e2e-tests/playwright/utils/proxy.ts[R5-6]

+ * fields, but CI environments typically set a single `HTTPS_PROXY` URL like
+ * `http://user:pass@host:3128`. This helper splits the URL into the shape
Relevance

●●● Strong

Removing credential-like text from source comments is a straightforward security-compliance fix; no
contrary precedent was found.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
PR Compliance ID 2137 prohibits hardcoded secrets/credentials in source files. The newly added
comment contains a credential-like user:pass segment in a URL example, which resembles embedded
credentials.

Rule 2137: Secrets must not be hardcoded in source files
e2e-tests/playwright/utils/proxy.ts[5-6]

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

## Issue description
A comment includes a credential-like proxy URL (`http://user:pass@host:3128`), which can be treated as hardcoded credentials.

## Issue Context
The compliance rules require that secrets/credentials not be present in committed code, including test/support files and comments.

## Fix Focus Areas
- e2e-tests/playwright/utils/proxy.ts[5-6]

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


5. Post-renderer misses chart Deployment 🔗 Cross-repo conflict ≡ Correctness
Description
The Helm post-renderer tries to target the RHDH Deployment by checking for the literal name
"*-developer-hub", but rhdh-chart renders the Deployment name via a computed fullname (typically
"<release>-developer-hub"). As a result, the post-renderer never matches and therefore never injects
the disconnected volumes/mounts (registries.conf, policy.json, CA), breaking Helm-based disconnected
deployments (including plugin installation/TLS trust).
Code

.ci/pipelines/resources/disconnected/helm-post-renderer.sh[R29-31]

+yq eval "
+  (select(.kind == \"Deployment\" and .metadata.name == \"*-developer-hub\") |
+    .spec.template.spec.volumes += [
Relevance

●●● Strong

Literal wildcard equality cannot match computed Deployment names; selector correction is
deterministic.

PR-#4288

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
In .ci/pipelines/resources/disconnected/helm-post-renderer.sh, the yq selector uses strict
string equality (==) against a name containing * (i.e., `select(.kind == "Deployment" and
.metadata.name == "*-developer-hub")), which means *` is treated as a literal character rather
than a wildcard or glob. Meanwhile, the external redhat-developer/rhdh-chart Deployment template
sets .metadata.name using {{ include "common.names.fullname" . }} (producing names like
${RELEASE_NAME}-developer-hub) and applies stable labels such as
app.kubernetes.io/component=backstage; therefore the selector will not match the actual rendered
Deployment and the disconnected mount/volume injection the Helm job relies on will not occur.

.ci/pipelines/resources/disconnected/helm-post-renderer.sh[27-40]
.ci/pipelines/resources/disconnected/helm-post-renderer.sh[29-40]
.ci/pipelines/jobs/ocp-disconnected-helm.sh[225-237]
.ci/pipelines/lib/disconnected/helm.sh[30-32]
External repo: redhat-developer/rhdh-chart, charts/backstage/vendor/backstage/charts/backstage/templates/backstage-deployment.yaml [11-18]

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

## Issue description
`.ci/pipelines/resources/disconnected/helm-post-renderer.sh` uses `yq` to select the RHDH Deployment with an expression equivalent to:

```sh
select(.kind == "Deployment" and .metadata.name == "*-developer-hub")
```

In `yq`, `==` is strict string equality, so the `*` is not a wildcard; because `rhdh-chart` renders the Deployment name via a computed fullname (typically `<release>-developer-hub`), this selector never matches and the disconnected volumes/mounts (registries.conf, policy.json, mirror CA) and related initContainer mounts are never injected.

## Issue Context
The disconnected Helm smoke/job path in this repo consumes the external `redhat-developer/rhdh-chart` chart, so the post-renderer must identify the correct Deployment using stable chart-defined selectors (e.g., labels like `app.kubernetes.io/component=backstage`) rather than relying on a guessed/wildcarded name. The Helm disconnected job depends on the post-renderer during `helm upgrade -i` to add CA + registry configuration mounts needed for disconnected plugin installation and TLS trust.

## Fix Focus Areas
- .ci/pipelines/resources/disconnected/helm-post-renderer.sh[29-40]

## Suggested change
Update the `yq` selector to use a robust match:
- Prefer label-based matching, e.g. select the Deployment with `app.kubernetes.io/component=backstage` (optionally also filtering by `app.kubernetes.io/instance`).
- If suffix matching is required, use `endswith("-developer-hub")` or a regex like `test(".*-developer-hub$")` instead of a literal `*`.

## Implementation notes
Make the patch resilient to missing arrays/fields:
- Initialize `.spec.template.spec.volumes` and the target container/initContainer `volumeMounts` if null.
- Prefer locating the initContainer by name rather than assuming it is always at index `[0]`.

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



Remediation recommended

6. RETURN trap clobbered 🐞 Bug ☼ Reliability
Description
disconnected::resolve_homepage_plugin_package sets a global RETURN trap for tmp_dir cleanup but
never restores/chains any existing RETURN trap, so other cleanup traps in the same shell can be
overwritten and won’t run. The new trap also persists after the function returns, causing unexpected
repeated execution on subsequent function returns.
Code

.ci/pipelines/lib/disconnected/plugins.sh[R199-202]

+  }
+  # shellcheck disable=SC2064 # expand tmp_dir now, not at trap-firing time
+  trap "rm -rf '${tmp_dir}'" RETURN
+
Relevance

●●● Strong

Exact recent precedent accepted RETURN-trap clobbering cleanup finding in PR #4679.

PR-#4679

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The function sets a RETURN trap for tmp_dir cleanup, and because this library is sourced into the
main disconnected handler shell, that trap is global and can override any earlier RETURN trap
handlers in the job shell (a known recurring bug pattern).

.ci/pipelines/lib/disconnected/plugins.sh[195-207]
.ci/pipelines/lib/disconnected.sh[20-33]
PR-#4679

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

### Issue description
`disconnected::resolve_homepage_plugin_package` installs a `trap ... RETURN` to clean up a temp dir but does not preserve any pre-existing RETURN trap handler, and it does not restore the previous handler after cleanup. This can break other cleanup logic (same pattern as a previously accepted bug), and the trap continues firing for later function returns.

### Issue Context
This file is sourced into the main CI shell via `lib/disconnected.sh`, so a RETURN trap here affects the entire job shell after the function runs.

### Fix Focus Areas
- `.ci/pipelines/lib/disconnected/plugins.sh[195-207]`

### Suggested fix approach
Use a trap-chaining pattern:
1. Capture any existing RETURN trap (`trap -p RETURN`).
2. Install a new RETURN trap that:
  - removes the temp dir
  - restores the prior RETURN trap (or runs it) so behavior is preserved

Alternative (often simpler/safer): avoid RETURN traps entirely by using an explicit cleanup function and a single `return` path (or a `cleanup` + `rc` pattern) so temp cleanup is always executed without touching global traps.

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


7. Pull-secret clobbered ✓ Resolved 🐞 Bug ☼ Reliability
Description
disconnected::ensure_olm_mirror_pull_secret rebuilds the cluster pull-secret as a JSON object
containing only auths, discarding any other docker config fields and potentially failing if either
input lacks an auths object. This can break cluster-wide registry auth (and/or abort the operator
install flow) after partially mutating openshift-config/pull-secret.
Code

.ci/pipelines/lib/disconnected/namespace.sh[R103-105]

+  merged=$(jq -n --argjson existing "${existing}" --argjson mirror "${mirror_auth}" \
+    '{auths: ($existing.auths + $mirror.auths)}') || {
+    log::error "Failed to merge mirror credentials into pull-secret JSON"
Relevance

●●● Strong

The merge is lossy and null-unsafe; preserving dockerconfig fields is a clear reliability fix.

PR-#4679

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The jq expression explicitly constructs a new object with only auths and then writes it back to
openshift-config/pull-secret, which is both lossy and not null-safe.

.ci/pipelines/lib/disconnected/namespace.sh[94-114]

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

### Issue description
`disconnected::ensure_olm_mirror_pull_secret` constructs `{auths: (...)}` which drops other valid dockerconfigjson fields (e.g., `credHelpers`, `credsStore`, etc.) and uses `+` on `.auths` without null-safety.

### Issue Context
This function writes back to the cluster-wide `openshift-config/pull-secret`, so robustness is important to avoid breaking unrelated cluster pulls.

### Fix Focus Areas
- .ci/pipelines/lib/disconnected/namespace.sh[91-114]

### Implementation notes
- Preserve the original JSON object and only merge/override `.auths`:
 - Use `($existing.auths // {})` and `($mirror.auths // {})`.
 - Keep other top-level keys from `$existing`.
- Example jq shape (conceptual):
 - `jq -n --argjson existing ... --argjson mirror ... '$existing as $e | $mirror as $m | ($e * $m) | .auths = (($e.auths // {}) + ($m.auths // {}))'`
- Optionally validate that `.auths` is an object before merging.

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


8. Homepage summary parse fails ✓ Resolved 🐞 Bug ☼ Reliability
Description
disconnected::resolve_homepage_plugin_package assumes the mirroring summary uses the Unicode 
separator and errors if it does not, even though other code in this PR explicitly supports both 
and ->. If mirror-plugins.sh outputs ASCII ->, homepage plugin resolution will abort and the
disconnected deploy cannot generate the homepage-only dynamic-plugins YAML.
Code

.ci/pipelines/lib/disconnected/plugins.sh[R187-190]

+  left="${line%%→*}"
+  if [[ "${left}" == "${line}" ]]; then
+    log::error "Summary line for ${name} has no separator: ${line}"
+    return 1
Relevance

●●● Strong

ASCII separator support is an obvious consistency fix matching the adjacent parser.

PR-#4288

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The homepage resolver hard-fails when  isn’t present, while local disconnected summary parsing in
this PR is explicitly written to handle both  and ->, demonstrating inconsistent assumptions
about the summary format.

.ci/pipelines/lib/disconnected/plugins.sh[171-190]
.ci/pipelines/lib/disconnected/local.sh[698-706]

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

### Issue description
`resolve_homepage_plugin_package` splits summary lines using only the Unicode `→` separator and treats missing `→` as fatal. The repository already contains parsing logic that supports both `→` and `->`, so the resolver should do the same.

### Issue Context
The homepage resolver is on the critical path for both Helm and Operator disconnected smokes.

### Fix Focus Areas
- .ci/pipelines/lib/disconnected/plugins.sh[171-200]

### Implementation notes
- Implement a shared helper to split summary lines that:
 - accepts either `→` or `->`
 - trims whitespace on both sides
 - returns a clear error only when neither separator is present.
- Update both homepage and (optionally) catalog-index resolvers to use the same helper for consistency.

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



Informational

9. Misleading MCP success log ✓ Resolved 🐞 Bug ◔ Observability
Description
disconnected::wait_mcp_updated logs "All MachineConfigPools are Updated" even when oc wait timed
out, which misrepresents cluster state and complicates debugging rollout-related flakes. The success
log should be conditional on the wait succeeding.
Code

.ci/pipelines/lib/disconnected/mirror.sh[R245-249]

+  log::info "Waiting for MachineConfigPool updates to complete (up to 20m)..."
+  if ! oc wait machineconfigpool --all --for=condition=Updated=True --timeout=20m; then
+    log::warn "MachineConfigPool wait timed out -- proceeding anyway"
+  fi
+  log::success "All MachineConfigPools are Updated"
Relevance

●●● Strong

Conditional success logging is a trivial, deterministic observability fix.

PR-#4288

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The function always emits a success message regardless of whether the oc wait succeeded or timed
out.

.ci/pipelines/lib/disconnected/mirror.sh[242-250]

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

### Issue description
`wait_mcp_updated` warns on timeout but then unconditionally logs a success message stating all MCPs are updated.

### Issue Context
The function is intentionally warn-and-continue, but logs should not contradict the wait result.

### Fix Focus Areas
- .ci/pipelines/lib/disconnected/mirror.sh[244-250]

### Implementation notes
- Only log `log::success "All MachineConfigPools are Updated"` when `oc wait` succeeds.
- On timeout/failure, log a neutral completion message (or keep only the warning).

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


Grey Divider

Context sources
⚠️ Tickets: not configured — ticket URL found in PR but could not be fetched — check ticket provider credentials
✅ Compliance rules (platform): 46 rules
✅ Cross-repo context — repo relationships
  Explored: repo: redhat-developer/rhdh-chart (sha: 0e081376)
  Explored: repo: redhat-developer/rhdh-operator (sha: 3513f811)

Grey Divider

Tip of the day
💡 Did you know, you can ask Qodo to dismiss a finding you disagree with, with your reason on record

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Previous reviews

Review updated until commit 7ffd43c

Results up to commit d4a33ef ⚖️ Balanced


🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (1) 📜 Skill insights (0)


Action required
1. Post-renderer misses chart Deployment 🔗 Cross-repo conflict ≡ Correctness
Description
The Helm post-renderer tries to target the RHDH Deployment by checking for the literal name
"*-developer-hub", but rhdh-chart renders the Deployment name via a computed fullname (typically
"<release>-developer-hub"). As a result, the post-renderer never matches and therefore never injects
the disconnected volumes/mounts (registries.conf, policy.json, CA), breaking Helm-based disconnected
deployments (including plugin installation/TLS trust).
Code

.ci/pipelines/resources/disconnected/helm-post-renderer.sh[R29-31]

+yq eval "
+  (select(.kind == \"Deployment\" and .metadata.name == \"*-developer-hub\") |
+    .spec.template.spec.volumes += [
Relevance

●●● Strong

Literal wildcard equality cannot match computed Deployment names; selector correction is
deterministic.

PR-#4288

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
In .ci/pipelines/resources/disconnected/helm-post-renderer.sh, the yq selector uses strict
string equality (==) against a name containing * (i.e., `select(.kind == "Deployment" and
.metadata.name == "*-developer-hub")), which means *` is treated as a literal character rather
than a wildcard or glob. Meanwhile, the external redhat-developer/rhdh-chart Deployment template
sets .metadata.name using {{ include "common.names.fullname" . }} (producing names like
${RELEASE_NAME}-developer-hub) and applies stable labels such as
app.kubernetes.io/component=backstage; therefore the selector will not match the actual rendered
Deployment and the disconnected mount/volume injection the Helm job relies on will not occur.

.ci/pipelines/resources/disconnected/helm-post-renderer.sh[27-40]
.ci/pipelines/resources/disconnected/helm-post-renderer.sh[29-40]
.ci/pipelines/jobs/ocp-disconnected-helm.sh[225-237]
.ci/pipelines/lib/disconnected/helm.sh[30-32]
External repo: redhat-developer/rhdh-chart, charts/backstage/vendor/backstage/charts/backstage/templates/backstage-deployment.yaml [11-18]

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

## Issue description
`.ci/pipelines/resources/disconnected/helm-post-renderer.sh` uses `yq` to select the RHDH Deployment with an expression equivalent to:

```sh
select(.kind == "Deployment" and .metadata.name == "*-developer-hub")
```

In `yq`, `==` is strict string equality, so the `*` is not a wildcard; because `rhdh-chart` renders the Deployment name via a computed fullname (typically `<release>-developer-hub`), this selector never matches and the disconnected volumes/mounts (registries.conf, policy.json, mirror CA) and related initContainer mounts are never injected.

## Issue Context
The disconnected Helm smoke/job path in this repo consumes the external `redhat-developer/rhdh-chart` chart, so the post-renderer must identify the correct Deployment using stable chart-defined selectors (e.g., labels like `app.kubernetes.io/component=backstage`) rather than relying on a guessed/wildcarded name. The Helm disconnected job depends on the post-renderer during `helm upgrade -i` to add CA + registry configuration mounts needed for disconnected plugin installation and TLS trust.

## Fix Focus Areas
- .ci/pipelines/resources/disconnected/helm-post-renderer.sh[29-40]

## Suggested change
Update the `yq` selector to use a robust match:
- Prefer label-based matching, e.g. select the Deployment with `app.kubernetes.io/component=backstage` (optionally also filtering by `app.kubernetes.io/instance`).
- If suffix matching is required, use `endswith("-developer-hub")` or a regex like `test(".*-developer-hub$")` instead of a literal `*`.

## Implementation notes
Make the patch resilient to missing arrays/fields:
- Initialize `.spec.template.spec.volumes` and the target container/initContainer `volumeMounts` if null.
- Prefer locating the initContainer by name rather than assuming it is always at index `[0]`.

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



Remediation recommended
2. Pull-secret clobbered ✓ Resolved 🐞 Bug ☼ Reliability
Description
disconnected::ensure_olm_mirror_pull_secret rebuilds the cluster pull-secret as a JSON object
containing only auths, discarding any other docker config fields and potentially failing if either
input lacks an auths object. This can break cluster-wide registry auth (and/or abort the operator
install flow) after partially mutating openshift-config/pull-secret.
Code

.ci/pipelines/lib/disconnected/namespace.sh[R103-105]

+  merged=$(jq -n --argjson existing "${existing}" --argjson mirror "${mirror_auth}" \
+    '{auths: ($existing.auths + $mirror.auths)}') || {
+    log::error "Failed to merge mirror credentials into pull-secret JSON"
Relevance

●●● Strong

The merge is lossy and null-unsafe; preserving dockerconfig fields is a clear reliability fix.

PR-#4679

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The jq expression explicitly constructs a new object with only auths and then writes it back to
openshift-config/pull-secret, which is both lossy and not null-safe.

.ci/pipelines/lib/disconnected/namespace.sh[94-114]

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

### Issue description
`disconnected::ensure_olm_mirror_pull_secret` constructs `{auths: (...)}` which drops other valid dockerconfigjson fields (e.g., `credHelpers`, `credsStore`, etc.) and uses `+` on `.auths` without null-safety.

### Issue Context
This function writes back to the cluster-wide `openshift-config/pull-secret`, so robustness is important to avoid breaking unrelated cluster pulls.

### Fix Focus Areas
- .ci/pipelines/lib/disconnected/namespace.sh[91-114]

### Implementation notes
- Preserve the original JSON object and only merge/override `.auths`:
 - Use `($existing.auths // {})` and `($mirror.auths // {})`.
 - Keep other top-level keys from `$existing`.
- Example jq shape (conceptual):
 - `jq -n --argjson existing ... --argjson mirror ... '$existing as $e | $mirror as $m | ($e * $m) | .auths = (($e.auths // {}) + ($m.auths // {}))'`
- Optionally validate that `.auths` is an object before merging.

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


3. Homepage summary parse fails ✓ Resolved 🐞 Bug ☼ Reliability
Description
disconnected::resolve_homepage_plugin_package assumes the mirroring summary uses the Unicode 
separator and errors if it does not, even though other code in this PR explicitly supports both 
and ->. If mirror-plugins.sh outputs ASCII ->, homepage plugin resolution will abort and the
disconnected deploy cannot generate the homepage-only dynamic-plugins YAML.
Code

.ci/pipelines/lib/disconnected/plugins.sh[R187-190]

+  left="${line%%→*}"
+  if [[ "${left}" == "${line}" ]]; then
+    log::error "Summary line for ${name} has no separator: ${line}"
+    return 1
Relevance

●●● Strong

ASCII separator support is an obvious consistency fix matching the adjacent parser.

PR-#4288

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The homepage resolver hard-fails when  isn’t present, while local disconnected summary parsing in
this PR is explicitly written to handle both  and ->, demonstrating inconsistent assumptions
about the summary format.

.ci/pipelines/lib/disconnected/plugins.sh[171-190]
.ci/pipelines/lib/disconnected/local.sh[698-706]

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

### Issue description
`resolve_homepage_plugin_package` splits summary lines using only the Unicode `→` separator and treats missing `→` as fatal. The repository already contains parsing logic that supports both `→` and `->`, so the resolver should do the same.

### Issue Context
The homepage resolver is on the critical path for both Helm and Operator disconnected smokes.

### Fix Focus Areas
- .ci/pipelines/lib/disconnected/plugins.sh[171-200]

### Implementation notes
- Implement a shared helper to split summary lines that:
 - accepts either `→` or `->`
 - trims whitespace on both sides
 - returns a clear error only when neither separator is present.
- Update both homepage and (optionally) catalog-index resolvers to use the same helper for consistency.

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



Informational
4. Misleading MCP success log ✓ Resolved 🐞 Bug ◔ Observability
Description
disconnected::wait_mcp_updated logs "All MachineConfigPools are Updated" even when oc wait timed
out, which misrepresents cluster state and complicates debugging rollout-related flakes. The success
log should be conditional on the wait succeeding.
Code

.ci/pipelines/lib/disconnected/mirror.sh[R245-249]

+  log::info "Waiting for MachineConfigPool updates to complete (up to 20m)..."
+  if ! oc wait machineconfigpool --all --for=condition=Updated=True --timeout=20m; then
+    log::warn "MachineConfigPool wait timed out -- proceeding anyway"
+  fi
+  log::success "All MachineConfigPools are Updated"
Relevance

●●● Strong

Conditional success logging is a trivial, deterministic observability fix.

PR-#4288

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The function always emits a success message regardless of whether the oc wait succeeded or timed
out.

.ci/pipelines/lib/disconnected/mirror.sh[242-250]

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

### Issue description
`wait_mcp_updated` warns on timeout but then unconditionally logs a success message stating all MCPs are updated.

### Issue Context
The function is intentionally warn-and-continue, but logs should not contradict the wait result.

### Fix Focus Areas
- .ci/pipelines/lib/disconnected/mirror.sh[244-250]

### Implementation notes
- Only log `log::success "All MachineConfigPools are Updated"` when `oc wait` succeeds.
- On timeout/failure, log a neutral completion message (or keep only the warning).

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


Results up to commit c56d5ef ⚖️ Balanced


🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (1) 📜 Skill insights (0)


Action required
1. Backstage extraFiles mountPath wrong 🔗 Cross-repo conflict ≡ Correctness
Description
The disconnected Backstage CR template sets spec.application.extraFiles[*].mountPath to
directories (e.g., /etc/containers), but rhdh-operator v1alpha5 defines mountPath as the path to
mount the file (full file path) and its integration tests validate mounts as <baseMountPath>/<key>
unless an explicit full-path mount is given. This mismatch can prevent required files
(registries.conf, policy.json, CA cert, auth.json) from being mounted correctly, breaking
operator-based disconnected plugin installation in rhdh-operator.
Code

.ci/pipelines/resources/rhdh-operator/rhdh-start-disconnected-smoke.yaml[R46-49]

+        - name: rhdh-plugin-mirror-conf
+          key: rhdh-registries.conf
+          mountPath: /etc/containers/registries.conf.d
+          containers:
Relevance

●●● Strong

This is a concrete operator-schema mismatch affecting required file mounts; no contrary repository
precedent was found.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The PR’s Backstage CR sets mountPath values to directories; rhdh-operator v1alpha5 defines mountPath
as the path to mount the object (file), and its integration tests assert mounts appear at
<baseMountPath>/<key> or at the explicit full-path mountPath when provided—indicating
directory-only mountPaths are incorrect for single-file mounts.

.ci/pipelines/resources/rhdh-operator/rhdh-start-disconnected-smoke.yaml[40-66]
External repo: redhat-developer/rhdh-operator, api/v1alpha5/backstage_types.go [209-229]
External repo: redhat-developer/rhdh-operator, integration_tests/cr-config_test.go [88-101]
External repo: redhat-developer/rhdh-operator, integration_tests/cr-config_test.go [158-166]

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 Backstage CR template for the disconnected operator smoke test uses `spec.application.extraFiles.*[].mountPath` as directory paths (e.g., `/etc/containers`). In rhdh-operator v1alpha5, `mountPath` is treated as the *file mount location* (full path), or else the operator mounts files as `<extraFiles.mountPath>/<key>` when per-item `mountPath` is not set.

As written, mounts may attempt to mount a file onto an existing directory path (e.g. `/etc/containers` or `/tmp`), which can fail at runtime and prevent `install-dynamic-plugins` from reading `registries.conf`, `policy.json`, CA cert, and `auth.json`.

### Issue Context
This CR is consumed by **redhat-developer/rhdh-operator** (Backstage CRD `rhdh.redhat.com/v1alpha5`). The operator’s API/docs and integration tests show that file mounts are expected as either:
- `<extraFiles.mountPath>/<key>` when using a base mountPath, or
- the exact `FileObjectRef.mountPath` when explicitly set (as a full file path).

### Fix Focus Areas
- .ci/pipelines/resources/rhdh-operator/rhdh-start-disconnected-smoke.yaml[40-66]

### Suggested change
Update the per-item `mountPath` values to be full file paths, e.g.:
- `rhdh-registries.conf` -> `/etc/containers/registries.conf.d/rhdh-registries.conf`
- `policy.json` -> `/etc/containers/policy.json`
- `ca.crt` -> `/etc/containers/certs.d/${MIRROR_REGISTRY_URL}/ca.crt`
- `auth.json` -> `/tmp/auth.json`

Alternatively, set `spec.application.extraFiles.mountPath` to a base dir and omit per-item `mountPath` when appropriate, but because these files must land in different directories, explicit full paths are the simplest/clearest approach.

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


2. Proxy comment includes user:pass ✓ Resolved 📘 Rule violation ⛨ Security
Description
e2e-tests/playwright/utils/proxy.ts includes an example proxy URL containing credential-like
user:pass in a comment, which can be interpreted as hardcoded credentials and may trigger secret
scanning. Use a redacted or placeholder format that does not resemble real credentials.
Code

e2e-tests/playwright/utils/proxy.ts[R5-6]

+ * fields, but CI environments typically set a single `HTTPS_PROXY` URL like
+ * `http://user:pass@host:3128`. This helper splits the URL into the shape
Relevance

●●● Strong

Removing credential-like text from source comments is a straightforward security-compliance fix; no
contrary precedent was found.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
PR Compliance ID 2137 prohibits hardcoded secrets/credentials in source files. The newly added
comment contains a credential-like user:pass segment in a URL example, which resembles embedded
credentials.

Rule 2137: Secrets must not be hardcoded in source files
e2e-tests/playwright/utils/proxy.ts[5-6]

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

## Issue description
A comment includes a credential-like proxy URL (`http://user:pass@host:3128`), which can be treated as hardcoded credentials.

## Issue Context
The compliance rules require that secrets/credentials not be present in committed code, including test/support files and comments.

## Fix Focus Areas
- e2e-tests/playwright/utils/proxy.ts[5-6]

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



Remediation recommended
3. RETURN trap clobbered 🐞 Bug ☼ Reliability
Description
disconnected::resolve_homepage_plugin_package sets a global RETURN trap for tmp_dir cleanup but
never restores/chains any existing RETURN trap, so other cleanup traps in the same shell can be
overwritten and won’t run. The new trap also persists after the function returns, causing unexpected
repeated execution on subsequent function returns.
Code

.ci/pipelines/lib/disconnected/plugins.sh[R199-202]

+  }
+  # shellcheck disable=SC2064 # expand tmp_dir now, not at trap-firing time
+  trap "rm -rf '${tmp_dir}'" RETURN
+
Relevance

●●● Strong

Exact recent precedent accepted RETURN-trap clobbering cleanup finding in PR #4679.

PR-#4679

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The function sets a RETURN trap for tmp_dir cleanup, and because this library is sourced into the
main disconnected handler shell, that trap is global and can override any earlier RETURN trap
handlers in the job shell (a known recurring bug pattern).

.ci/pipelines/lib/disconnected/plugins.sh[195-207]
.ci/pipelines/lib/disconnected.sh[20-33]
PR-#4679

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

### Issue description
`disconnected::resolve_homepage_plugin_package` installs a `trap ... RETURN` to clean up a temp dir but does not preserve any pre-existing RETURN trap handler, and it does not restore the previous handler after cleanup. This can break other cleanup logic (same pattern as a previously accepted bug), and the trap continues firing for later function returns.

### Issue Context
This file is sourced into the main CI shell via `lib/disconnected.sh`, so a RETURN trap here affects the entire job shell after the function runs.

### Fix Focus Areas
- `.ci/pipelines/lib/disconnected/plugins.sh[195-207]`

### Suggested fix approach
Use a trap-chaining pattern:
1. Capture any existing RETURN trap (`trap -p RETURN`).
2. Install a new RETURN trap that:
  - removes the temp dir
  - restores the prior RETURN trap (or runs it) so behavior is preserved

Alternative (often simpler/safer): avoid RETURN traps entirely by using an explicit cleanup function and a single `return` path (or a `cleanup` + `rc` pattern) so temp cleanup is always executed without touching global traps.

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


Grey Divider

Qodo Logo

@zdrapela

Copy link
Copy Markdown
Member Author

/test e2e-ocp-disconnected-helm-nightly

@zdrapela

Copy link
Copy Markdown
Member Author

/test e2e-ocp-disconnected-operator-nightly

@github-actions

Copy link
Copy Markdown
Contributor

Image was built and published successfully. It is available at:

@zdrapela
zdrapela force-pushed the refactor/disconnected-ci-modular branch from d4a33ef to cbcef51 Compare August 19, 2026 07:43
@zdrapela

Copy link
Copy Markdown
Member Author

/test e2e-ocp-disconnected-helm-nightly

@zdrapela

Copy link
Copy Markdown
Member Author

/test e2e-ocp-disconnected-operator-nightly

@zdrapela
zdrapela force-pushed the refactor/disconnected-ci-modular branch from cbcef51 to 5513416 Compare August 19, 2026 08:27
@zdrapela

Copy link
Copy Markdown
Member Author

/test e2e-ocp-disconnected-helm-nightly

@zdrapela

Copy link
Copy Markdown
Member Author

/test e2e-ocp-disconnected-operator-nightly

@github-actions

Copy link
Copy Markdown
Contributor

Image was built and published successfully. It is available at:

@github-actions

Copy link
Copy Markdown
Contributor

Image was built and published successfully. It is available at:

@zdrapela

Copy link
Copy Markdown
Member Author

/test e2e-ocp-disconnected-helm-nightly

@zdrapela

Copy link
Copy Markdown
Member Author

/test e2e-ocp-disconnected-helm-nightly
/test e2e-ocp-disconnected-operator-nightly

@github-actions

Copy link
Copy Markdown
Contributor

Image was built and published successfully. It is available at:

@zdrapela

Copy link
Copy Markdown
Member Author

/test e2e-ocp-disconnected-helm-nightly

@github-actions

Copy link
Copy Markdown
Contributor

The container image build and publish workflows were skipped (either due to [skip-build] tag or no relevant changes with existing image).

@github-actions

Copy link
Copy Markdown
Contributor

The container image build and publish workflows were skipped (either due to [skip-build] tag or no relevant changes with existing image).

@github-actions

Copy link
Copy Markdown
Contributor

The container image build and publish workflows were skipped (either due to [skip-build] tag or no relevant changes with existing image).

@zdrapela

Copy link
Copy Markdown
Member Author

/agentic_review

@zdrapela

Copy link
Copy Markdown
Member Author

/test e2e-ocp-disconnected-helm-nightly
/test e2e-ocp-disconnected-operator-nightly

/test e2e-ocp-helm

@rhdh-qodo-merge

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit c56d5ef

@zdrapela
zdrapela force-pushed the refactor/disconnected-ci-modular branch from c56d5ef to 0839c7a Compare August 21, 2026 08:13
@zdrapela

Copy link
Copy Markdown
Member Author

/test e2e-ocp-disconnected-operator-nightly

@zdrapela
zdrapela marked this pull request as ready for review August 21, 2026 08:17
The hub recovery in ensure_helm_hub_after_postgres deleted the hub pod
after a 30s grace. On AWS the pod delete coincided with the PostgreSQL
StatefulSet pod being rescheduled, triggering a ~5 min EBS CSI volume
detach/re-attach (FailedAttachVolume) that cascaded into a smoke-test
healthcheck timeout.

Replace the destructive oc delete pod with a rolling oc rollout restart
deployment, which only touches the hub Deployment and never disturbs the
PostgreSQL StatefulSet. Raise the pre-restart grace 30s->180s so the hub
usually goes Available on its own (making the restart a no-op), and the
rollout timeout 300s->420s to absorb slow disconnected image pulls.

Assisted-by: OpenCode
resolve_homepage_plugin_package grepped the mirroring summary and tried
two hardcoded candidate names (dynamic-home-page, homepage) to cope with
the RHIDP-14515 frontend rename, plus ->/-> separator handling.

Read the homepage frontend package straight from the plugin catalog
index that CATALOG_INDEX_IMAGE already pins instead. This works
unmodified whether the index is pinned (e.g. :1.10, which pins
dynamic-home-page) or tracks :next (which pins homepage), and survives
any future rename: the package name, digest and frontend id are all
taken from whichever homepage frontend the index actually references.

Add a shared _catalog_index_source_ref helper (used by mirror_plugins,
the local digest-list build, and homepage resolution) and resolve
homepage before resolve_catalog_index_image, which rewrites
CATALOG_INDEX_IMAGE into its mirror-consumption form.

Assisted-by: OpenCode
Move the volumes/volumeMounts injected by the disconnected Helm
post-renderer into helm-post-renderer-patch.yaml, rendered with envsubst
for ${MIRROR_REGISTRY_URL} and merged into the rendered Deployment with
yq. The patch is now readable/reviewable as plain YAML instead of an
inline yq expression.

yq is still used for the merge (a structured array-append into the
install-dynamic-plugins initContainer by name), preserving the chart's
default volumes and avoiding the Helm array-clobber pitfall. Behavior is
unchanged; verified the rendered output matches the previous inline
version for single- and multi-document manifest streams.

Assisted-by: OpenCode
…ed mirror

The integrated OCP registry does not auto-create projects on push. On a
fresh cluster, skopeo/oc-mirror manifest writes to a missing namespace
upload blobs but fail the manifest write with "denied", breaking the
local disconnected smoke at the ImageStream-tagging stage.

Pre-create the known push-target projects in setup_local_ocp_mirror and
lazily create any data-driven namespace (derived from plugin source
paths, e.g. rhdh-plugin-export-overlays) inside the ImageStream-tag loop.
Grant image-puller across the same shared project list so the workload
can pull mirrored plugins cross-namespace.

Scoped entirely to the LOCAL_DISCONNECTED path; no effect on Prow CI.

Assisted-by: OpenCode
The app-config-rhdh-disconnected-smoke.yaml ConfigMap is disconnected-only
and belongs alongside the other disconnected resources rather than in the
shared config_map directory.

Assisted-by: OpenCode
Move the aarch64 skopeo shim heredoc out of local.sh into
resources/disconnected/skopeo-amd64-shim.sh.tpl and render it with
envsubst, substituting only ${REAL_SKOPEO} so the shim's own runtime
$#/$1/$@/${cmd} expansions are preserved. Keeps the generated script
out of the shell library and easier to read/lint.

Assisted-by: OpenCode
…esql-15

The community Helm chart's default PostgreSQL image
(registry.redhat.io/rhel9/postgresql-15) is not pullable without a Red Hat
pull secret. Align the disconnected fallback with the showcase value files
(quay.io/fedora/postgresql-15) and hoist it into a shared
POSTGRESQL_IMAGE_{REGISTRY,REPO,TAG} constant in env_variables.sh.

Also coalesce yq's literal "null" (missing key) to empty so the default
fallback is actually applied.

Assisted-by: OpenCode
redhat-developer#5263 made CATALOG_INDEX_IMAGE always set (via CATALOG_INDEX_IMAGE_OVERRIDE
falling back to :RELEASE_VERSION), so the chart-value catalog-index pinning is
now redundant dead code and a second source of truth.

Delete it and consume the shared env contract everywhere:
- Remove the CI_* chart-derived catalog block in ocp-disconnected-helm.sh; the
  index is mirrored from CATALOG_INDEX_IMAGE and re-pinned to the mirrored
  digest by resolve_catalog_index_image.
- mirror.sh additionalImages now mirrors CATALOG_INDEX_IMAGE directly so CI and
  LOCAL_DISCONNECTED mirror and consume the same index.
- Delete disconnected::pin_local_catalog_index_from_chart (local.sh), its
  CI-safe stub (plugins.sh), its export -f (disconnected.sh) and its call in
  ocp-disconnected-operator.sh.

This also removes the duplicated @sha256 digest-separator normalization the
catalog blocks carried (self-review #4/#5/#6); the remaining PG separator block
is a single inline caller and stays as-is.

Assisted-by: OpenCode
The disconnected Helm job duplicated the image --set flag construction with
three divergences from helm::get_image_params: backstage registry points at the
in-cluster mirror host, the catalog index registry points at MIRROR_REGISTRY_URL,
and LOCAL_DISCONNECTED omits the hub image (chart + IDMS resolve it).

Parameterize the shared helper with optional --backstage-registry,
--catalog-registry and --omit-backstage-image; all default to the existing
connected-install behavior so aks/eks/gke callers are unchanged. The disconnected
job now builds its image flags through the helper instead of a bespoke block.

Assisted-by: OpenCode
env.sh mixed environment/auth concerns with fetching helper scripts from the
rhdh-operator repo. Move that helper into a dedicated lib/disconnected/scripts.sh
and rename it disconnected::fetch_operator_repo_script to say what it fetches.

Source the new module in disconnected.sh, update the export -f, and update the
three call sites (plugins.sh, local.sh, ocp-disconnected-operator.sh). No
behavior change.

Assisted-by: OpenCode
local.sh had grown to ~700 lines mixing five distinct concerns. Split it into
lib/disconnected/local/ modules behind a thin facade that preserves the
sourced-once guard and the LOCAL_DISCONNECTED-only source contract:

  hooks.sh        cluster_mirror_host + _hook_* overrides for mirror.sh/plugins.sh
  registry.sh     integrated-registry bring-up, retry-on-503, MIRROR_* bootstrap,
                  amd64 skopeo shim
  access.sh       mirror push-target projects, workload + OLM pull access
  mirror-host.sh  IDMS/ITMS push-route -> in-cluster registry service rewrite
  plugins.sh      full mirror_plugins override + digest list + imagestream tags

Pure code movement: the set of defined functions and the
DISCONNECTED_LOCAL_MIRROR_PROJECTS array are byte-identical to the original, and
all 21 functions load through the facade. No behavior change.

Assisted-by: OpenCode
fetch_operator_repo_script had sha/pull-request/branch ref detection and an
optional $3 ref override, but no call site ever passed $3 and CI always sets
RELEASE_BRANCH_NAME from JOB_SPEC (defaulting to main locally). Drop the dead
branching and always fetch from refs/heads/${RELEASE_BRANCH_NAME}, matching the
existing convention in install-methods/operator.sh.

Assisted-by: OpenCode
Local disconnected runs leave pull secrets and CA files in mktemp dirs;
extend the existing openshift-ci-tests.sh cleanup trap to rm -rf
DISCONNECTED_TMPDIR when set.
Operator requires a separate rhdh-plugin-mirror-policy ConfigMap for
extraFiles volume keys; Helm and Operator now both mount the same
resources/disconnected/policy.json instead of duplicating inline JSON.
Disconnected Helm was splitting chart repository@sha256 fields inline.
Move that into common::normalize_chart_image_ref so digest vs tag
handling is not duplicated if other jobs need the same chart encoding.
A sourced RETURN trap clobbers later cleanup in the job shell, and
tar -xzf fails on uncompressed OCI layers. Extract with tar -xf and
remove the temp dir on an explicit return path instead.
…d smoke

ref:// is resolved through the mirrored catalog index, so constructing
oci:// package refs (and the CI vs local hook) is unnecessary.
--plugin-index enumerated every catalog plugin, including unpublished
:tag refs that fail to pull. Smoke only needs homepage plus the catalog
index so ref:// still resolves.
ref:// cannot resolve with includes: []; envsubst the catalog digest so
install-dynamic-plugins does not CrashLoop. Tighten the homepage filter
and override skopeo os/arch on catalog extract.
Integrated registry only serves ImageStreams by tag, so local disconnected
needs :sha256-<digest> for the catalog index. Retry skopeo copies on
registry Recreate EOF.
Smoke only needs the current stream; mirroring * on next pulled 43 historical images and aborted on quay CDN EOF.
@zdrapela
zdrapela force-pushed the refactor/disconnected-ci-modular branch from a68ac85 to 372a443 Compare August 27, 2026 06:15
@openshift-ci openshift-ci Bot removed the lgtm label Aug 27, 2026
@github-actions

Copy link
Copy Markdown
Contributor

The container image build workflow finished with status: failure.

@sonarqubecloud

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown
Contributor

Image was built and published successfully. It is available at:

@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.

lgtm thanks!

@openshift-ci openshift-ci Bot added the lgtm label Aug 27, 2026
@openshift-merge-bot
openshift-merge-bot Bot merged commit 8b343e2 into redhat-developer:main Aug 27, 2026
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants