Skip to content

docs(specs): add OpenShell branch build spec for kind-openshell-up - #148

Draft
squizzi wants to merge 3 commits into
mainfrom
squizzi/kind-openshell-pr-deploy
Draft

docs(specs): add OpenShell branch build spec for kind-openshell-up#148
squizzi wants to merge 3 commits into
mainfrom
squizzi/kind-openshell-pr-deploy

Conversation

@squizzi

@squizzi squizzi commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

Adds specs/platform/openshell-branch-build.spec.md, a behavior contract for a new make kind-openshell-up workflow that builds OpenShell gateway, supervisor, and sandbox base images from a specified branch or PR (OPENSHELL_BRANCH/OPENSHELL_PR/OPENSHELL_REPO), loads them into Kind, and seeds a distinctly named, hypershell.redhat.io/openshell-dev-build-labeled gateway running those images so unreleased OpenShell changes can be validated end-to-end. The spec mirrors the existing deterministic cloud-provider-kind external-source build pattern (isolated checkout, resolve-to-SHA, rebuild on branch movement). It introduces a new sandbox_image Gateway provisioning field (peer to image/supervisor_image, defaulting to the community base) to close the gap where the sandbox default_image is currently hardcoded. For cross-spec consistency, data-model.spec.md gains the sandbox_image field row and local-development.spec.md registers the kind-openshell-up target plus the OPENSHELL_* variables. This is spec-only; no implementation code changes are included.

@squizzi squizzi added amber/self-review This PR was reviewed by the Amber review agent by one of the contributors to the PR. amber/changes-requested Amber requested changes on this PR labels Aug 18, 2026

@squizzi squizzi left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Amber Analysis

This spec is the right shape for a Kind branch-build workflow, but several requirements cannot be implemented as written: sandbox images are not built from the OpenShell repo, sandbox_image is a production Gateway field parked in a local-dev spec, and the control plane has no path to apply the proposed dev-build labels. I believe those contracts need to be closed before this is a safe implementation target.

Assessment: REQUEST_CHANGES (submitted as COMMENT — GitHub does not allow requesting changes on your own PR)
Confidence: High

Findings Summary (ordered by severity, highest first):

  1. [Major] Sandbox base cannot be built from the OpenShell checkout; it lives in OpenShell-Community — Spec Consistency (openshell-branch-build.spec.md L97–L110)
  2. [Major] sandbox_image is a production Gateway/CP field but is defined only here; openshell-gateway.spec.md and specs/index.spec.md are not updated — Spec Consistency (L134–L141, data-model.spec.md L181)
  3. [Major] Dev-build labels/annotations have no Gateway data-model path and will not survive reconcile; Gateway name is not a DNS-1123 identity — Architecture (L162–L174)
  4. [Major] Distinct GatewayRelease is required, or release_id precedence will ignore Gateway.image / break coexistence — Spec Consistency (L114–L118)
  5. [Major] Both OPENSHELL_BRANCH and OPENSHELL_PR is unspecified; OPENSHELL_PR against a fork will fail — Spec Completeness (L38–L40)
  6. [Major] Coexistence contradicts the entry-point “no ref → fail” rule and is ambiguous on add-vs-replace / full kind-up stack — Spec Consistency (L196–L202)

Convention Checklist (omit conventions not applicable to the diff):

Convention Result
Conventional commit message Pass
Separate configuration from code (OPENSHELL_* vars) Pass
Reconcile / update-or-create for the seeded gateway Pass (stated as add-or-update)
Input validated (new sandbox_image field) Fail
Image references specified consistently Fail
Spec registry (specs/index.spec.md) updated Fail
Canonical gateway spec updated for new provisioning field Fail
Full Analysis

This is spec-only (docs(specs): add OpenShell branch build spec for kind-openshell-up). The local-dev entry point, SHA-resolved rebuild, and “fail closed on missing ref / build error” rules are sound and match the cloud-provider-kind pattern.

The sandbox requirement is the hardest stop: OpenShell’s image build script does not produce the community sandbox base. Wiring sandbox_image is the right production gap to close (default_image is hardcoded in components/control-plane/manifests/gateway/configmap.yaml), but that contract belongs in openshell-gateway.spec.md next to supervisor_image, with REST/gRPC/OpenAPI/CLI/migration and ValidateImageReference.

Labels on CP-created workloads need a Gateway-level source of truth; otherwise the next reconcile overwrites them. A fixed name such as openshell-dev-gateway plus a dedicated GatewayRelease keeps coexistence and release_id precedence honest.

