[HYPERSHELL-45] Update gateway and supervisor openshell images to Red Hat ones - #201
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
26e26c9 to
457f787
Compare
JGiola
added
the
do-not-merge/hold
|
Put on hold for engaging with the opendatahub team for solving some open points with the new images. |
457f787 to
dc740d8
Compare
JGiola
removed
the
do-not-merge/hold
I didn't see that semver tags were already pushed and available with our concerns already addressed. Removing the on-hold labels |
dc740d8 to
c9769a3
Compare
Amber reviewStatus: Complete VerdictCOMMENT (approve-leaning). The core change is sound and convention-aligned: it removes hard-coded upstream Hi, Amber here. I loaded What this PR does well
Findings1. [Major - Spec Consistency] 2. [Major - Spec Consistency] Specs still advertise built-in defaults the code no longer provides. 3. [Minor - Docs] 4. [Minor - Docs] 5. [Minor - Docs] Typo 6. [Minor - Docs] Stale/self-contradictory "GHCR image tag convention" note. Cross-PR coordinationI listed all currently open PRs against Open PRs at review time: #216, #214, #212, #211, #210, #209, #208, #207, #206, #201 (this), #200, #194, #189, #188, #185, #182, #179, #151, #150, #148, #135, #109, #75, #73. Material conflict - #194
Coordination - #148 No material conflict found with the remaining open PRs (#211/#150 touch Kind/local-dev but different keys/files; the rest are UI, auth, e2e, docs, or dependency bumps with no competing interface or ordering dependency on this change). Findings Summary (ordered by severity, highest first)
Convention Checklist
|
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
COMMENT (approve-leaning). The core change is sound and convention-aligned: it removes hard-coded upstream ghcr.io/nvidia/openshell image defaults from Go code, resolves gateway/supervisor images from GATEWAY_IMAGE/GATEWAY_SUPERVISOR_IMAGE (with an explicit error when neither config nor env is set), and pins the Red Hat images in the base deploy overlay. No blockers or security issues; the findings below are documentation/spec drift and one process-breaking skill inconsistency that should be fixed before merge.
Hi, Amber here. I loaded CLAUDE.md, the security and control-plane conventions specs, and the review guidance, then reviewed the diff against main and cross-checked the other open PRs for coordination issues.
What this PR does well
- Config over code. Deleting
defaultGatewayImage/defaultSupervisorImageand sourcing them from env/GatewayConfigmatches the "separate configuration from code" and "image references must match across the stack" conventions. The pins now live indeploy/base/controller.yaml,scripts/kind/lib.sh, and the specs. - Proper error handling.
manifests.gousescmp.Or(...)and returns an expliciterrors.New(...)when no image is resolvable; the caller inreconciler.go:722wraps it withfmt.Errorf("apply substitutions for %s: %w", ...). Nopanic(), no swallowed errors. - Test guarantee preserved. The one modified assertion in
validation_test.go:16keepswantErr: falseand still exercises the multi-segment-path/digest regex path - it is not a weakened guarantee, just a new example string.
Findings
1. [Major - Spec Consistency] update-openshell skill now points at consts that no longer exist.
skills/tooling/update-openshell/SKILL.md still declares the "authoritative current version is the pair of consts in config.go: defaultGatewayImage/defaultSupervisorImage" and lists config.go as the "Source of truth - change here first" (L41-L66). This PR deletes those consts. Following the skill after this merge will fail (the grep finds nothing) and the real pins - now in deploy/base/controller.yaml env and scripts/kind/lib.sh - are not in the footprint table. This is the workflow the repo uses to bump OpenShell versions; it will silently drift. Update the "Source of truth" section and footprint table to point at the deploy overlay env vars.
2. [Major - Spec Consistency] Specs still advertise built-in defaults the code no longer provides.
specs/platform/data-model.spec.md:202 and specs/platform/openshell-gateway.spec.md:674,798-799 document image/supervisor_image as optional fields with a code default. After this change DefaultGatewayImage()/DefaultSupervisorImage() return "", and ApplyManifestToNamespace errors out unless the deployment sets the env vars or the Gateway sets the field. The out-of-box default is satisfied by deploy/base/controller.yaml, but the "No / default" spec columns now overstate the guarantee: a control-plane deployed without those env vars (any overlay that doesn't inherit the base env) will fail provisioning. Please reword the specs to say the default is supplied by the control-plane deployment env, not the field itself. (Related: the misconfig error is returned as a generic error and will be retried as if transient rather than marking the Gateway Failed - see inline note.)
3. [Minor - Docs] ibm-cluster skill: broken JSON in the gateway-create example.
skills/deploy/ibm-cluster/SKILL.md:434 - the supervisor_image value is missing its closing quote and trailing comma, so the curl -d '{...}' body is now invalid JSON.
4. [Minor - Docs] ibm-cluster skill: supervisor mirror uses the gateway digest.
skills/deploy/ibm-cluster/SKILL.md:389 copies odh-openshell-supervisor:v0.0.109-rhaiv.0@sha256:a80b79e5... - that digest is the gateway digest. The supervisor digest is sha256:96e21135... per deploy/base/controller.yaml:52. A digest-pinned skopeo copy against the supervisor repo with the gateway digest will pull the wrong content or fail.
5. [Minor - Docs] Typo odh-openshell-gatway (missing "e").
specs/platform/openshell-gateway.spec.md:194,382,425,832 all read odh-openshell-gatway. Correct is odh-openshell-gateway (as used consistently elsewhere in the PR).
6. [Minor - Docs] Stale/self-contradictory "GHCR image tag convention" note.
specs/platform/openshell-gateway.spec.md:382 still says OpenShell images "on GHCR use commit-SHA tags only (no semver tags)" while giving a quay.io semver example and asserting it "corresponds to v0.0.91" (the tag is v0.0.109). Given the PR title ("use semver version") this paragraph should be rewritten for the quay.io/semver reality.
Cross-PR coordination
I listed all currently open PRs against openshift-online/hypershell and compared goals, ownership, data models, and change order with this PR.
Open PRs at review time: #216, #214, #212, #211, #210, #209, #208, #207, #206, #201 (this), #200, #194, #189, #188, #185, #182, #179, #151, #150, #148, #135, #109, #75, #73.
Material conflict - #194 feat(control-plane): adopt upstream OpenShell Helm chart for gateway deployments. This is a genuine design-level collision, not a mere file overlap:
- #194 deletes
components/control-plane/internal/gateway/manifests.goin its entirety and replaces the static-YAML placeholder-substitution model (IMAGE_PLACEHOLDER/SUPERVISOR_IMAGE_PLACEHOLDERinApplyManifestToNamespace) with a Helm-chart deployment (internal/helm/values.go,helm_deploy.go). The exact function this PR edits (ApplyManifestToNamespace, addingcmp.Or+ error-on-empty) is removed by #194. - Both PRs edit the same
config.goimage-default lines in opposite directions: this PR removesdefaultGatewayImage/defaultSupervisorImage; #194 keeps them and repointsdefaultGatewayImageat a personal dev registry (quay.io/bsquizza/openshell-gateway:16112bc) with a mismatched supervisor tag. The two encode incompatible philosophies for image resolution (env-only vs. code const). - Both also touch
scripts/kind/lib.sh(GATEWAY_IMAGEdefault) andspecs/platform/openshell-gateway.spec.md. - Decision needed: maintainers should sequence these and decide whether the env-only image resolution (this PR) carries into #194's Helm values mapping, and reconcile the Red Hat image pins vs. #194's dev-registry defaults. Whichever merges second must be reworked, not auto-merged.
Coordination - #148 docs(specs): add OpenShell branch build spec. #148 introduces a new first-class sandbox_image Gateway field (peer to image/supervisor_image) defaulting to the community base, explicitly to "close the gap where the sandbox default_image is currently hardcoded." This PR leaves defaultSandboxImage hardcoded in config.go while removing the gateway/supervisor code defaults - so the two PRs move the sandbox image in different directions, and both edit the same field tables in specs/platform/data-model.spec.md and specs/platform/openshell-gateway.spec.md. Decision needed: agree on one direction for the sandbox image (keep code default vs. promote to a configurable field) and align the shared spec field tables.
No material conflict found with the remaining open PRs (#211/#150 touch Kind/local-dev but different keys/files; the rest are UI, auth, e2e, docs, or dependency bumps with no competing interface or ordering dependency on this change).
Findings Summary (ordered by severity, highest first)
- [Major]
update-openshellskill still names deletedconfig.goconsts as the source of truth; deploy-env pins not added to footprint - Spec Consistency (update-openshell/SKILL.md L41-L66) - [Major] Specs advertise
image/supervisor_imagebuilt-in defaults the code no longer provides (now required unless env/config set) - Spec Consistency (data-model.spec.md L202, openshell-gateway.spec.md L674, L798-799) - [Minor] Broken JSON in ibm-cluster gateway-create example - Docs (ibm-cluster/SKILL.md L434)
- [Minor] Supervisor mirror copy uses the gateway digest - Docs (ibm-cluster/SKILL.md L389)
- [Minor] Typo
odh-openshell-gatway- Docs (openshell-gateway.spec.md L194, L382, L425, L832) - [Minor] Stale/contradictory GHCR tag-convention note - Docs (openshell-gateway.spec.md L382)
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
errors.IsNotFound/error propagation on reconcile paths |
Pass |
| No secrets in logs or responses | Pass |
| Reconcile pattern (not create-or-skip) | Pass |
| Image references consistent across manifests | Fail (supervisor digest + typo drift in docs) |
| Configuration separate from code | Pass |
| Test Diff Scrutiny (modified assertions) | Pass |
| Specs consistent with code | Fail (defaults documented but removed) |
| Conventional commit messages | Pass |
| {name: "bare name with tag", ref: "postgres:18", wantErr: false}, | ||
| {name: "docker hub library path", ref: "docker.io/library/postgres:18", wantErr: false}, | ||
| {name: "ghcr multi-segment path with tag", ref: "ghcr.io/nvidia/openshell/gateway:0.0.101", wantErr: false}, | ||
| {name: "ghcr multi-segment path with tag", ref: "quay.io/opendatahub/odh-openshell-gateway:v0.0.109-rhaiv.0@sha256:a80b79e514826e8d57ea137749cf18a6e7f3d92e26bfefe005f3a9c4a55b8bdd", wantErr: false}, |
There was a problem hiding this comment.
Assertion still wantErr: false, so the guarantee is preserved - no concern there. Minor: the case is still named "ghcr multi-segment path with tag" but the ref is now a quay.io digest reference. Rename for accuracy.
| @@ -44,8 +44,8 @@ The **authoritative** current version is the pair of consts in | |||
| `components/control-plane/internal/gateway/config.go`: | |||
|
|
|||
| ```go | |||
There was a problem hiding this comment.
[Major] This "Source of truth" block and the footprint row at L66 reference defaultGatewayImage/defaultSupervisorImage in config.go, which this PR removes. The version-bump workflow will break (grep finds nothing) and the real pins in deploy/base/controller.yaml + scripts/kind/lib.sh are not listed. Update the source-of-truth and footprint table.
| docker://quay.io/opendatahub/odh-openshell-gateway:v0.0.109-rhaiv.0@sha256:a80b79e514826e8d57ea137749cf18a6e7f3d92e26bfefe005f3a9c4a55b8bdd docker://$REG/openshift/openshell-gateway:v0.0.109-rhaiv.0 | ||
| skopeo copy --dest-tls-verify=false --dest-creds "pusher:$(oc -n hypershell create token pusher)" \ | ||
| docker://ghcr.io/nvidia/openshell/supervisor:0.0.109 docker://$REG/openshift/openshell-supervisor:0.0.109 | ||
| docker://quay.io/opendatahub/odh-openshell-supervisor:v0.0.109-rhaiv.0@sha256:a80b79e514826e8d57ea137749cf18a6e7f3d92e26bfefe005f3a9c4a55b8bdd docker://$REG/openshift/openshell-supervisor:v0.0.109-rhaiv.0 |
There was a problem hiding this comment.
[Minor] This copies the supervisor image but pins @sha256:a80b79e5..., which is the gateway digest. The supervisor digest is sha256:96e21135... (see deploy/base/controller.yaml:52). A digest-pinned copy with the wrong digest pulls the wrong content or fails.
| "image":"image-registry.openshift-image-registry.svc:5000/openshift/openshell-gateway:0.0.109", | ||
| "supervisor_image":"image-registry.openshift-image-registry.svc:5000/openshift/openshell-supervisor:0.0.109", | ||
| "image":"image-registry.openshift-image-registry.svc:5000/openshift/openshell-gateway:v0.0.109-rhaiv.0", | ||
| "supervisor_image":"image-registry.openshift-image-registry.svc:5000/openshift/openshell-supervisor:v0.0.109-rhaiv.0 |
There was a problem hiding this comment.
[Minor] Broken JSON: the supervisor_image value is missing its closing quote and trailing comma, so the curl -d '{...}' payload is now invalid.
| - AND the Gateway SHALL not be reconciled until the configuration is corrected | ||
|
|
||
| > **GHCR image tag convention:** OpenShell gateway images on GHCR use commit-SHA tags only (no semver tags). For example, `ghcr.io/nvidia/openshell/gateway:21da343c9f838bd9ac85dc61bf44889de1a72873` corresponds to v0.0.91. The GatewayReconciler continuously reconciles the image field, so the gitops overlay must be the source of truth for the image tag - manual image changes on the Deployment will be reverted. | ||
| > **GHCR image tag convention:** OpenShell gateway images on GHCR use commit-SHA tags only (no semver tags). For example, `quay.io/opendatahub/odh-openshell-gatway:v0.0.109-rhaiv.0@sha256:a80b79e514826e8d57ea137749cf18a6e7f3d92e26bfefe005f3a9c4a55b8bdd` corresponds to v0.0.91. The GatewayReconciler continuously reconciles the image field, so the gitops overlay must be the source of truth for the image tag - manual image changes on the Deployment will be reverted. |
There was a problem hiding this comment.
[Minor] Two issues on this line: (1) typo odh-openshell-gatway (missing "e", also at L194/L425/L832); (2) the note says GHCR images "use commit-SHA tags only (no semver tags)" and "corresponds to v0.0.91", but the example is now a quay.io semver tag v0.0.109-rhaiv.0. Rewrite for the quay.io/semver reality (the PR title even says "use semver version").
Amber reviewStatus: Complete VerdictCOMMENT. This PR cleanly shifts gateway/supervisor image selection out of hardcoded Go constants and into deploy-time configuration (env vars on the control-plane deployment or the Gateway resource fields), replacing the ghcr.io/nvidia OpenShell images with digest-pinned Red Hat quay images. The design is sound and aligns with the "separate configuration from code" convention; I found only minor issues (a stale path in the update tooling doc, a second source of truth for the sandbox image, and a documented-but-breaking upgrade edge) plus one cross-PR coordination item. What's good
Findings[Minor] Stale path in the update-openshell skill — [Minor] Second source of truth for the sandbox image — [Minor] Documented but breaking for non-base deployments — removing the constants makes Cross-PR coordinationAn open pull request proposes replacing the static-YAML + placeholder-substitution gateway deployment mechanism (the
Maintainers need to decide which image-configuration model wins and in what order these merge. If the Helm-adoption PR lands first, the env-var-only image defaults and the "controller.yaml is the source of truth" contract from this PR must be re-wired into the Helm values path; if this PR lands first, the other must drop the reintroduced constants and route images through the env vars. Please coordinate the ownership of image resolution and the merge order before either lands. See #194. Findings Summary (ordered by severity, highest first)
Convention Checklist
|
Amber reviewStatus: Complete VerdictThis PR cleanly moves the gateway/supervisor image pins out of hardcoded Go constants and into required What I verified
Findings[Minor] Required env vars are only validated deep in the per-gateway reconcile loop. [Minor] Comment nits in Cross-PR coordinationThe open PR that adopts the upstream OpenShell Helm chart for gateway deployments makes a directly competing design choice for the same ownership area this PR changes: gateway/supervisor image defaults and injection. That PR keeps Findings Summary
Convention Checklist
|
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
This PR cleanly moves the gateway/supervisor image pins out of hardcoded Go constants and into required GATEWAY_IMAGE / GATEWAY_SUPERVISOR_IMAGE env vars (with the per-Gateway image/supervisor_image still taking precedence), and adds a fail-fast when neither a config value nor an env default is available. The change is well-scoped and consistent across code, manifests, specs, and skills; I have one operability suggestion and a cross-PR coordination item, but nothing blocking.
What I verified
- Config-over-code alignment. Removing
defaultGatewayImage/defaultSupervisorImageand readingos.Getenv(...)matches the "Separate configuration from code" convention. The digest pins now live indeploy/base/controller.yaml, and thekind,openshift, andibmoverlays allresources: [../base]and add to the controller env via strategic-merge patch rather than replacing it, so the required vars are inherited on every standard deploy path. Good. - Image consistency across the stack. The gateway digest (
sha256:a80b79...) and supervisor digest (sha256:96e211...) match acrossdeploy/base/controller.yaml,scripts/kind/lib.sh, theibm-clusterskill, and the platform specs. The remainingghcr.io/nvidia/openshell/...and bare0.0.109strings are the intentional regex fixture invalidation_test.goand historical/illustrative references theupdate-openshellskill explicitly says not to touch. - Error handling.
ApplyManifestToNamespacereturns a plainerrors.New(...)sentinel (no wrapped cause, which is fine here) and the caller wraps it withfmt.Errorf("apply substitutions for %s: %w", ...). Nopanic(), correct%wpropagation. - Test diff scrutiny. The
validation_test.gochange is additive: the existing ghcr case still assertswantErr: false(only the tag string changed), and a new digest-pinned quay case is added. No pre-existing guarantee was flipped or removed.
Findings
[Minor] Required env vars are only validated deep in the per-gateway reconcile loop. GATEWAY_IMAGE/GATEWAY_SUPERVISOR_IMAGE are now effectively required for the controller to provision any gateway, but a missing value only surfaces inside ApplyManifestToNamespace when the first gateway is reconciled, and then re-errors on every retry. Consider validating their presence once at controller startup (cmd/hypershell-controller/main.go) so a misconfigured deployment fails fast and unambiguously instead of appearing healthy until a tenant gateway is created. The default gitops path is covered by deploy/base, so this is robustness, not a defect. Confidence: Medium.
[Minor] Comment nits in config.go. The trimmed doc comments ("Set via GATEWAY_IMAGE" / "Set via GATEWAY_SUPERVISOR_IMAGE") lost their trailing periods and no longer state the now-important behavior that an unset var yields an empty string that the caller rejects. A one-line note would help the next reader. Confidence: High.
Cross-PR coordination
The open PR that adopts the upstream OpenShell Helm chart for gateway deployments makes a directly competing design choice for the same ownership area this PR changes: gateway/supervisor image defaults and injection. That PR keeps defaultGatewayImage/defaultSupervisorImage as hardcoded constants in components/control-plane/internal/gateway/config.go (repointing them to a different registry/tag) and replaces the manifests.go IMAGE_PLACEHOLDER/SUPERVISOR_IMAGE_PLACEHOLDER substitution with Helm values, whereas this PR deletes those constants entirely and makes the images resolve solely from env vars, with a fail-fast in ApplyManifestToNamespace. These two approaches cannot both land as-is: maintainers need to decide whether image resolution is env-var-driven (this PR) or constant/Helm-values-driven, agree on the merge order, and ensure the winning image-default model is carried into the Helm values path. This requires a maintainer decision and coordination between the two authors.
Findings Summary
- [Minor] Required image env vars validated only at reconcile time, not controller startup - Operability (config.go L42-49, manifests.go L91-105)
- [Minor]
config.godoc-comment nits for the new env-var-only resolution - Docs (config.go L40-49)
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
| No secrets in logs or responses | Pass |
| Image references consistent across the stack | Pass |
| Separate configuration from code | Pass |
| Reconcile pattern (not create-or-skip) | Pass |
| Test diff scrutiny (no silently flipped assertions) | Pass |
Amber reviewStatus: Complete VerdictCOMMENT. This is a focused, well-scoped switch from the upstream SummaryThe change removes the Findings1. [Major] Image-tag drift between the ROKS e2e script and the mirror instructions — Image consistency 2. [Major] Gateway/supervisor images are now hard-required with no fallback, validated only at per-gateway reconcile time — Reconciliation / optional→required Findings Summary (ordered by severity, highest first)
Convention Checklist
Cross-PR coordinationAnother open pull request replaces the gateway deployment mechanism this PR modifies: it deletes |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
COMMENT. This is a focused, well-scoped switch from the upstream ghcr.io/nvidia/openshell/* images to the Red Hat quay.io/opendatahub/odh-openshell-* builds, and it correctly moves the image pin out of code and into deployment configuration (config-separate-from-code). Two things need attention before merge: an image-tag drift between the ROKS e2e script and the mirroring instructions, and the fact that the gateway/supervisor images are now hard-required env vars with no fallback, which is only validated deep in the per-gateway reconcile path.
Summary
The change removes the defaultGatewayImage/defaultSupervisorImage constants, resolves both images from GATEWAY_IMAGE/GATEWAY_SUPERVISOR_IMAGE (empty → reconcile error), pins the new digests in deploy/base/controller.yaml, and updates specs/skills so deploy/base/controller.yaml becomes the source of truth. Error handling is clean (errors.New, no panic(), error is wrapped and propagated by the reconciler), the digest pins are internally consistent across the base manifest, specs, kind lib, and the ibm-cluster skill, and the cmp.Or refactor is idiomatic.
Findings
1. [Major] Image-tag drift between the ROKS e2e script and the mirror instructions — Image consistency
components/pr-test/e2e-openshell-roks.sh:67-68 defaults the mirrored refs to openshell-gateway:0.0.109-rhaiv.0 / openshell-supervisor:0.0.109-rhaiv.0 (no v prefix), but skills/deploy/ibm-cluster/SKILL.md:387-389,455-456 mirrors and consumes those same internal-registry images as v0.0.109-rhaiv.0 (with the v). A ROKS e2e run that has to create a gateway would request a tag that the documented mirroring step never produced, yielding ImagePullBackOff. Pick one tag convention (the upstream quay tag carries the v, so v0.0.109-rhaiv.0 is the natural choice) and align both files. Confidence: High.
2. [Major] Gateway/supervisor images are now hard-required with no fallback, validated only at per-gateway reconcile time — Reconciliation / optional→required
config.go:43-52 now returns the bare os.Getenv(...) value, and manifests.go:92-94,103-105 fails reconciliation with "... image is not configured and no default is available" when it is empty. This turns a previously-optional setting into a required one with no code fallback and no backfill. It is coordinated in-PR via deploy/base/controller.yaml, but only the base overlay sets these vars — a control plane rolled out with just a new image (env vars not re-applied), or any deployment path that does not inherit base, will silently stop provisioning gateways and only surface the failure once a specific Gateway is reconciled. Recommend validating both env vars at controller startup (fail-fast with a clear message) so a mis-provisioned deployment is caught immediately rather than per-gateway, and calling out the "must set these env vars on upgrade" requirement in the deploy docs. Confidence: Medium.
Findings Summary (ordered by severity, highest first)
- [Major] ROKS e2e mirror tag missing the
vprefix vs. the ibm-cluster mirror/consume tag — Image consistency (e2e-openshell-roks.sh L67-68) - [Major] Images moved optional→required with no fallback; only validated at reconcile time, not at startup — Reconciliation (config.go L43-52, manifests.go L92-105)
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
| Errors wrapped / returned with context | Pass |
| No secrets in logs or responses | Pass |
| Reconcile pattern (not create-or-skip) | Pass |
| Config separated from code | Pass |
| Image references consistent across the stack | Fail |
| Optional→required change has fallback/backfill | Fail |
| Test diff scrutiny (no silently flipped assertions) | Pass |
| Conventional commit messages | Pass |
Cross-PR coordination
Another open pull request replaces the gateway deployment mechanism this PR modifies: it deletes components/control-plane/internal/gateway/manifests.go (the placeholder-substitution path this PR edits) and reworks config.go to render gateway images through upstream Helm chart values (image.repository/image.tag, "split at last :"), while keeping hardcoded defaultGatewayImage/defaultSupervisorImage constants pointing at a different registry and pin (quay.io/bsquizza/openshell-gateway:16112bc). This directly conflicts with this PR's core decisions: (a) which image-resolution mechanism wins (placeholder substitution vs Helm values), (b) whether hardcoded default constants exist at all vs. env-var-required-with-no-fallback, (c) the canonical registry and source-of-truth pin (and the update-openshell skill text both PRs rewrite differently), and (d) a Helm "split at last :" mapping would mis-parse the digest-pinned ...:v0.0.109-rhaiv.0@sha256:... references this PR mandates. Maintainers should decide which approach lands first and reconcile the image-defaulting contract and source-of-truth before merging either; that PR is #194.
| REG_MIRROR="${REG_MIRROR:-image-registry.openshift-image-registry.svc:5000/openshift}" | ||
| GW_IMAGE="${GW_IMAGE:-${REG_MIRROR}/openshell-gateway:0.0.109}" | ||
| GW_SUPERVISOR_IMAGE="${GW_SUPERVISOR_IMAGE:-${REG_MIRROR}/openshell-supervisor:0.0.109}" | ||
| GW_IMAGE="${GW_IMAGE:-${REG_MIRROR}/openshell-gateway:0.0.109-rhaiv.0}" |
There was a problem hiding this comment.
This defaults the mirrored refs to tag 0.0.109-rhaiv.0 (no v), but skills/deploy/ibm-cluster/SKILL.md mirrors and consumes the same internal-registry images as v0.0.109-rhaiv.0 (with the v). A create-path e2e run would pull a tag that was never mirrored, causing ImagePullBackOff. Align to v0.0.109-rhaiv.0 for both gateway and supervisor.
54e0a49 to
f8334dd
Compare
Amber reviewStatus: Complete VerdictREQUEST_CHANGES. This PR cleanly retargets the gateway/supervisor images from Findings[Blocker] Required image env vars are missing from the deployed control-plane manifests — Reconciliation / optional→required without fallback (High confidence)
The env vars were added only to
None of these set Fix: add both env vars to Positive notes
Cross-PR coordinationAnother open pull request proposes shifting gateway deployment from the static-manifest renderer to the upstream OpenShell Helm chart. That work deletes Findings Summary (ordered by severity, highest first)
Convention Checklist
|
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
REQUEST_CHANGES. This PR cleanly retargets the gateway/supervisor images from ghcr.io/nvidia to digest-pinned Red Hat quay.io/opendatahub refs and consolidates the version pin into deployment env vars, and the image references are consistent across every manifest, script, and spec. However, removing the in-code fallback constants turns GATEWAY_IMAGE/GATEWAY_SUPERVISOR_IMAGE into hard requirements, and the env vars were only added to deploy/base/controller.yaml — the control-plane manifests that the deploy skills actually apply do not set them, so gateway provisioning regresses to a hard failure on those clusters.
Findings
[Blocker] Required image env vars are missing from the deployed control-plane manifests — Reconciliation / optional→required without fallback (High confidence)
config.go:43-51 drops the defaultGatewayImage/defaultSupervisorImage constants so DefaultGatewayImage()/DefaultSupervisorImage() now return a bare os.Getenv(...). Combined with manifests.go:91-104, a gateway whose image/supervisor_image fields are empty (both are optional per specs/platform/openshell-gateway.spec.md) now hard-fails reconciliation with "gateway image is not configured and no default is available".
The env vars were added only to deploy/base/controller.yaml. But the control-plane deployments that the deploy skills actually apply live in a different tree and do not set them:
components/api-server/deploy/openshift/controller.yaml— applied bydeploy-cluster(oc kustomize deploy/openshift/ | oc apply -f -)components/api-server/deploy/ibmoverlay — applied byibm-cluster(oc kustomize deploy/ibm | oc apply -f -)components/api-server/deploy/kind/controller.yaml— applied bymake kind-up
None of these set GATEWAY_IMAGE/GATEWAY_SUPERVISOR_IMAGE. So on a real OpenShift/IBM/kind deployment, any Gateway created without explicit image fields will fail to provision — a regression from the previous working default. This is the optional→required-without-fallback pattern: the "old value" (env var absent) already exists in the deployed manifests. The PR's own updated skills/deploy/ibm-cluster/SKILL.md (~line 442) even documents that the default comes from these env vars "on the control-plane deployment," yet the manifests it deploys don't set them.
Fix: add both env vars to components/api-server/deploy/{kind,openshift}/controller.yaml (and confirm the deploy/ibm overlay inherits/overrides them for the mirror), or keep a safe fallback. Ensure whichever control-plane manifest tree is authoritative carries the pin.
Positive notes
- Image references (including sha256 digests) are consistent across
deploy/base/controller.yaml,scripts/kind/lib.sh, the ibm-cluster/update-openshell skills, and all specs. - The
validation_test.gochange is additive: it keeps the existing ghcr case and adds a new digest+tag case; no existing assertion was flipped, so no guarantee was removed. - New error paths in
manifests.goare wrapped and propagated correctly by the caller (reconciler.go:834-836).
Cross-PR coordination
Another open pull request proposes shifting gateway deployment from the static-manifest renderer to the upstream OpenShell Helm chart. That work deletes components/control-plane/internal/gateway/manifests.go (the file this PR edits to add the required-image error paths) and takes a different image-defaulting stance — it retains hardcoded defaultGatewayImage/defaultSupervisorImage constants pointed at yet another registry/tag (quay.io/bsquizza/..., commit-SHA tags) rather than this PR's env-var-required, digest-pinned quay.io/opendatahub model. These are competing designs for the same subsystem (where the gateway image default lives and how manifests are rendered). Maintainers need to decide which approach wins and the merge order: if the Helm-chart adoption lands, this PR's env-var image contract must be re-expressed as Helm values mapping, and the manifest edits here become moot; if this PR lands first, the Helm work must adopt the env-var/digest model rather than reintroducing constants.
Findings Summary (ordered by severity, highest first)
- [Blocker] Required
GATEWAY_IMAGE/GATEWAY_SUPERVISOR_IMAGEenv vars absent from the control-plane manifests the deploy skills apply (components/api-server/deploy/{kind,openshift,ibm}); gateways without explicit image fields fail to provision after the fallback constants were removed - Reconciliation (config.go:43-51, manifests.go:91-104)
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
| Errors wrapped/propagated with context | Pass |
| No secrets in logs or responses | Pass |
| Image references consistent across the stack | Pass |
| Config separate from code | Pass |
| Reconcile fallback/backfill for optional→required change | Fail |
| Test Diff Scrutiny (no silently flipped assertions) | Pass |
| Conventional commit messages | Pass |
Amber reviewStatus: Complete VerdictCOMMENT — This is a clean, well-scoped migration from the upstream NVIDIA/ghcr OpenShell images to Red Hat ( What looks good
Findings
|
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
COMMENT — This is a clean, well-scoped migration from the upstream NVIDIA/ghcr OpenShell images to Red Hat (quay.io/opendatahub/odh-openshell-*) images that also removes the hardcoded default image constants and makes GATEWAY_IMAGE/GATEWAY_SUPERVISOR_IMAGE explicit control-plane inputs. The code, deployment overlays, and specs are internally consistent, the optional→required transition is backfilled in deploy/base/controller.yaml, and the empty-image case fails with an explicit error (not a panic) that propagates and marks the gateway Failed. My only in-PR note is a minor fail-fast/observability suggestion; the more important output is cross-PR coordination below.
What looks good
manifests.goresolves images withcmp.Or(config.X, images.DefaultX())and returns an explicit error when the result is empty; the error is wrapped bydeployGateway/Handleand parks the gateway atFailed— correct terminal-error handling.- Image references are consistent across the stack:
deploy/base/controller.yaml,scripts/kind/lib.sh,validation_test.go, and the specs all use the samev0.0.109-rhaiv.0@sha256:…digests. - Test diff scrutiny:
validation_test.gois purely additive (new digest+tag case, a cosmetic version-literal bump) — no removed guarantee. - The optional→required image transition is backfilled:
deploy/base/controller.yamlsets both env vars, inherited by thekindandopenshiftoverlays.
Findings
- [Minor] Observability / fail-fast.
DefaultGatewayImage()/DefaultSupervisorImage()now return the raw env var with no fallback, so a control plane started without these env vars only fails at the first tenant-gateway reconcile (gateway image is not configured…), per namespace, rather than at startup. Any overlay not derived fromdeploy/base(custom/legacy deployments) inherits this latent failure. Consider validating both variables once at controller startup so a misconfiguration surfaces immediately instead of as a per-gatewayFailedstate. Confidence: High.
Cross-PR coordination
-
#194 (adopt upstream OpenShell Helm chart for gateway deployments): Competing design and change-order conflict for the exact mechanism this PR edits. This PR keeps the static-manifest
IMAGE_PLACEHOLDER/SUPERVISOR_IMAGE_PLACEHOLDERsubstitution and redefines image resolution soGATEWAY_IMAGE/GATEWAY_SUPERVISOR_IMAGEare required (empty → error). #194 removes that placeholder-substitution path entirely in favor of Helm values mapping, retains a hardcodeddefaultGatewayImageconstant fallback, and sets different default pullspecs in bothconfig.goandscripts/kind/lib.sh. Both PRs modifycomponents/control-plane/internal/gateway/config.go,manifests.go, andscripts/kind/lib.shwith incompatible models. Maintainers must decide which image-resolution design wins, the merge order, and whether the Helm values mapping carries forward this PR's "images are required, no built-in default" contract and the Red Hat pullspecs. -
#227 (per-gateway resource quotas / GatewayProfile): Structural/ownership and change-order conflict. This PR deletes the legacy
components/api-server/deploy/{kind,openshift,ibm}trees (consolidating onto the top-leveldeploy/tree), while #227 actively extends those same legacy trees with new functional RBAC (resourcequotas/limitrangesverbs) and kustomization edits (components/api-server/deploy/openshift/controller-clusterrbac.yaml,.../kind/controller-clusterrbac.yaml,.../{openshift,ibm}/kustomization.yaml). Whichever merges second is broken: #227's grants would land in files this PR removes, or this PR's deletion would drop #227's grants. Maintainers must sequence these and ensure #227's new RBAC is re-homed in the canonicaldeploy/baseRBAC. -
#210 (reconcile gateway version for CLI installation): Assumption dependency requiring a decision. #210 derives the OpenShell CLI install version by stripping a
--prefixed postfix (v0.0.109-rhaiv.0→v0.0.109) and installs the client from the NVIDIA/OpenShellmainbranch, while this PR switches the runtime gateway to the Red Hatodh-openshell-gatewaybuild that produces exactly that-rhaiv.Nversion string. The postfix-stripping contract is shaped by this PR's tag scheme, and #210 pairs it with an NVIDIA-upstream CLI source. Maintainers should confirm that the Red Hat runtime image is compatible with the NVIDIA-mainOpenShell CLI at the stripped version, and that any future change to this PR's tag scheme is reflected in #210's derivation.
| func (StaticImageDefaults) DefaultGatewayImage() string { | ||
| if v := os.Getenv("GATEWAY_IMAGE"); v != "" { | ||
| return v | ||
| } | ||
| return defaultGatewayImage | ||
| return os.Getenv("GATEWAY_IMAGE") | ||
| } | ||
|
|
||
| // DefaultSupervisorImage resolves the supervisor sidecar image used when a | ||
| // Gateway resource does not specify one. Overridable via GATEWAY_SUPERVISOR_IMAGE | ||
| // for the same ghcr.io-unreachable clusters as DefaultGatewayImage. | ||
| // Gateway resource does not specify one. Must be set via GATEWAY_SUPERVISOR_IMAGE environment | ||
| // variable; reconciliation will fail if not provided. | ||
| func (StaticImageDefaults) DefaultSupervisorImage() string { | ||
| if v := os.Getenv("GATEWAY_SUPERVISOR_IMAGE"); v != "" { | ||
| return v | ||
| } | ||
| return defaultSupervisorImage | ||
| return os.Getenv("GATEWAY_SUPERVISOR_IMAGE") |
There was a problem hiding this comment.
[Minor] Fail-fast on required image env vars. Both resolvers now return the raw env var with no fallback, so a control plane launched without GATEWAY_IMAGE/GATEWAY_SUPERVISOR_IMAGE won't fail until the first tenant-gateway reconcile hits the empty-string check in manifests.go and parks that gateway at Failed (repeated per namespace). The canonical deploy/base/controller.yaml backfills both, so this is not a regression for the standard path — but any overlay not derived from base inherits a latent, deferred failure. Consider validating both variables once at controller startup so misconfiguration surfaces immediately with a single clear error. Confidence: High.
37bba1e to
4e33502
Compare
Amber reviewStatus: Complete VerdictThis is a clean, well-documented refactor that removes the hardcoded SummaryThe optional→required config change (removing the in-code fallback defaults) is intentional, called out in the PR description, marked The findings below are documentation/consistency drift introduced by, or adjacent to, this change. Findings[Major] Stale [Minor] Dangling [Minor] Orphaned [Minor/nit] Cross-PR coordinationTwo open pull requests have material, non-file-overlap conflicts with this change that need maintainer coordination:
Convention Checklist
Findings Summary (highest severity first):
|
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
This is a clean, well-documented refactor that removes the hardcoded ghcr.io/nvidia/openshell/* image constants and makes the gateway/supervisor image pins configuration-driven (GATEWAY_IMAGE / GATEWAY_SUPERVISOR_IMAGE), pointing at Red Hat (quay.io/opendatahub/odh-openshell-*) digest-pinned images. The change is consistent with "separate configuration from code," properly returns errors (no panics) when an image is unconfigured, and the specs + update-openshell source-of-truth were moved in lockstep; the remaining issues are documentation drift and a few stale image references, none blocking.
Summary
The optional→required config change (removing the in-code fallback defaults) is intentional, called out in the PR description, marked *(required)* in the spec, and given both a resource-level fallback (Gateway.image / supervisor_image) and a manifest-level source (deploy/base/controller.yaml sets both env vars, inherited by the openshift/ibm overlays). Missing values now yield an explicit errors.New(...) propagated up the reconcile stack rather than a panic. This satisfies the Test-Diff-Scrutiny bar for a tightened precondition: the modified validation_test.go case is additive (a new digest-reference case plus a version bump), not a flipped guarantee.
The findings below are documentation/consistency drift introduced by, or adjacent to, this change.
Findings
[Major] Stale ghcr.io/nvidia/openshell/gateway references remain in skills/deploy/gcp-cluster/SKILL.md — Image Consistency
This PR's stated goal is to move off the upstream GHCR images, and update-openshell mandates that "every other occurrence of the version in the repo MUST agree." skills/deploy/gcp-cluster/SKILL.md (lines ~433, ~456, ~547) still creates gateways with ghcr.io/nvidia/openshell/gateway:0.0.109. This file is in the PR's base (merge-base includes the GCP-cluster commit), so it is in scope. It should be updated to the quay.io/opendatahub/odh-openshell-gateway:v0.0.109-rhaiv.0@sha256:... pin, and gcp-cluster should be added to the update-openshell footprint table (which currently lists only deploy-cluster / ibm-cluster). Confidence: High.
[Minor] Dangling /kind skill references after deleting skills/deploy/kind/SKILL.md — Docs Consistency
skills/build/full-stack-pipeline/SKILL.md:174 ("see /kind skill") and skills/RECONCILE.md:42 (... /deploy-cluster or /kind) still point at the removed skill. The same skill docs also reference make kind-rebuild / make image / make image-controller, which this PR removes from components/api-server/Makefile and which do not exist in the root Makefile (the canonical Kind flow is make kind-up → scripts/kind/up.sh, plus the kind-*-up swap targets). Please update or drop these references so the documented dev workflow stays runnable. Confidence: High.
[Minor] Orphaned KIND_CLUSTER_NAME and trailing blank line in components/api-server/Makefile — Cleanup
After removing all the kind-*/image* targets, KIND_CLUSTER_NAME?=hypershell-dev (line 129) has no remaining consumers, and the file ends with an added blank line. Minor cleanup. Confidence: High.
[Minor/nit] validation_test.go fixture tag bump — Consistency
update-openshell/SKILL.md explicitly says "Do NOT change" the example tag in validation_test.go because it exercises the regex, not the deployed version; this PR bumps 0.0.101→0.0.109 on the pre-existing case. Harmless (the regex assertion is unchanged), but it contradicts the guidance the same PR reaffirms. Confidence: Medium.
Cross-PR coordination
Two open pull requests have material, non-file-overlap conflicts with this change that need maintainer coordination:
-
#194 (adopt upstream OpenShell Helm chart for gateway deployments) presents a competing design for exactly the mechanism this PR modifies. #194 deletes
components/control-plane/internal/gateway/manifests.go(and themanifests/gateway/*.yamltemplates) and replaces the placeholder-substitution model with a Helm-values mapping, while also editinginternal/gateway/config.goandscripts/kind/lib.sh. This PR instead hardens the placeholder/substitution path (addscmp.Or+ required-image error returns inmanifests.go) and removes the in-code image defaults fromconfig.go. The maintainers must decide (a) which image-injection model wins and (b) the merge order: if #194 lands first, this PR'smanifests.goedits are moot; if this PR lands first, #194 must carry forward the "requiredGATEWAY_IMAGE/GATEWAY_SUPERVISOR_IMAGE, no hardcoded default" contract into its Helm values mapping. -
#227 (per-gateway resource quotas / GatewayProfile) invests in the legacy
components/api-server/deploy/tree that this PR removes. This PR (head commit "merge components/api-server/deploy with deploy") deletescomponents/api-server/deploy/{kind,openshift,ibm}/…, migrating deployment manifests to the top-leveldeploy/. #227 concurrently adds/renames/modifies manifests in that same tree (e.g.components/api-server/deploy/kind/controller-clusterrbac.yaml,components/api-server/deploy/openshift/kustomization.yaml,components/api-server/deploy/ibm/kustomization.yaml). This is a structural/ownership conflict, not a plain merge conflict: the two PRs disagree on where deployment manifests live. Maintainers must fix a merge order and ensure whichever PR lands second relocates its GatewayProfile RBAC/kustomize changes into the survivingdeploy/layout.
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
| Errors returned with context / not swallowed | Pass |
| Reconcile surfaces terminal config errors (no silent skip) | Pass |
| SecurityContext on pod specs (unchanged by this PR) | N/A |
| Optional→required config has fallback + migration path | Pass |
| Image references consistent across the stack | Fail (gcp-cluster skill) |
| Docs/skills updated with the change | Fail (dangling /kind, stale make targets) |
| Conventional commit messages | Pass |
| OpenAPI client not manually edited | N/A |
Findings Summary (highest severity first):
- [Major] Stale
ghcr.io/nvidia/openshellrefs inskills/deploy/gcp-cluster/SKILL.md; add it to theupdate-openshellfootprint — Image Consistency - [Minor] Dangling
/kindskill +make kind-rebuild/make imagereferences after deletions — Docs Consistency - [Minor] Orphaned
KIND_CLUSTER_NAME+ trailing blank line incomponents/api-server/Makefile— Cleanup - [Minor/nit]
validation_test.gofixture tag bump contradictsupdate-openshellguidance — Consistency
| {name: "bare name with tag", ref: "postgres:18", wantErr: false}, | ||
| {name: "docker hub library path", ref: "docker.io/library/postgres:18", wantErr: false}, | ||
| {name: "ghcr multi-segment path with tag", ref: "ghcr.io/nvidia/openshell/gateway:0.0.101", wantErr: false}, | ||
| {name: "ghcr multi-segment path with tag", ref: "ghcr.io/nvidia/openshell/gateway:0.0.109", wantErr: false}, |
There was a problem hiding this comment.
Nit: update-openshell/SKILL.md explicitly lists this fixture tag under "Do NOT change" (it exercises the reference regex, not the deployed version). The bump from 0.0.101→0.0.109 is harmless since wantErr is unchanged, but it contradicts the guidance this PR reaffirms. The new digest-reference case on the next line is a good additive test.
| @@ -127,74 +127,4 @@ proto-clean: | |||
| rm -rf pkg/api/grpc/ | |||
|
|
|||
| KIND_CLUSTER_NAME?=hypershell-dev | |||
There was a problem hiding this comment.
After removing the image*/kind-* targets, KIND_CLUSTER_NAME?=hypershell-dev no longer has any consumer in this file, and the file now ends with a trailing blank line. Minor cleanup — drop the orphaned variable (or the whole tail) so the Makefile doesn't carry a dangling declaration.
Amber reviewStatus: Complete VerdictThis PR cleanly retires the hardcoded upstream OpenShell image constants and makes SummaryThe change removes the in-code Findings[Minor] Optional→required image default is an operational breaking change on in-place upgrades — [Nit] Test Diff Scrutiny
Cross-PR coordinationAnother open pull request replaces the gateway deployment path wholesale — it removes the very
Findings Summary (ordered by severity, highest first)
Convention Checklist
|
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
This PR cleanly retires the hardcoded upstream OpenShell image constants and makes GATEWAY_IMAGE / GATEWAY_SUPERVISOR_IMAGE explicit, required inputs sourced from the control-plane deployment manifest, with the Red Hat quay.io digests wired through consistently and the specs/docs updated to match. The code changes are sound (proper error returns, no panics, additive test coverage); the only items worth attention are one operational upgrade note and a cross-PR design decision about how gateway images will be resolved going forward.
Summary
The change removes the in-code defaultGatewayImage / defaultSupervisorImage fallbacks so image resolution is now: per-Gateway field → env var → hard error if neither is set (cmp.Or + explicit errors.New, propagated and wrapped by the caller). The env-var defaults are set in deploy/base/controller.yaml and inherited by the kind/openshift/ibm overlays, image digests are consistent across deploy/base, scripts/kind/lib.sh, the validation test, and the specs, and the legacy duplicated components/api-server/deploy/** tree plus stale Makefile kind targets are deleted with no dangling references. This is a deliberate, well-documented optional→required move; the fallback for pre-existing Gateway records with a blank image is the env var applied on every reconcile, so no data backfill is required as long as the deployment carries the env vars.
Findings
[Minor] Optional→required image default is an operational breaking change on in-place upgrades — config.go (L43-52) and manifests.go (L91-104). Removing the code-level defaults means a control plane that is upgraded by image tag alone, without re-applying the deployment manifest that now carries GATEWAY_IMAGE / GATEWAY_SUPERVISOR_IMAGE, will start failing reconciliation for every Gateway that relied on the default (gateway image is not configured and no default is available). Within this repo the mitigation is present (both env vars live in deploy/base/controller.yaml, inherited by all overlays), and the error is explicit rather than a panic, so this is not a blocker. Recommend calling the required rollout order out in the PR description / release notes (apply the manifest, not just bump the image) so downstream GitOps/operator consumers add the env vars before cutover. Confidence: High.
[Nit] errors.New messages are actionable but not env-specific — manifests.go L93, L104. Consider naming the missing env var in the message (e.g. "gateway image is not configured: set the Gateway image field or GATEWAY_IMAGE on the control-plane deployment") to shorten operator triage, since this is now the sole failure mode when the default is absent. Confidence: Medium.
Test Diff Scrutiny
validation_test.go adds one new case (a quay.io digest-pinned reference, wantErr: false) and leaves the existing ghcr.io/nvidia/openshell/gateway:0.0.101 case intact. This is additive — no pre-existing assertion was flipped from accept→reject or optional→required — so no guarantee was silently removed.
Cross-PR coordination
Another open pull request replaces the gateway deployment path wholesale — it removes the very ApplyManifestToNamespace placeholder-substitution function this PR edits and switches image resolution to a Helm values mapping, while keeping (and re-pointing) the defaultGatewayImage / defaultSupervisorImage code constants that this PR deletes. These are two incompatible designs for the same image-resolution code and cannot both land as-is:
- #194 — Maintainers must decide which image-resolution model is authoritative: this PR's "no in-code defaults;
GATEWAY_IMAGE/GATEWAY_SUPERVISOR_IMAGErequired, hard-fail otherwise" contract versus #194's "keep hardcoded constants + resolve through Helm chart values." Whichever merges second must be reworked, and if #194 is adopted its values mapping needs to carry (or consciously drop) this PR's required-env-var contract and the Red Hatquay.io/opendatahub/odh-openshell-*digests rather than the developer-registry images it currently pins. A merge-order and design decision is needed before either lands.
Findings Summary (ordered by severity, highest first)
- [Minor] Optional→required image default is an in-place-upgrade breaking change; ensure rollout re-applies the manifest env vars - Reconciliation / Config (config.go L43-52, manifests.go L91-104)
- [Nit] Failure messages could name the missing env var for faster operator triage - Observability (manifests.go L93, L104)
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context (caller) |
Pass |
| Reconcile errors propagated, not swallowed | Pass |
| Image references consistent across manifests/specs | Pass |
| Input validated (image reference regex) | Pass |
| No secrets in logs or responses | Pass |
| Test diff additive (no flipped assertions) | Pass |
| OpenAPI client not manually edited | Pass (N/A) |
| Conventional commit messages | Pass |
e8396c0 to
9f2aae8
Compare
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
COMMENT. This is a clean, well-scoped migration off the NVIDIA/GHCR upstream images to Red Hat (quay.io/opendatahub/...) pullspecs, and it correctly turns the gateway/supervisor images into required, digest-pinned deployment inputs instead of code-baked constants. The Go changes are sound (cmp.Or + explicit errors.New, error propagated up the reconcile stack, additive test case), and the env vars are wired into deploy/base/controller.yaml so every overlay that layers on ../base inherits them. My only actionable items are a couple of operability nits and one cross-PR coordination decision the maintainers should make before merge.
Amber Assessment
What this PR does well
- Removes the hardcoded
defaultGatewayImage/defaultSupervisorImageconstants and replaces theif x != "" { ... }fallback ladder withcmp.Or(config.X, images.DefaultX()), returning an explicit error when nothing resolves. This is a proper reconcile-time failure (not apanic), and it is wrapped with context at the call site (apply substitutions for %s: %w). - Image references are updated consistently across specs, README, kind/lib.sh, e2e scripts, and the deploy overlays. I could not find any lingering reference to the deleted
components/api-server/deploy/...trees or to the oldghcr.io/nvidia/openshell/...defaults in code. - The one modified pre-existing test (
validation_test.go) is additive — it adds atag+digestacceptance case rather than flipping an existing assertion, so no guarantee was silently removed. - Required env vars are set in
deploy/base/controller.yaml, anddeploy/kind,deploy/openshift,deploy/hub, and (transitively)deploy/ibmall compose../base, so the new hard requirement is satisfied on every deploy path.
Findings (none blocking)
- [Minor] No fail-fast on missing image env vars —
DefaultGatewayImage()/DefaultSupervisorImage()now reados.Getenvlazily on every reconcile. If an operator forgets to setGATEWAY_IMAGE/GATEWAY_SUPERVISOR_IMAGE, the controller starts up healthy and then fails every gateway reconcile at runtime. Consider validating presence once at controller startup so misconfiguration surfaces immediately rather than per-gateway. Confidence: Medium. - [Minor] Error messages don't name the missing knob — the two
errors.New("... is not configured and no default is available")messages don't tell the operator which env var to set. IncludingGATEWAY_IMAGE/GATEWAY_SUPERVISOR_IMAGE(or the Gatewayimage/supervisor_imagefield) in the message makes the failure self-diagnosing. Confidence: High.
Cross-PR coordination
Another open pull request reworks the gateway deployment path to install gateways at runtime via the upstream OpenShell Helm chart: it deletes components/control-plane/internal/gateway/manifests.go in its entirety, moves image resolution into a Helm values mapping, and — critically — keeps (and merely re-points) the hardcoded defaultGatewayImage/defaultSupervisorImage constants that this PR deliberately removes. The two PRs encode incompatible designs for the same concern (how the default gateway/supervisor image is resolved and injected): this PR makes the images required, digest-pinned env-var inputs with a hard failure when unset, while the Helm PR relies on non-empty in-code defaults and a different placeholder/values path. They also edit the same lines in internal/gateway/config.go, scripts/kind/lib.sh, and specs/platform/openshell-gateway.spec.md, and set different image pullspecs.
This needs a maintainer decision on merge order and on where the "required, no in-code default" semantics live. If the Helm PR merges first, this PR's env-var-required design, Red Hat pullspecs, and fail-on-empty behavior must be re-applied inside the Helm values mapping (manifests.go will no longer exist). If this PR merges first, the Helm PR must drop its resurrected image constants and carry the required-env-var contract into its values path rather than reintroducing hardcoded defaults. Please coordinate before either lands so the image-resolution contract is not silently reverted.
Findings Summary (ordered by severity, highest first)
- [Minor] No startup validation for required
GATEWAY_IMAGE/GATEWAY_SUPERVISOR_IMAGE; misconfig only surfaces per-reconcile — Operability (config.go L43-L51) - [Minor] Missing-image errors don't name the env var / field to set — Observability (manifests.go L93, L104)
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf/explicit error and context |
Pass |
| Reconcile pattern (fail + propagate, no create-or-skip) | Pass |
| No secrets in logs or error messages | Pass |
| Image references consistent across the stack | Pass |
| Test Diff Scrutiny (modified assertion is additive, not a flipped guarantee) | Pass |
| Config separated from code (images now deployment inputs) | Pass |
| Conventional commit messages | Pass |
Move from the upstream image to the Red Hat downstream ones. Removed the default versions pinned in code and leave the selection only via the env variables to avoid doing a new release only to move the default versions used if a user will not set the variables. Update SKILLs and specs to point to the new source of truth for the images versions and remove stale references.
Merge kustomization manifests in a single location to avoid drift and missing configurations for overlays. Reconcile SKILLs and specs for the new layout and removed duplicated instructions in components/api-server/Makefile to handle all things related to kind in a single location.
9f2aae8 to
bab3708
Compare
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
COMMENT. This is a clean, well-scoped switch from upstream ghcr.io/nvidia/openshell gateway/supervisor images to the Red Hat downstream quay.io/opendatahub/odh-openshell-* digests, plus a consolidation of the deploy tree onto top-level /deploy. The code change is sound and image references are consistent across manifests, scripts, and specs; the only in-repo nits are a stale fleet_id in a touched doc example, and cross-PR coordination is required with two other open PRs that redesign gateway image resolution / restructure the same deploy tree.
What I verified
- Image references consistent across the stack — old
ghcr.io/nvidia/openshell/gateway|supervisorrefs are fully removed; the new digests appear consistently indeploy/base/controller.yaml,scripts/kind/lib.sh, specs, and skills. - Required-image change is fully wired —
DefaultGatewayImage()/DefaultSupervisorImage()now return the raw env var (no hardcoded fallback), andApplyManifestToNamespacefails fast if neither theGatewayConfigfield nor the env var is set. Every overlay (base,openshift,hub,kind,ibm) inherits../base'scontroller.yaml, which setsGATEWAY_IMAGEandGATEWAY_SUPERVISOR_IMAGE, so no in-tree deployment path regresses. This is a good fail-fast improvement over silently defaulting to an upstream image. - Error handling — new error paths use
errors.New(...)and propagate through the reconciler with%wwrapping (apply substitutions for %s: %w); nopanic(), no secret leakage in messages. - Test diff scrutiny — the only test change (
validation_test.go) is purely additive (a new acceptedtag@sha256reference case); no pre-existing assertion was flipped. - Deleted
components/api-server/deploy/**tree — no dangling references remain in Go, Makefiles, scripts, or CI; the removed api-serverkind-*/image*Makefile targets are superseded by the top-levelMakefiletargets and reference an already-removedfleetsendpoint.
Findings
-
[Minor]
skills/deploy/gcp-cluster/SKILL.md:432— theGatewayReleaseexample payload still sends"fleet_id":"$FLEET_ID", but the Fleet entity andfleet_idwere removed onmain(this PR's base). This PR edits that exact line to update the image, so it's the right moment to drop the removed field. Spec/Docs Consistency -
[Minor] Operational note (
components/control-plane/internal/gateway/config.go:44,51) — because the images are now required with no fallback, any out-of-tree/custom control-plane deployment that predates this change must setGATEWAY_IMAGEandGATEWAY_SUPERVISOR_IMAGEor gateway reconciliation will fail. In-tree overlays are covered; worth a one-line callout in the PR/upgrade notes for downstream consumers. Docs Completeness
Cross-PR coordination
Two open pull requests require maintainer coordination or a merge-order decision with this PR:
-
#194 (adopt upstream OpenShell Helm chart for gateway deployments) presents a competing design for the exact mechanism this PR changes. #194 deletes
components/control-plane/internal/gateway/manifests.go(the placeholder-substitution path this PR edits), keeps the hardcodeddefaultGatewayImage/defaultSupervisorImageconstants this PR removes (re-pointing them at different pullspecs), and injects images through Helm values mapping instead of env-var resolution. Maintainers must decide which image-resolution model wins and in which order they land: if #194 merges first, this PR's env-required semantics must be re-expressed as Helm values; if this PR merges first, #194 must carry the "images required, no upstream default" contract into its values mapping rather than reintroducing hardcoded defaults. -
#227 (per-gateway resource quotas / GatewayProfile) adds and modifies manifests under
components/api-server/deploy/kind,.../openshift, and.../ibm— the entire directory tree this PR deletes when consolidating onto top-level/deploy. This is a structural/ownership conflict, not just a text merge: #227's new controller ClusterRBAC (resourcequotas/limitranges) manifests must be re-homed into/deployand its kustomizations rebased onto the unified layout. Maintainers should agree on merge order (ideally this consolidation first) and where #227's RBAC lands.
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf/%w context |
Pass |
| No secrets in logs or error messages | Pass |
| Image references consistent across the stack | Pass |
| Reconcile pattern (not create-or-skip) | Pass |
| Test Diff Scrutiny (no silently flipped assertions) | Pass |
| Conventional commit messages | Pass |
Findings Summary (ordered by severity, highest first):
- [Minor] Stale
fleet_idin a touched doc example - Spec/Docs Consistency (gcp-cluster/SKILL.md:432) - [Minor] Required-image change needs an upgrade callout for out-of-tree deployments - Docs Completeness (config.go:44,51)
| RELEASE=$(curl -sk -X POST "$API/gateway_releases" -H 'Content-Type: application/json' \ | ||
| -H "Authorization: Bearer $TOKEN" \ | ||
| -d "{\"name\":\"openshell-0.0.109\",\"fleet_id\":\"$FLEET_ID\",\"image\":\"ghcr.io/nvidia/openshell/gateway:0.0.109\"}") | ||
| -d "{\"name\":\"openshell-0.0.109\",\"fleet_id\":\"$FLEET_ID\",\"image\":\"quay.io/opendatahub/odh-openshell-gateway:v0.0.109-rhaiv.0\"}") |
There was a problem hiding this comment.
Stale field: this payload still sends "fleet_id":"$FLEET_ID", but the Fleet entity and fleet_id were removed on main (this PR's base, #223). Since you're already editing this line to update the image, please drop fleet_id (and the $FLEET_ID capture above it, if it's now unused) so the example matches the current API. [Minor]
bab3708 to
0fecf3e
Compare
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
COMMENT — This is a clean, well-documented supply-chain change: it retires the NVIDIA OpenShell upstream gateway/supervisor images in favor of digest-pinned Red Hat (quay.io/opendatahub/...) images and makes the image references configuration-driven. The core code change (removing hardcoded image defaults so GATEWAY_IMAGE/GATEWAY_SUPERVISOR_IMAGE become required env vars) is intentional and correctly wired through deploy/base/controller.yaml, README, and specs; my only substantive note is that a missing value fails per-reconcile rather than at startup.
What this PR does well
- Digest-pinned images in
deploy/base/controller.yamlandscripts/kind/lib.sh(v0.0.109-rhaiv.0@sha256:...) give immutable, verifiable pulls — good supply-chain hygiene. - Config separated from code: the image references now live in manifests/env, not Go constants, matching the "separate configuration from code" convention.
- Clean error handling:
manifests.goreturns expliciterrors.New(...)when no image is resolvable, andreconciler.gowraps and propagates it (fmt.Errorf("apply substitutions for %s: %w", ...)) — no silently swallowed failures, no secrets in messages, nopanic(). - Additive test change:
validation_test.goadds a digest-reference case instead of flipping an existing assertion, so no guarantee is silently removed. - Consistent references: the gateway image string matches across
deploy/base/controller.yaml,scripts/kind/lib.sh(seed release), and the docs.
Findings
See inline comments. Summary below.
Findings Summary (ordered by severity, highest first):
- [Major] Required-image env vars (
GATEWAY_IMAGE/GATEWAY_SUPERVISOR_IMAGE) are validated only at gateway-reconcile time, not at controller startup; a misconfigured control plane boots healthy and fails every provisioning. Add a fail-fast startup check. — Control-Plane / Config (config.go L44, L51; manifests.go L93, L104) - [Minor] No unit test covers the new "no image configured -> error" path in
ApplyManifestToNamespace. Add a table case. — Test Coverage (validation_test.go L17)
The optional->required transition is a contract change, but it is explicitly called out in the PR body/README/specs and backfilled in the same PR via deploy/base/controller.yaml (and the IBM overlay), so it does not meet the Blocker bar for an unannounced tightened precondition.
Convention Checklist:
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context / explicit errors |
Pass |
| No secrets in logs or responses | Pass |
| Image references consistent across the stack | Pass |
| Config separated from code | Pass |
| Test Diff Scrutiny (no silently-removed guarantees) | Pass |
| Fail-fast validation of required config | Fail |
Cross-PR coordination
Two open pull requests have material conflicts with this one and need a maintainer decision on merge order and design ownership:
-
PR #194 (adopt upstream OpenShell Helm chart for gateway deployments) is a competing design for the same concern — how gateway/supervisor images are injected — and rests on the opposite assumption about image defaults. This PR (#201) removes the hardcoded
defaultGatewayImage/defaultSupervisorImageconstants and makes the env vars required with no fallback (fail-closed). #194 instead keeps those hardcoded constants (repointing them at a personal dev registry) and deletesinternal/gateway/manifests.goentirely, moving image resolution into a Helm values mapping. Both PRs also editconfig.go's image-default methods andscripts/kind/lib.sh. Maintainers must decide the merge order and, whichever lands second, ensure the "required env var, no hardcoded fallback" policy from #201 is preserved in #194's Helm values path rather than silently reintroducing a fallback (and a non-Red-Hat dev image). -
PR #227 (per-gateway resource quotas) adds and renames ClusterRBAC resources under
components/api-server/deploy/{kind,openshift}/and edits the kustomizations there. This PR (#201) deletes that entirecomponents/api-server/deploy/tree as part of unifying on the repo-rootdeploy/. If #201 merges first, #227's new RBAC lands in a removed, unwired directory and is orphaned; if #227 merges first, #201's deletion must re-home that RBAC intodeploy/. Maintainers must coordinate merge order and decide where the new quota RBAC lives in the unified deploy layout.
| return v | ||
| } | ||
| return defaultGatewayImage | ||
| return os.Getenv("GATEWAY_IMAGE") |
There was a problem hiding this comment.
[Major] DefaultGatewayImage/DefaultSupervisorImage now return the bare os.Getenv result with no fallback, so GATEWAY_IMAGE/GATEWAY_SUPERVISOR_IMAGE move from optional to required. This is a deliberate, documented fail-closed change (README, PR body, specs, and deploy/base/controller.yaml all updated), which is good — but the missing-value error only surfaces per-gateway at reconcile time (manifests.go:93/:104), not at controller startup. A control plane deployed without these env vars boots healthy and then fails every gateway provisioning with a runtime error.
Recommend validating presence once at startup (fail-fast in cmd/hypershell-controller or internal/config) so a misconfiguration is caught at boot rather than on first reconcile. Confidence: Medium. — Control-Plane / Config
| supervisorImage = config.SupervisorImage | ||
| supervisorImage := cmp.Or(config.SupervisorImage, images.DefaultSupervisorImage()) | ||
| if supervisorImage == "" { | ||
| return nil, errors.New("supervisor image is not configured and no default is available") |
There was a problem hiding this comment.
Good: image resolution now uses cmp.Or(config.X, images.DefaultX()) and returns an explicit error when nothing is configured, and the caller wraps and propagates it (reconciler.go:836). No secrets in the message. One note — since the default is now env-only (see config.go:44), this error is the only guardrail against a missing GATEWAY_IMAGE/GATEWAY_SUPERVISOR_IMAGE; pairing it with a startup-time check would make failures easier to diagnose. Confidence: High. — Control-Plane / Error Handling
|
/lgtm |
3 participants
This PR is for using Red Hat images instead of the OpenShell upstream project ones.
It also remove image references in code to avoid having images defined there, now they are required to be set as env variables or inside the gatewayconfig resource.
Right now we will not set MintMaker to update the image until we will have a way to test the full compatibility with the new gateway/supervisor version with more testing.