[HYPERSHELL-259] feat: reconcile gateway version for CLI installation - #210
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: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
jsell-rh
changed the title
Amber reviewStatus: Complete VerdictThis is a clean, well-tested web-console change that adds a gateway-version-matched OpenShell CLI install command, reorders the Connection tab so installation precedes the combined registration + provider block, and removes the service-account shortcut - all reflected in the spec and covered by additive tests. I recommend it for merge after two Minor cleanups; the main action item is cross-PR coordination with #208, which restructures the same Connection tab and still consumes the pre-rename Amber Analysis The refactor of Findings[Minor] Translator description contradicts the rendered order - Content / i18n [Minor] Cross-PR coordinationI reviewed the other open PRs in
No material conflict with the other open PRs. The dashboard PRs (#209/#214) and the deps/i18n PRs touch 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 is a clean, well-tested web-console change that adds a gateway-version-matched OpenShell CLI install command, reorders the Connection tab so installation precedes the combined registration + provider block, and removes the service-account shortcut - all reflected in the spec and covered by additive tests. I recommend it for merge after two Minor cleanups; the main action item is cross-PR coordination with #208, which restructures the same Connection tab and still consumes the pre-rename EditableCommand/buildSetupScript API this PR replaces.
Amber Analysis
The refactor of EditableCommand -> CommandBlock (making marker/label/value props optional so a static command can reuse the same highlighted, copyable block) is a good consolidation, and the user-supplied endpoint is correctly passed through shellArgument() before it lands in the generated command (with a test that injects a space). Test diff scrutiny came back clean: the modified assertions track real behavior changes (prereq copy, step ordering, service-account link removal) and are all mirrored in the spec and PR description, not silent contract removals - and the endpoint-undefined readiness case that moved out of gateway-connection-steps.test.tsx is still covered by the unit test in gateway-connections.test.ts.
Findings
[Minor] Translator description contradicts the rendered order - Content / i18n
connectionInstallPrereq's description says the note is "shown after gateway registration and before provider setup" (messages.ts:102, en.json:156), but the component renders the prerequisite alert (and its install command) before the combined registration+provider block, and the test asserts exactly that (installationIndex < registrationIndex). The description gives translators the wrong placement context; align it with the actual "before the one-time setup commands" order the spec mandates.
[Minor] install.sh is fetched from main while the CLI version is pinned - Supply chain (Confidence: Low)
buildOpenShellInstallCommand pins OPENSHELL_VERSION to the gateway's reported version but pulls the installer itself from .../NVIDIA/OpenShell/main/install.sh (gateway-connections.ts:80-81,104). A future breaking change to install.sh on main could desync from the pinned version. This matches the command verbatim added to architecture.spec.md in this same PR, so it is intentional - flagging only so maintainers consciously accept pulling the installer from an unpinned ref.
Cross-PR coordination
I reviewed the other open PRs in openshift-online/hypershell (#216, #214, #212, #211, #209, #208, #207, #206, #201, #200, #194, #189, #188, #185, #182, #179, #151, #150, #148, #135, #109, #75, #73). One material conflict:
- #208 "[HYPERSHELL-129] feat(web-console): Adding section to gateway UI with instructions for sandbox connecting" - This is a genuine design/interface conflict, not just a file-merge overlap. Both PRs restructure the same Connection tab and edit the same shared files (
editable-command.tsx,gateway-connection-steps.tsx,gateway-connections.ts,gateway-connection-steps.test.tsx,gateway-connections.test.ts,messages.ts,locales/en.json). Critically, #210 renames the shared exportsEditableCommand -> CommandBlockandbuildSetupScript -> buildOneTimeSetupScript, while #208 still imports and uses the oldEditableCommandandbuildSetupScriptto add its "Connect to a sandbox" step. Whichever merges first, the other must rebase onto the renamed component API and the reordered step layout - and both are inserting/reordering children inside the sameGatewayConnectionStepsstep list. Maintainers should decide a merge order and have the later PR adopt this PR'sCommandBlock/buildOneTimeSetupScriptAPI rather than reintroducing the old names.
No material conflict with the other open PRs. The dashboard PRs (#209/#214) and the deps/i18n PRs touch locales/en.json/messages.ts only additively (no competing design), #206 (hsctl login) is a separate Go CLI and does not compete with this UI's "register the gateway" copy change, and #216 (OpenShift Route console) changes how the control plane publishes the gateway endpoint but keeps the same endpoint field this UI consumes.
Findings Summary (ordered by severity, highest first)
- [Minor] Translator
descriptionforconnectionInstallPrereqsays "after gateway registration" but the alert renders before registration - Content / i18n (messages.ts L102, en.json L156) - [Minor] Installer pulled from
NVIDIA/OpenShell/main/install.sh(unpinned) while CLI version is pinned - Supply chain (gateway-connections.ts L80-81, L104)
Convention Checklist
| Convention | Result |
|---|---|
| Input validated / shell arguments quoted | Pass |
| No secrets in logs or responses | Pass |
| Reuse shared/PatternFly UI components (no duplicate component) | Pass |
| Config separate from code (i18n messages, URLs) | Pass |
| Test Diff Scrutiny (modified assertions justified, no hidden contract removal) | Pass |
| Spec updated to match behavior | Pass |
| Conventional commit messages | Pass |
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
This is a well-structured feature: the runtime version is observed from the gateway /health endpoint, stored through a dedicated field-owned gRPC write that a whole-row Replace cannot clobber, and the new concurrent health loop is correctly guarded (route-timer maps behind mu, Keycloak token returned as a locked snapshot). I found no blockers; the notes below are minor, plus one deploy change that is out of scope for this feature and overlaps a dedicated PR.
Strengths
SetGatewayVersionis atomic (UPDATE ... WHERE gateway_version IS DISTINCT FROM ?) and emits the update event only on a real change;Replacenow omitsGatewayVersionalongsideActiveSandboxCount, andgrpc_integration_test.goproves a whole-row update cannot overwrite the reconciled version. Good use of the transactional-outbox pattern.- The move to 4 bounded workers is race-safe: the shared route-state maps are all accessed under
h.mu, andkeycloak.Client.ensureTokennow returns a token snapshot instead of letting callers readc.tokenunlocked. - Version observation is defensively bounded: 3s timeout, redirects rejected (
http.ErrUseLastResponse), response body size-limited, control characters and over-long values rejected, and observation failures are logged without perturbingphase/status. - Spec, data-model, OpenAPI, proto, SDKs, and web console are all updated consistently, and the read-only field is documented as control-plane-owned.
Minor findings
-
Per-tick, per-gateway health-access reconcile (
components/control-plane/internal/reconciler/gateway_version.go:43) -ReconcileGatewayHealthAccessruns for every ready gateway on every tick, issuing a Service+NetworkPolicyGet(and possibleUpdate) each pass. This is bounded and correct, but the owned resources rarely drift; consider reconciling them less frequently (e.g. only on create/first-observe or on a longer cadence) to reduce steady-state API traffic across a large fleet. Confidence: Medium. -
Unrelated deploy/auth change bundled into this feature PR (
deploy/openshift/kustomization.yaml:80-81) - addingAPI_ENV=development_oidcand restructuring the env patch to append is an OpenShift JWT-auth fix, not part of gateway-version reconciliation. It also duplicates a dedicated PR (see Cross-PR coordination). Consider dropping it here so the feature PR stays focused. Confidence: High. -
Sandbox NetworkPolicy no longer allows port 8081 (
components/control-plane/internal/gateway/reconciler.go,openshell-gateway-allow-sandbox-v2) - health access on 8081 is now restricted to the controller-only policy. The spec was updated to match (sandboxes need only gRPC 8080), so this looks intentional; please confirm no sandbox workload relies on reaching the gateway health port. Confidence: Medium.
Cross-PR coordination
- The dedicated OpenShift JWT-auth fix (
API_ENV=development_oidc) that this PR adds in its final commit todeploy/openshift/kustomization.yamlis the entire subject of another open pull request that changes the same file for the same purpose. This is a duplicate/competing solution: maintainers must decide which PR owns the deploy-env change and coordinate merge order, because the second to merge will conflict and carry a redundant fix. - Another open pull request adds a new "Connect to a sandbox" section to the same gateway Connection-tab components this PR restructures. This PR renames the shared
EditableCommandcomponent toCommandBlock(making its props optional) and renamesbuildSetupScripttobuildOneTimeSetupScriptingateway-connections.ts. That is a competing interface change to shared UI code the other PR builds on; whichever merges second must be updated to the renamed component/API, so the owners need to agree on the final component surface and a merge order. - Another open pull request is stacked on this branch and asserts, in an e2e test, the exact console-recommended install command that this PR's
buildOpenShellInstallCommandproduces (including the--suffix stripping) and consumes the new read-onlygateway_versionfield. It must merge after this PR, and any change reviewers request to the install-command format here has to be mirrored there; coordinate the command contract and merge order.
Findings Summary (ordered by severity, highest first)
- [Minor] Unrelated OpenShift JWT-auth deploy change bundled into a version-reconcile feature PR - Scope / Cross-PR (deploy/openshift/kustomization.yaml:80)
- [Minor] Per-tick, per-gateway reconcile of owned health Service/NetworkPolicy adds steady-state API traffic - Performance (gateway_version.go:43)
- [Minor] Confirm no sandbox relies on gateway port 8081 after tightening the sandbox NetworkPolicy - Spec Consistency (reconciler.go)
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
errors.IsNotFound handled for 404 scenarios |
Pass |
| No secrets in logs or responses | Pass |
| Input validated (version length/control chars, required ID) | Pass |
| Reconcile pattern used (update-or-create) | Pass |
| Status/field updated only on change; error paths logged | Pass |
| Concurrency safe (shared maps + Keycloak token guarded) | Pass |
OpenAPI client not manually edited (make generate) |
Pass |
| Image references consistent across manifests | Pass |
| Conventional commit messages | Pass |
| Test diff scrutiny (assertions additive, no flipped guarantees) | Pass |
amber-review-bot
left a comment
amber-review-bot
left a comment
There was a problem hiding this comment.
Verdict
Carried-forward assessment: COMMENT.
This entry carries forward the existing Amber review of commit 6f43074361614cb97889c7ff7109f3dbf7be27b2.
Original Amber review by @jsell-rh. The original review contains the findings and inline comments.
No new analysis was performed for this migration entry.
Amber reviewStatus: Complete |
Adapt the installation test from PR #219 to the current CI flow. Always install in CI and compare complete version values.
Amber reviewStatus: Complete |
amber-review-bot
left a comment
amber-review-bot
left a comment
There was a problem hiding this comment.
Verdict
This PR adds a read-only gateway_version field that the health reconciler observes from the gateway's internal /health endpoint and writes back through a dedicated, RBAC-restricted, field-owned gRPC path, then drives an OpenShell CLI install command in the console. The implementation is careful and convention-compliant (bounded HTTP with redirect rejection and size/length/control-char validation, atomic IS DISTINCT FROM write with transactional-outbox event, service-account-only RPC, restrictive NetworkPolicy/Service reconciliation, and a mutex added to make the now-concurrent health loop safe). I found no blocking defects; my notes are minor, plus cross-PR coordination items maintainers should resolve before merge.
Strengths
- Secret/observability hygiene: observed version is validated for control characters, capped in length, response body is bounded, and redirects are rejected. No secrets logged.
- Field ownership:
ReplaceomitsGatewayVersion(likeActiveSandboxCount), the DAO write is atomic and only emits an event when the value changes, andSetGatewayVersionis added to the service-account-only method set with a matching interceptor test. - Concurrency correctness: moving the health loop from serial to four workers is backed by a new
sync.Mutexguarding the route-timer maps and by reworkingkeycloak.ensureTokento return a token snapshot under lock (the previous code readc.tokenoutside the lock, which would race under concurrency). - Migration is additive and nullable (
ADD COLUMN IF NOT EXISTS gateway_version TEXT); no existing test flips an optional field to required, so no backfill gap.
Minor notes (non-blocking)
- Per-tick access reconciliation cost -
reconcileGatewayVersioncallsReconcileGatewayHealthAccessfor every Ready gateway on every 30s tick, which performs severalGet/potentialUpdatecalls (Service, controller policy, three legacy policies). This is correct reconcile behavior, but at fleet scale it multiplies API-server/K8s calls each tick; consider a lighter cadence or a "converged" short-circuit once the legacy cleanup is done. (Minor, observability/scale.) - Shared 3s access timeout -
gatewayHealthAccessTimeout = 3swraps the Service plus network-policy reconciliation plus three legacy-policy edits, each withRetryOnConflict. Under conflict-heavy conditions this shared budget can abort mid-reconcile and skip the version observation for that tick. It self-heals next tick, so low severity, but a per-operation timeout would be more predictable. (Minor.) - Concurrent Keycloak client use - the token race is fixed, and
go test -raceis reported clean. Please confirm no other mutable state on the sharedConsoleClientCheckeris touched concurrently by the four workers now that the loop is parallel. (Low confidence, verification request.)
Cross-PR coordination
- #269 introduces
provisioning_conditionsasoptional/repeatedfield number 23 in theGatewayproto message, the same field number this PR assigns togateway_version. Both also add a new gateway migration and a new OpenAPIGatewayproperty. This is a direct competing interface change: whichever merges second must renumber its proto field (to 24) and reconcile the migration ordering and generated SDK/OpenAPI output. Maintainers should pick a merge order and assign non-overlapping field numbers. - #261 independently rewrites the same
reconcileGatewayHealthDegraded/Provisioning determination (adding a deployment-not-ready grace timer and new per-gateway timer maps) while this PR changes that function's signature and makes the loop concurrent. The two designs collide on the same function and onopenshell-gateway-health.spec.md; a maintainer decision is needed on which lands first and on ensuring #261's new timer maps are guarded by the mutex this PR introduces. - #219 provides the e2e "install the console-recommended CLI" test and is stacked on this branch, but this PR now incorporates that same install test inline. This is a duplicate solution: maintainers should decide whether #219 is closed/reworked after this merges, or whether the install test is dropped here in favor of #219, to avoid two implementations of the same e2e step.
Findings Summary (ordered by severity, highest first):
- [Minor] Full health-access reconciliation runs for every Ready gateway every tick, multiplying K8s calls at fleet scale - Observability/Scale (gateway_version.go L43)
- [Minor] Single 3s timeout shared across Service + policy + three legacy-policy edits can abort mid-reconcile under conflicts - Reliability (health_access.go L46)
- [Minor] Confirm no other shared mutable state on
ConsoleClientCheckerunder the new concurrent workers - Concurrency (health.go L70)
Convention Checklist:
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
errors.IsNotFound / record-not-found handled |
Pass |
| No secrets in logs or responses | Pass |
| Input validated (size/length/control chars, redirect rejection) | Pass |
| SecurityContext / NetworkPolicy scoping | Pass |
| Reconcile pattern (get-then-create/update) | Pass |
| Image references consistent | Pass |
| OpenAPI/SDK generated, not hand-edited | Pass |
| Test Diff Scrutiny (no optional->required flip without fallback) | Pass |
| Conventional commits | Pass |
Amber reviewStatus: Stopped The pull request head changed before Amber posted the review. A later job can review the new head. |
Amber reviewStatus: Complete |
amber-review-bot
left a comment
amber-review-bot
left a comment
There was a problem hiding this comment.
Verdict
This PR adds a read-only gateway_version field, a control-plane health observer that reports the gateway's runtime version, and a CLI-only installer wired into the console; the code is well structured, defensively written, and thoroughly tested (bounded HTTP reads, redirect rejection, DNS-label validation, field-owned atomic writes, SA-only RPC, checksum-verified download). I found no blockers or security regressions; the notes below are minor, plus cross-PR coordination that needs a maintainer decision.
Amber Analysis
The change follows HyperShell conventions closely. Secret handling is clean (no secrets logged), input is validated (IsDNS1123Label on the namespace, version regex in the installer, control-character/length checks on the observed version), errors are wrapped with context, errors.IsNotFound/ErrRecordNotFound are handled, and reconciliation uses update-or-create with retry-on-conflict rather than create-or-skip. The write path is correctly narrowed: Replace omits GatewayVersion, a dedicated SetGatewayVersion DAO does an atomic IS DISTINCT FROM update with a transactional-outbox event, and the gRPC method is added to the service-account-only allowlist. The concurrency change (serial health loop to four bounded workers) is handled with care: the previously "serial, no sync needed" Keycloak client now returns a locked token snapshot, and all shared reconciler maps (routeNotReadySince, routeTornDown, routeVerifiedAt, healthAccessCheckedAt) are guarded by h.mu. Tests are additive and prove the new guarantees (SA-only RPC rejection, access-check cadence, retry after failed observation, checksum-failure handling); no pre-existing assertion was flipped to hide a removed guarantee.
Minor findings
proto/hypershell/v1/gateways.protoallocates field 24 and intentionally leaves 23 unused for a sibling change. If that sibling allocation shifts, an unrelated future field could silently reuse 23. Considerreserved 23;to make the reservation explicit and wire-safe.healthAccessCheckDuereads then later records under separate lock acquisitions, so two workers processing gateways that share a namespace could both run the (idempotent) access reconcile in the same pass. Harmless today given per-gateway namespaces; noting only in case namespaces are ever shared.
Cross-PR coordination
- #269 introduces
provisioning_conditionson the sameGatewayprotobuf message and regenerates the same shared artifacts (gateways.pb.go, the OpenAPI model), adds a new migration registered in the sameplugin.go, and edits the samegrpc_handler.go/grpc_presenter.go/presenter.go/model.go. This PR reserves proto field 24 and leaves 23 for that work, and claims distinct migration IDs. Maintainers must confirm the 23-vs-24 field allocation and migration-ID ordering, and whichever PR merges second must regenerate the code-generated files against the other's field. - #261 modifies the same gateway health reconciliation pass. This PR restructures
reconcileGatewayHealth(it now returns(namespace, ready), runs under bounded concurrent workers, and drives version observation), while #261 adds provisioning readiness timers to that same function to fix the intermittent Degraded status. These are incompatible in-place designs; maintainers must decide the merge order and ensure the version-observation step is preserved when the two are reconciled. - #219 adds an e2e OpenShell CLI installation test and edits the same e2e workflow/lib. This PR incorporates that test (adapted, original author retained) and the surrounding installer path, making #219 redundant once this merges. Maintainers must decide whether to close #219 or land it first.
Findings Summary (ordered by severity, highest first):
- [Minor] Proto field 23 is left unused for a sibling change without an explicit
reservedguard - Interface Safety (gateways.proto L32) - [Minor]
healthAccessCheckDuecheck-then-record is non-atomic across workers sharing a namespace - Concurrency (gateway_version.go L46, L163)
Convention Checklist:
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
errors.IsNotFound / record-not-found handled |
Pass |
| No secrets in logs or responses | Pass |
| Input validated (DNS label, version, URL/redirect/size) | Pass |
| Reconcile pattern (update-or-create, retry-on-conflict) | Pass |
| Restricted write path / SA-only RPC | Pass |
| Concurrency safety (shared state under mutex) | Pass |
| Image/reference and manifest consistency | Pass |
| OpenAPI/proto generated, not hand-edited | Pass |
| Test diff scrutiny (no flipped guarantees) | Pass |
| Conventional commits | Pass |
Amber reviewStatus: Complete |
amber-review-bot
left a comment
amber-review-bot
left a comment
There was a problem hiding this comment.
Verdict
The gateway-version feature is well engineered: the observed runtime version is a control-plane-owned, RBAC-restricted, atomically-written field, the health HTTP client is hardened (DNS-label validation, bounded body, redirect rejection, per-op timeouts), and the newly concurrent health loop is correctly synchronized (all shared maps guarded by mu, Keycloak token now returned as a locked snapshot). I found no blockers or critical issues; the main items needing a human decision are cross-PR coordination (merge order and shared proto/data-model boundaries), plus one minor supply-chain note.
What I checked and liked
- Ownership / atomic write:
SetGatewayVersionuses anUPDATE ... WHERE ... IS DISTINCT FROM ... RETURNINGinside a transaction with a transactional-outbox event, andReplacenow omitsGatewayVersion(likeactive_sandbox_count) so whole-row updates cannot clobber the reconciled value. Thegrpc_integration_test.gocase proves a subsequentUpdateGatewaydoes not overwrite it. - RBAC:
SetGatewayVersionis added toisServiceAccountOnlyMethod, andTestUnaryInterceptor_GatewayVersionRestrictedToServiceAccountprovesgateway:owner/gateway:creatorhumans are denied while the control-plane SA is allowed. - Input validation / hardening: namespace validated with
IsDNS1123Label, response size capped, redirects rejected, control chars and over-length versions rejected, HTTPS-only checksum-verified installer. - Concurrency: worker pool dedups gateway IDs; all route-timer maps and
healthAccessCheckedAtare mutex-protected; KeycloakensureTokenreturns the token under lock, removing the previous serial-only assumption safely. - Migration: additive nullable
TEXTcolumn viaADD COLUMN IF NOT EXISTS, unique ID, idempotent rollback. Read-only field with pointer semantics means pre-existing rows (NULL) need no backfill. - Test diff scrutiny: modified pre-existing tests are additive (new field, new assertions); no existing assertion was flipped from accept to reject and no optional->required tightening of pre-existing data was introduced.
Findings
Minor
- Supply chain / reproducibility -
packages/gateway-management-ui/src/gateways/gateway-connections.ts:82: the console-recommended command pipescurl .../main/scripts/install-openshell.sh | sh, pinned to themainbranch. The recommended command therefore always fetches whatever is onmainat run time, so a future incompatible edit to the script silently changes every gateway's install instructions and there is no reproducible pin. Consider pinning to a released tag/commit of the script (or serving it from a versioned path) so a given gateway version maps to a fixed installer.
Cross-PR coordination
- #269 introduces
provisioning_conditionson the sameGatewayprotobuf message and adds its own gateways-table migration. This PR takes proto field 24 forgateway_versionand deliberately leaves field 23 unreserved for #269'sprovisioning_conditions. Maintainers must ensure #269 actually claims field 23 (not 24) and that the two gateways-table migration IDs stay distinct, then regenerate the SDK/OpenAPI/pb artifacts once, since both PRs edit the same generated files. This is a shared data-model/interface boundary that needs a merge-coordination decision, not an independent merge. - #261 modifies the same health-reconciliation pass (
components/control-plane/internal/reconciler/health.go) that this PR restructures from a serial loop into a bounded concurrent worker pool that also drives version observation. These are competing designs for the same loop with an ordering dependency: whichever merges second must be reconciled by hand so the version-observation step (and the mutex-guarded shared state) is preserved. Maintainers should decide and enforce the merge order and re-integration. - #219 contributes the e2e "install CLI via the console-recommended command" test that this PR now incorporates directly. The #219 author and maintainers should decide whether #219 is rebased/closed as subsumed once this PR merges, to avoid landing the same test twice.
Findings Summary (ordered by severity, highest first)
- [Minor] Console install command pins the installer script to the
mainbranch, so it is neither reproducible nor version-locked - Security / Supply Chain (gateway-connections.ts:82)
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
errors.IsNotFound / not-found handled |
Pass |
| No secrets in logs or responses | Pass |
| Input validated (K8s DNS label, response bounds) | Pass |
| Reconcile pattern (update-or-create, atomic writes) | Pass |
| SecurityContext / least-privilege NetworkPolicy | Pass |
Proper context propagation (no context.TODO()) |
Pass |
| Concurrency correctly synchronized | Pass |
| RBAC restricts control-plane-only mutation | Pass |
| OpenAPI/proto regenerated, not hand-edited | Pass |
| Migration additive / idempotent, no backfill gap | Pass |
| Test Diff Scrutiny (no silently flipped assertions) | Pass |
| Conventional commit messages | Pass |
| "https://docs.nvidia.com/openshell/about/installation"; | ||
|
|
||
| const installScriptUrl = | ||
| "https://raw.githubusercontent.com/openshift-online/hypershell/main/scripts/install-openshell.sh"; |
There was a problem hiding this comment.
[Minor] Supply chain / reproducibility. The console-recommended install command pipes this script from the main branch (curl .../main/scripts/install-openshell.sh | sh). Because it tracks main, any later incompatible change to install-openshell.sh silently rewrites the install instructions for every gateway, and a given gateway version has no reproducible installer pin. Consider pinning to a released tag/commit or serving a versioned path so a gateway version maps to a fixed installer script.
3 participants
Jira: HYPERSHELL-259
Ready gateways now publish their observed runtime version in the read-only
gateway_versionfield. The console uses that value to show an OpenShell CLI installation command. For example,v0.0.109-rh9a8f8selects the upstream CLI releasev0.0.109, while the API keeps the full reported value./healthendpoint with bounded requests, response-size limits, and redirect rejection. Keep the last value if observation fails.SetGatewayVersion.~/.local/bin. Add that directory to the current shell PATH. The upstream installer starts a local gateway; its TLS status check failed in all three live CI jobs. Remove the first-and all following text from the installer version, and add a leadingvif needed. The command requires neither an installed OpenShell CLI norjq.The branch is updated against main through
67a1e80c. API and SDK output was regenerated. Fleet remains removed. The duplicate OpenShift environment change was dropped because #232 already supplies it. Desired image selection from #238 and release reconciliation from #235 remain separate from observed runtime version reporting.Validation on this branch:
make checkthrough commit and push hooksgo test -race ./...API_ENV=integration_testing go test ./plugins/gateways -count=1make ci-test: all six shell test files passedv0.0.109CLI archiveThe original nine Amber review threads and the two follow-up threads are addressed. Shared Keycloak configuration and its HTTP client stay unchanged after construction. The mutex protects the token and its expiry. The concurrent token test passes under the race detector.
Related PRs:
gateway_version. The migration IDs are distinct.The API migration setup error reported in the previous PR description is fixed. The new column migration uses explicit SQL. Both CI gates and all three live jobs (deployment, external, and CNPG) passed on implementation commit
1b5a49c6. The live tests verify installation of CLIv0.0.109for runtime0.0.109-rhaiv.0. The image builds and SDK drift check also passed.Checks run · Tests run
Follow-up review decisions (
9847c9f3, comments only):provisioning_conditionsin feat: gateway provisioning progress model and stepper UI #269. Do not addreserved 23;: a reserved number cannot be used for that field. See the Protocol Buffers field-number rules. The allocation is now documented besidegateway_version.Protocol generation produced no generated-code changes. Repository checks passed. CI has restarted for the comments-only commit.