Comment thread specs/platform/openshell-branch-build.spec.md Outdated
Comment thread specs/platform/openshell-branch-build.spec.md Outdated
Comment thread specs/platform/openshell-branch-build.spec.md Outdated
Comment thread specs/platform/openshell-branch-build.spec.md Outdated
Comment thread specs/platform/openshell-branch-build.spec.md Outdated
Comment thread specs/platform/data-model.spec.md Outdated
Comment thread specs/platform/openshell-branch-build.spec.md Outdated
squizzi and others added 2 commits August 18, 2026 15:59
Define the make kind-openshell-up workflow that builds OpenShell gateway,
supervisor, and sandbox base images from a specified branch/PR and deploys
a distinctly named, dev-labeled gateway running those images. Adds the
sandbox_image Gateway field to the data model and registers the new target
and OPENSHELL_* variables in the local-development spec.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Kyle Squizzato <kysquizz@redhat.com>
Resolves all major issues identified in PR review:

1. Source ref validation: Specify OPENSHELL_BRANCH takes precedence when both
   vars are set. Clarify PR refs only work against the repo hosting the PR,
   not forks.

2. Sandbox image: Correct the build contract - OpenShell's build script only
   produces gateway and supervisor images. Sandbox base image uses the
   published community image since it's maintained in a separate repo
   (NVIDIA/OpenShell-Community).

3. GatewayRelease: Explicitly omit release_id for branch-built gateways to
   avoid precedence issues (release_id overrides direct image fields per
   data-model.spec).

4. sandbox_image field: Move field definition from this local-dev spec to
   openshell-gateway.spec.md where it belongs. Update data-model.spec to
   reference the gateway spec. Add to gateway.toml mapping and validation.
   Register openshell-branch-build.spec in specs/index.spec.md.

5. Dev gateway identity: Add new Gateway fields (dev_build, dev_build_metadata)
   that the control plane copies to K8s labels/annotations, surviving reconcile.
   Use stable name "openshell-dev-gateway" for update-or-create semantics.
   Specify E2E targets gateway by name or dev_build query.

6. Coexistence: Clarify openshell-dev-gateway coexists with standard dev-gateway.
   When cluster exists, reuse full stack. When it doesn't, create full stack
   (API, CP, Keycloak, dev-gateway) then add openshell-dev-gateway.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Signed-off-by: Kyle Squizzato <kysquizz@redhat.com>

@squizzi squizzi left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Amber Analysis

The follow-up closed the hard stops from the first pass: sandbox is no longer built from the OpenShell checkout, sandbox_image lives in the gateway spec, the seeded name is openshell-dev-gateway, and coexistence is an add-alongside. I believe four contract gaps still block a safe implementation: leftover sandbox wording in local-development.spec.md, an entry-point SHALL that still retargets every Kind gateway, omitting release_id against a required create field, and new production fields with no API/migration contract.

Assessment: REQUEST_CHANGES (submitted as COMMENT — GitHub does not allow requesting changes on your own PR)
Confidence: High

Findings Summary (ordered by severity, highest first):

  1. [Major] local-development.spec.md still says kind-openshell-up builds the sandbox base — Spec Consistency (local-development.spec.md L678, L696)
  2. [Major] Entry-point SHALL still replaces every Kind gateway, contradicting coexistence — Spec Consistency (openshell-branch-build.spec.md L25–L27)
  3. [Major] Omitting release_id is not a valid Gateway create; seed payload is incomplete — Spec Consistency (L155–L170)
  4. [Major] sandbox_image / dev_build / dev_build_metadata have no REST/gRPC/OpenAPI/CLI/migration contract — Spec Completeness (openshell-gateway.spec.md L809–L810)
  5. [Minor] “exactly one source ref” contradicts BRANCH-wins-when-both-set — Spec Consistency (L44–L47)

Convention Checklist (omit conventions not applicable to the diff):

Convention Result
Conventional commit message Pass
Separate configuration from code (OPENSHELL_* vars) Pass
Reconcile / update-or-create for the seeded gateway Pass
Spec registry (specs/index.spec.md) updated Pass
Canonical gateway spec updated for new provisioning fields Pass
Image references specified consistently Fail
Input validated (new Gateway fields) Fail
Full Analysis

Previous Majors that landed: sandbox uses the published community image; sandbox_image moved to openshell-gateway.spec.md with gateway.toml default_image mapping and validation scenario; spec registered in specs/index.spec.md; stable DNS-1123 name openshell-dev-gateway; dev_build / dev_build_metadata as the CP source of truth; BRANCH precedence and fork+PR failure; coexistence creates the full stack then adds the second gateway.

Remaining: local-development.spec.md was not updated to match the sandbox correction. The entry-point requirement still says the Kind environment’s gateways (plural) run branch-built images. POST /gateways currently requires release_id; omitting it needs an API contract change, or a distinct GatewayRelease. Schema-table rows for sandbox_image and the dev-build fields will not drive OpenAPI, gRPC, CLI, or migrations through /reconcile.

| `make kind-teardown` | Destroy the Kind cluster + stop cloud-provider-kind + stop CoreDNS + flush port forwarding rules + revert resolver |
| `make kind-status` | Show cluster info, pods, services, hostnames, DNS status, port forwarding status, and active component swaps |
| `make kind-fix-ports` | Re-establish host port forwarding (443 + 8080) after a cloud-provider-kind restart; re-discovers ephemeral ports and re-runs the stop-then-start flush |
| `make kind-openshell-up` | Build OpenShell (gateway + supervisor + sandbox base) from `OPENSHELL_BRANCH`/`OPENSHELL_PR` + load into cluster (creating it if needed) + seed a dev-labeled gateway running those images. See [`openshell-branch-build.spec.md`](./openshell-branch-build.spec.md) |

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Major] local-development still says this target builds the sandbox base

The follow-up correctly dropped sandbox from the OpenShell checkout. This row (and OPENSHELL_BRANCH at L678) still says “gateway + supervisor + sandbox base.” Implementers of kind-openshell-up will rebuild the contradiction we just closed.

Fix both rows to: build gateway and supervisor from OPENSHELL_BRANCH/OPENSHELL_PR; use the published community sandbox image.

Confidence: High

Comment on lines +25 to +27
The platform SHALL provide a `make kind-openshell-up` target that brings up a
local Kind environment whose OpenShell gateways are built from a caller-specified
OpenShell source ref instead of the pinned default images.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Major] Entry-point SHALL still replaces every Kind gateway

“whose OpenShell gateways are built from a caller-specified OpenShell source ref instead of the pinned default images” is the same all-gateways wording the last review flagged. Coexistence now says dev-gateway stays on pinned images and openshell-dev-gateway is added beside it.

Fix: the target SHALL provision or update openshell-dev-gateway with branch-built images and SHALL NOT retarget dev-gateway.

Confidence: High

Comment on lines +155 to +170
be set via the Gateway's `image` and `supervisor_image` fields directly. The
Gateway MUST NOT have a `release_id` set, because per `data-model.spec.md`,
when both `release_id` and `image` are present, `release_id` takes precedence
and the direct image references would be ignored. The `sandbox_image` field
SHALL reference the published community base image.

The control plane SHALL reconcile the gateway workload so that the running
gateway container and supervisor sidecar use the branch-built images, and launched
sandboxes use the community base image.

#### Scenario: Dev gateway runs branch-built images

- GIVEN a completed OpenShell branch build with gateway and supervisor images
loaded into Kind
- WHEN the dev gateway is seeded and reconciled
- THEN the Gateway resource SHALL have no `release_id` field set

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Major] Omitting release_id is not a valid Gateway create

data-model.spec.md and the OpenAPI/gRPC/CLI contracts require release_id (plus fleet_id, cluster_id, database_id) on create. “MUST NOT have a release_id” cannot be implemented without first making that field optional.

Specify the rest of the seed payload too: reuse Kind’s fleet/cluster; set route: {} so E2E can reach the gateway on *.gw.localhost; say whether database_id is reused as an FK or a distinct ManagedDatabase is created.

Pick one:

  1. Distinct GatewayRelease owned by this gateway, with release_id pointing at it (implementable against today’s API), or
  2. Make release_id optional in data-model / OpenAPI / CLI when image is set, then omit it here.

Confidence: High

Comment on lines +809 to +810
| `dev_build` | No | `false` | Marks this as a dev/branch build. Control plane applies `hypershell.redhat.io/openshell-dev-build` label to K8s resources |
| `dev_build_metadata` | No | - | Dev build provenance (JSONB): `{ref, sha, repo}`. Control plane copies to annotations on K8s resources |

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Major] New production fields have no API/migration contract

sandbox_image, dev_build, and dev_build_metadata are now Gateway schema fields, but the branch-build Purpose still says this feature “does not change how production gateways are provisioned.” A schema-table row is not enough for /reconcile: add REST/gRPC/OpenAPI/CLI fields, gateways migrations, and the hsctl apply Gateway field list in data-model.spec.md.

Also add a sandbox_image default-mapping SHALL next to the existing supervisor_image paragraph (~L675). The validation scenario mentions sandbox_image, but there is no requirement that ValidateImageReference actually covers it.

Confidence: High

Comment on lines +44 to +47
The target SHALL require exactly one source ref. When both `OPENSHELL_BRANCH`
and `OPENSHELL_PR` are set, `OPENSHELL_BRANCH` SHALL take precedence and
`OPENSHELL_PR` SHALL be ignored. When neither is set, the target SHALL fail
with an actionable message explaining how to supply a branch or PR.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Minor] “exactly one source ref” contradicts precedence

The first sentence requires exactly one of OPENSHELL_BRANCH / OPENSHELL_PR. The next sentence allows both, with BRANCH winning. The scenario is unambiguous — change the SHALL to “at least one source ref” and keep the precedence rule.

Confidence: High

@squizzi
squizzi force-pushed the squizzi/kind-openshell-pr-deploy branch from 5200617 to 3941e2c Compare August 18, 2026 23:25
Keep sandbox_image and dev-build fields from this branch. Take main's
0.0.109 image defaults and CloudNativePG database provisioning.

Co-Authored-By: Cursor Grok 4.6 <noreply@cursor.com>
Signed-off-by: Kyle Squizzato <kysquizz@redhat.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@coderabbitai

coderabbitai Bot commented Aug 21, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: 563146dc-1664-4e40-80b2-01aede1eeb4b

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@squizzi squizzi left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Amber Analysis

The follow-up closed the hard stops from the first pass, but the merge with main did not fix the remaining contract gaps, and the new sandbox_imagedefault_image mapping drops the GATEWAY_SANDBOX_IMAGE override that air-gapped clusters already require. I believe those contracts need to be closed before this is a safe implementation target.

Assessment: REQUEST_CHANGES (submitted as COMMENT — GitHub does not allow REQUEST_CHANGES on your own pull request)

Findings Summary (ordered by severity, highest first):

  1. [Major] local-development.spec.md still says kind-openshell-up builds the sandbox base - Spec Consistency (L787, L807)
  2. [Major] Entry-point SHALL still retargets every Kind gateway - Spec Consistency (L27)
  3. [Major] Omitting release_id is not a valid Gateway create - Spec Consistency (L156)
  4. [Major] gateway.toml default_image mapping drops GATEWAY_SANDBOX_IMAGE - Spec Consistency (openshell-gateway.spec.md L638)
  5. [Major] New production fields have no API/migration contract - Spec Completeness (openshell-gateway.spec.md L806)
  6. [Minor] “exactly one source ref” contradicts precedence - Spec Consistency (L44)

Convention Checklist (omit conventions not applicable to the diff):

Convention Result
Image references consistent across manifests Fail (sandbox_image default ignores GATEWAY_SANDBOX_IMAGE)
Separate configuration from code Pass (OPENSHELL_* vars)
Reconcile, don't create-or-skip Pass (stable openshell-dev-gateway name)
Conventional commit message Pass
Full Analysis

Spec-only change for make kind-openshell-up. The branch-build spec itself is now the right shape (sandbox not built from OpenShell, sandbox_image owned by the gateway spec, coexistence as add-alongside). The leftover contradictions above will send /reconcile in two directions at once.

| `make kind-teardown` | Destroy the Kind cluster + stop cloud-provider-kind + stop CoreDNS + flush port forwarding rules + revert resolver |
| `make kind-status` | Show cluster info, pods, services, hostnames, DNS status, port forwarding status, and active component swaps |
| `make kind-fix-ports` | Re-establish host port forwarding (443 + 8080) after a cloud-provider-kind restart; re-discovers ephemeral ports and re-runs the stop-then-start flush |
| `make kind-openshell-up` | Build OpenShell (gateway + supervisor + sandbox base) from `OPENSHELL_BRANCH`/`OPENSHELL_PR` + load into cluster (creating it if needed) + seed a dev-labeled gateway running those images. See [`openshell-branch-build.spec.md`](./openshell-branch-build.spec.md) |

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Major] local-development still says this target builds the sandbox base

When make kind-openshell-up runs with OPENSHELL_BRANCH or OPENSHELL_PR set, this row (and OPENSHELL_BRANCH at L787) still says the target builds a sandbox base image. openshell-branch-build.spec.md now says the sandbox image is the published community image and is not built from the OpenShell checkout. Implementers of this Make target will rebuild a contradiction that the follow-up already closed.

Fix both rows to: build gateway and supervisor from OPENSHELL_BRANCH/OPENSHELL_PR; set sandbox_image to the published community image.

Confidence: High


The platform SHALL provide a `make kind-openshell-up` target that brings up a
local Kind environment whose OpenShell gateways are built from a caller-specified
OpenShell source ref instead of the pinned default images.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Major] Entry-point SHALL still replaces every Kind gateway

When a developer runs OPENSHELL_BRANCH=<ref> make kind-openshell-up on a cluster that already has dev-gateway, this SHALL says the Kind environment's OpenShell gateways are built from that ref instead of the pinned images. The coexistence requirement says dev-gateway stays on pinned images and openshell-dev-gateway is added beside it.

Fix: the target SHALL provision or update only openshell-dev-gateway with the branch-built images.

Confidence: High

The seeded dev gateway SHALL be provisioned with the branch-built images rather
than the pinned defaults. The branch-built gateway and supervisor images SHALL
be set via the Gateway's `image` and `supervisor_image` fields directly. The
Gateway MUST NOT have a `release_id` set, because per `data-model.spec.md`,

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Major] Omitting release_id is not a valid Gateway create

When kind-openshell-up seeds openshell-dev-gateway through the REST create path, this contract says the Gateway MUST NOT have release_id set. openapi.gateways.yaml lists release_id as required on create, and the Create Gateway scenario in data-model.spec.md requires release_id. The seed will be rejected.

Make release_id optional, or seed a dedicated GatewayRelease whose image is the branch-built tag (without changing the standard dev-gateway release). Also specify the rest of the seed payload: reuse Kind's fleet/cluster, and set route: {} so E2E can reach the gateway.

Confidence: High

sandbox_namespace = "<tenant-namespace>"
default_image = "<sandbox-default-image>"
supervisor_image = "<supervisor-image>"
default_image = "<Gateway.sandbox_image or default ghcr.io/nvidia/openshell-community/sandboxes/base:latest>"

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Major] default_image mapping drops GATEWAY_SANDBOX_IMAGE

On clusters whose nodes cannot pull from ghcr.io (for example IBM ROKS), global-architecture.spec.md requires GATEWAY_SANDBOX_IMAGE to override the gateway default_image. This mapping uses only Gateway.sandbox_image or the published ghcr.io/nvidia/openshell-community/sandboxes/base:latest default. If sandbox_image is unset, implementers will ignore the control-plane env override and sandbox pods will fail to pull.

Specify precedence: Gateway.sandbox_image when set, else GATEWAY_SANDBOX_IMAGE / DefaultSandboxImage().

Confidence: High

| `route.host` | No | auto-derived | Hostname for the GRPCRoute |
| `routeAddress` | - | - | Read-only. External address populated by the control plane |
| `dev_build` | No | `false` | Marks this as a dev/branch build. Control plane applies `hypershell.redhat.io/openshell-dev-build` label to K8s resources |
| `dev_build_metadata` | No | - | Dev build provenance (JSONB): `{ref, sha, repo}`. Control plane copies to annotations on K8s resources |

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Major] New production fields have no API/migration contract

When /reconcile implements sandbox_image, dev_build, and dev_build_metadata, this schema table is the only contract. The branch-build Purpose says the feature does not change production provisioning, but these are Gateway fields for every environment. The hsctl apply Gateway field list in data-model.spec.md (~L343) does not include them, and there is no REST/gRPC/OpenAPI/CLI or gateways migration requirement.

Add those contracts (or state that they are in scope for implementation) and qualify the Purpose so it does not deny the schema change.

Confidence: High

`https://github.com/NVIDIA/OpenShell.git`, so that forks and alternate
sources can be targeted.

The target SHALL require exactly one source ref. When both `OPENSHELL_BRANCH`

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Minor] “exactly one source ref” contradicts precedence

When both OPENSHELL_BRANCH and OPENSHELL_PR are set, this paragraph first requires exactly one source ref, then says BRANCH takes precedence and PR is ignored. The “Both source refs are set” scenario is unambiguous — change the SHALL to “at least one source ref” and keep the precedence rule so implementers do not fail the both-set case.

Confidence: High

@jsell-rh

jsell-rh commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator

Amber review

Status: Complete

Verdict

COMMENT - This is a well-structured, spec-only PR that adds a coherent make kind-openshell-up branch-build contract plus three new Gateway data-model fields (sandbox_image, dev_build, dev_build_metadata); the requirements, scenarios, and cross-references are internally consistent and no code changes are included. The main concerns are cross-PR: two other open PRs (#201 and #194) make competing decisions about the exact image-configuration surface this spec builds on, so a maintainer coordination call is needed before implementation lands.

Amber here. Below is the full assessment. Confidence is High on the cross-PR conflicts (verified from diffs) and Medium on the minor spec observations.

Summary

The spec cleanly mirrors the existing deterministic cloud-provider-kind external-source build pattern (isolated checkout, resolve-to-SHA, rebuild on branch movement) and correctly honors the existing release_id > image precedence contract by requiring the seeded gateway to leave release_id unset. The findings are mostly minor consistency/determinism nits within the spec itself, plus material cross-PR conflicts documented below.

Findings

[Minor] Sandbox base image default pins to a mutable :latest tag - Spec Consistency
ghcr.io/nvidia/openshell-community/sandboxes/base:latest is the default for the new sandbox_image field (data-model.spec.md:182, openshell-gateway.spec.md:792, openshell-branch-build.spec.md:120). This directly undercuts the "Deterministic Source Checkout" requirement (openshell-branch-build.spec.md:95-96) that the whole workflow resolve refs to a concrete SHA for reproducibility. Two runs of kind-openshell-up can silently launch sandboxes on different base images. The spec acknowledges the community base lives in a separate repo, so a pinned digest (or an explicit "sandbox base is intentionally floating; here is why" note) would keep the determinism story honest. Confidence: High.

[Minor] Dev image tag SHA length is unspecified / ambiguous - Spec Completeness
The examples use a short dev-abc123 (openshell-branch-build.spec.md:125,132-133) while the "Deterministic Source Checkout" requirement talks about recording the "concrete commit SHA". Short SHAs are not collision-free across a long-lived OpenShell history and make dev-<sha> tags ambiguous when correlating a running gateway back to a commit. Recommend specifying the full 40-char SHA (or an explicit fixed length) for the tag and for dev_build_metadata.sha. Confidence: Medium.

[Minor] Gateway resource-summary row not updated for the new fields - Spec Completeness
data-model.spec.md:343 enumerates the Gateway resource's fields (name, fleet_id, ... service_type) but was not extended with sandbox_image/dev_build/dev_build_metadata (it already omits supervisor_image, so this is a pre-existing drift). Adding at least sandbox_image keeps the summary line consistent with the provisioning-fields table you edited a few rows above. Confidence: Medium.

Convention Checklist

Convention Result
Config separate from code (branch selection via OPENSHELL_* vars, no code edit) Pass
Reconcile/update-or-create semantics (stable openshell-dev-gateway name) Pass
Honors existing contracts (release_id > image precedence verified) Pass
Label/annotation prefix matches hypershell.redhat.io/* convention Pass
Deterministic/reproducible builds Partial (sandbox :latest)
Spec cross-references valid Pass
No secrets in data model (dev_build_metadata is provenance only) Pass

Findings Summary (ordered by severity, highest first)

  1. [Minor] Sandbox base default pins to mutable :latest, at odds with the determinism requirement - Spec Consistency (data-model L182, openshell-branch-build L120)
  2. [Minor] Dev tag SHA length unspecified (short abc123 vs "concrete SHA") - Spec Completeness (openshell-branch-build L125, L132-133)
  3. [Minor] Gateway resource-summary row not updated with new fields - Spec Completeness (data-model L343)

@jsell-rh jsell-rh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Verdict

COMMENT - This is a well-structured, spec-only PR that adds a coherent make kind-openshell-up branch-build contract plus three new Gateway data-model fields (sandbox_image, dev_build, dev_build_metadata); the requirements, scenarios, and cross-references are internally consistent and no code changes are included. The main concerns are cross-PR: two other open PRs (#201 and #194) make competing decisions about the exact image-configuration surface this spec builds on, so a maintainer coordination call is needed before implementation lands.

Amber here. Below is the full assessment. Confidence is High on the cross-PR conflicts (verified from diffs) and Medium on the minor spec observations.

Summary

The spec cleanly mirrors the existing deterministic cloud-provider-kind external-source build pattern (isolated checkout, resolve-to-SHA, rebuild on branch movement) and correctly honors the existing release_id > image precedence contract by requiring the seeded gateway to leave release_id unset. The findings are mostly minor consistency/determinism nits within the spec itself, plus material cross-PR conflicts documented below.

Findings

[Minor] Sandbox base image default pins to a mutable :latest tag - Spec Consistency
ghcr.io/nvidia/openshell-community/sandboxes/base:latest is the default for the new sandbox_image field (data-model.spec.md:182, openshell-gateway.spec.md:792, openshell-branch-build.spec.md:120). This directly undercuts the "Deterministic Source Checkout" requirement (openshell-branch-build.spec.md:95-96) that the whole workflow resolve refs to a concrete SHA for reproducibility. Two runs of kind-openshell-up can silently launch sandboxes on different base images. The spec acknowledges the community base lives in a separate repo, so a pinned digest (or an explicit "sandbox base is intentionally floating; here is why" note) would keep the determinism story honest. Confidence: High.

[Minor] Dev image tag SHA length is unspecified / ambiguous - Spec Completeness
The examples use a short dev-abc123 (openshell-branch-build.spec.md:125,132-133) while the "Deterministic Source Checkout" requirement talks about recording the "concrete commit SHA". Short SHAs are not collision-free across a long-lived OpenShell history and make dev-<sha> tags ambiguous when correlating a running gateway back to a commit. Recommend specifying the full 40-char SHA (or an explicit fixed length) for the tag and for dev_build_metadata.sha. Confidence: Medium.

[Minor] Gateway resource-summary row not updated for the new fields - Spec Completeness
data-model.spec.md:343 enumerates the Gateway resource's fields (name, fleet_id, ... service_type) but was not extended with sandbox_image/dev_build/dev_build_metadata (it already omits supervisor_image, so this is a pre-existing drift). Adding at least sandbox_image keeps the summary line consistent with the provisioning-fields table you edited a few rows above. Confidence: Medium.

Convention Checklist

Convention Result
Config separate from code (branch selection via OPENSHELL_* vars, no code edit) Pass
Reconcile/update-or-create semantics (stable openshell-dev-gateway name) Pass
Honors existing contracts (release_id > image precedence verified) Pass
Label/annotation prefix matches hypershell.redhat.io/* convention Pass
Deterministic/reproducible builds Partial (sandbox :latest)
Spec cross-references valid Pass
No secrets in data model (dev_build_metadata is provenance only) Pass

Findings Summary (ordered by severity, highest first)

  1. [Minor] Sandbox base default pins to mutable :latest, at odds with the determinism requirement - Spec Consistency (data-model L182, openshell-branch-build L120)
  2. [Minor] Dev tag SHA length unspecified (short abc123 vs "concrete SHA") - Spec Completeness (openshell-branch-build L125, L132-133)
  3. [Minor] Gateway resource-summary row not updated with new fields - Spec Completeness (data-model L343)

Cross-PR coordination

I compared #148 against the 23 other open PRs, focusing on those touching the same specs (openshell-gateway.spec.md, data-model.spec.md, local-development.spec.md) or the same gateway image-configuration / provisioning surface. Three PRs warrant a maintainer decision; the rest are non-conflicting.

Other open PRs reviewed: #216, #214, #212, #211, #210, #209, #208, #207, #206, #201, #200, #194, #189, #188, #185, #182, #179, #151, #150, #135, #109, #75, #73.

1. #201 "[HYPERSHELL-45] Update gateway and supervisor openshell images to Red Hat ones" - MATERIAL CONFLICT (competing interface + competing defaults).

  • #201 removes the in-code default image constants (ghcr.io/nvidia/openshell/{gateway,supervisor}:0.0.109) and makes gateway/supervisor images come only from GATEWAY_IMAGE / GATEWAY_SUPERVISOR_IMAGE env vars (config.go), rewrites the data-model / openshell-gateway defaults to Red Hat quay.io/opendatahub/odh-openshell-* digests, and its validation_test.go swaps the accepted example ref away from the NVIDIA ghcr path. #148 instead keeps documenting the NVIDIA ghcr defaults and builds gateway:dev-<sha> / supervisor:dev-<sha> from https://github.com/NVIDIA/OpenShell.git.
  • More importantly, the two PRs choose different mechanisms for the sandbox base image: #201 configures it globally via a GATEWAY_SANDBOX_IMAGE env var on the controller (images.DefaultSandboxImage() in manifests.go, controller.yaml), while #148 introduces a per-Gateway sandbox_image data-model field. These are two competing designs for the same knob.
  • Decision needed: (a) what are the canonical default gateway/supervisor images (NVIDIA ghcr vs Red Hat quay), and (b) is the sandbox base configured globally (env var, #201) or per-Gateway (field, #148), or both (env as default, field as override)? #148's implementation must be reconciled with whichever image-config model #201 establishes.

2. #194 "feat(control-plane): adopt upstream OpenShell Helm chart for gateway deployments" - MATERIAL CONFLICT (changes the substrate #148 builds on + explicitly drops image-change reconcile).

  • #194 deletes the SSA/manifest+configmap deployment path (manifests.go, manifests/gateway/configmap.yaml with the default_image/supervisor_image/SANDBOX_IMAGE_PLACEHOLDER templating) and replaces it with a Helm-Go-SDK install (internal/helm/*, helm_deploy.go, values.go). #148 edits exactly that configmap-style config block (openshell-gateway.spec.md:638 default_image = "<Gateway.sandbox_image or default ...>"), so the plumbing #148 assumes for threading sandbox_image (and copying dev_build/dev_build_metadata labels/annotations onto Deployment/Service) would instead have to flow through Helm chart values.
  • #194's spec explicitly states "Gateway upgrades (image changes, config changes) are not handled at this time." #148 depends on re-running kind-openshell-up after a branch tip advances to rebuild and redeploy the gateway with new dev-<sha> images (openshell-branch-build.spec.md "Branch tip advances between runs"). Under #194 that image change would not be reconciled onto the running gateway.
  • Decision needed: ordering and design. If #194 lands first, #148 must (a) re-express the sandbox_image / dev_build / dev_build_metadata wiring as Helm values/labels rather than configmap placeholders, and (b) either #194 must gain image-change reconcile or #148 must recreate the dev gateway on rebuild.

3. #151 "spec(control-plane): gate gateway re-provisioning on desired-state convergence" - COORDINATION (compatible, but the field set must be reconciled).

  • #151 adds Gateway generation/observed_generation tracking and enumerates the desired-spec fields whose change bumps generation (image, supervisor_image, oidc, route, ... in desiredStateChanged()). #148 adds a new desired-spec field, sandbox_image (and metadata fields dev_build/dev_build_metadata). Whichever merges second must add sandbox_image to #151's generation-triggering set, otherwise a sandbox_image change would not advance generation and the control plane would never re-provision - which would also break #148's "branch tip advances -> rebuild -> redeploy" expectation if the only change were the sandbox image.
  • This is complementary rather than opposed (#151's generation model actually makes #148's rebuild-on-tip-move behavior work cleanly, unlike #194), but it needs a conscious "add the new fields to the generation set" step.

Non-conflicting overlaps (noted, no decision needed): #150 ("build LOCAL_IMAGES from working tree") and #148 both edit the local-development.spec.md env-var/target tables, but they address distinct concerns - #150 builds HyperShell's own component images from the working tree, #148 builds OpenShell gateway images from an external repo; they compose. #185 and #200 edit index.spec.md / control-plane specs and #211 edits kind scripts, but none touch #148's image-config or branch-build surface. These are ordinary text-merge overlaps, not design conflicts.

The platform SHALL build the gateway and supervisor images from the checked-out
OpenShell source using the repository's `docker-build-image.sh` script, which
produces the `gateway` and `supervisor` targets. The sandbox base image SHALL
use the published community image (`ghcr.io/nvidia/openshell-community/sandboxes/base:latest`)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Minor] Determinism gap. The sandbox base defaults to the mutable :latest tag, which contradicts the "Deterministic Source Checkout" requirement just above (resolve-to-SHA, reproducible rebuilds). Two kind-openshell-up runs could launch sandboxes on different base images without any ref change. Consider pinning to a digest, or explicitly document that the sandbox base is intentionally floating and why (separate community repo).

covered by this checkout.

Each branch-built image SHALL be tagged with a dev tag that encodes the resolved
commit SHA (e.g., `gateway:dev-abc123`, `supervisor:dev-abc123`) so branch builds

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Minor] Tag SHA length is ambiguous. Examples use a short dev-abc123, but the checkout requirement records the "concrete commit SHA". Short SHAs are not collision-free over OpenShell's history and make it harder to map a running gateway back to a commit. Recommend specifying the full 40-char SHA for both the dev-<sha> tag and dev_build_metadata.sha.

|---|---|---|
| `image` | string | Gateway container image reference (e.g., `ghcr.io/nvidia/openshell/gateway:21da343c9f838bd9ac85dc61bf44889de1a72873`) |
| `supervisor_image` | string | Supervisor sidecar container image (default: `ghcr.io/nvidia/openshell/supervisor:0.0.109`) |
| `sandbox_image` | string | Sandbox base image the gateway uses when launching sandboxes (default: `ghcr.io/nvidia/openshell-community/sandboxes/base:latest`). See [`openshell-gateway.spec.md`](./openshell-gateway.spec.md) |

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Minor] Default uses :latest. Same determinism concern as in openshell-branch-build.spec.md - a mutable default here means sandbox_image is not reproducible unless every caller overrides it.

Cross-PR: #201 configures the sandbox base globally via a GATEWAY_SANDBOX_IMAGE env var (images.DefaultSandboxImage()), whereas this PR adds it as a per-Gateway field. Maintainers should decide whether the sandbox base is a global default, a per-Gateway field, or both (env default + field override).

| `route_address` | text | Read-only external address populated by the control plane (e.g., `grpcs://hostname:443`) |
| `database` | JSONB | Database backend config: `{storageSize, image, externalSecretRef}` |
| `credential_driver` | JSONB | Credential storage driver config: `{type, kubernetes_secrets, vault}`. See [`openshell-gateway-credentials.spec.md`](./openshell-gateway-credentials.spec.md) |
| `dev_build` | boolean | Marks this Gateway as a dev/branch build (default: false). Control plane copies to `hypershell.redhat.io/openshell-dev-build` label on K8s resources. See [`openshell-branch-build.spec.md`](./openshell-branch-build.spec.md) |

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Cross-PR coordination (#151). #151 adds Gateway generation tracking and enumerates the desired-spec fields that bump generation in desiredStateChanged(). sandbox_image (added above) is a desired-spec field and must be included there, or a sandbox-image change won't trigger re-provisioning - which would also break this spec's "branch tip advances -> rebuild -> redeploy" behavior when only the sandbox image differs. dev_build/dev_build_metadata are metadata; decide explicitly whether they should advance generation.

sandbox_namespace = "<tenant-namespace>"
default_image = "<sandbox-default-image>"
supervisor_image = "<supervisor-image>"
default_image = "<Gateway.sandbox_image or default ghcr.io/nvidia/openshell-community/sandboxes/base:latest>"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Cross-PR conflict (#194). This config block (default_image/supervisor_image in the gateway configmap) is exactly what #194 removes when it replaces the SSA/manifest+configmap deployment path with a Helm-Go-SDK install. If #194 lands first, the sandbox_image -> default_image wiring described here must be re-expressed as Helm chart values. #194 also states image/config changes are not reconciled on existing gateways, which conflicts with this PR's rebuild-and-redeploy-on-tip-move requirement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

amber/changes-requested Amber requested changes on this PR amber/self-review This PR was reviewed by the Amber review agent by one of the contributors to the PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants