Skip to content

[HYPERSHELL-240] docs: Ephemeral pull-request environments - #252

Open
squizzi wants to merge 1 commit into
mainfrom
squizzi/ephemeral-openshift-e2e
Open

[HYPERSHELL-240] docs: Ephemeral pull-request environments#252
squizzi wants to merge 1 commit into
mainfrom
squizzi/ephemeral-openshift-e2e

Conversation

@squizzi

@squizzi squizzi commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

What

Added specification for ephemeral pull-request environments that deploy HyperShell to OpenShift for every PR, keep it live across the PR lifecycle, and release on merge/close.

Highlights

  • New ephemeral-pr-environments.spec.md defining the complete pull-request environment contract:
    • Per-PR namespace naming (hypershell-ci-pr-<number>) with ownership labels and environment identifiers
    • Continuous-deployment lifecycle (open/synchronize/reopen/close events)
    • Image gating and swap by digest using Konflux builds
    • Fixed 3-day timebox with out-of-band reaper for abandoned PRs
    • Per-commit PR comments with stable HTML marker for updates
    • GitHub-brokered Keycloak authentication (organization gate + allowlist, no Red Hat SSO)
    • Automated e2e using client-credentials and token-exchange impersonation
    • Deprecation path for legacy components/pr-test/e2e-openshell.sh
  • Updated e2e-testing.spec.md to support grant-agnostic OIDC token acquisition via E2E_OIDC_GRANT flag
  • Clarified scope boundaries: pull-request CI, timebox, GitHub auth, and deprecation window now owned by ephemeral-pr-environments.spec

Scope

Specification and documentation updates only; no code changes.

Signed-off-by: Kyle Squizzato <kysquizz@redhat.com>
@coderabbitai

coderabbitai Bot commented Sep 4, 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: Team

Run ID: 6e294130-3e1f-4b05-a170-1facf31d73e5

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

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

@jsell-rh

jsell-rh commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Amber review: changes requested

Amber review

Status: Complete

View the submitted review.

@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

This is a well-structured, spec-only change that cleanly extracts pull-request CI, timebox, GitHub-brokered Keycloak, and the e2e-openshell.sh deprecation window into a new ephemeral-pr-environments.spec.md and re-points openshift-development.spec.md / e2e-testing.spec.md at it. However, the re-scoping edit also deletes a substantive requirement that is not pull-request CI (the overlay drift check) and leaves a dangling cross-reference, so I am requesting changes to confirm those removals are intentional and re-homed.

Amber Assessment

The prose is careful and internally consistent within the new spec: reserved terms, ownership boundaries, scenarios, and a design-decision table are all present, security handling for credentials is explicit (no secrets in comments/logs/artifacts, secure-channel handoff, digest pinning over mutable tags), and no em dashes were introduced. The concerns below are about content lost from openshift-development.spec.md during the re-scoping, not about the new spec's design.

Major

1. The "Blessed OpenShift Overlay" requirement (and the overlay drift check) is deleted with no re-home - Spec Consistency / Removed Guarantee.
openshift-development.spec.md previously defined a ### Requirement: Blessed OpenShift Overlay with scenarios "Base domain comes from configuration" and "Drift check fails on unintended drift" (a CI gate that fails a PR when deploy/openshift/ drifts from deploy/base/ outside its declared allowlist). This PR removes that requirement entirely, along with the ## Deploy Directory Structure documentation and ### Known Limitations in deploy/openshift/. These are overlay concerns, not pull-request-CI concerns - and the PR's stated scope is only "pull-request CI, timebox, GitHub auth, and deprecation window now owned by ephemeral-pr-environments.spec." The new ephemeral-pr-environments.spec.md explicitly does not redefine the overlay ("This spec does not redefine ... the deploy/openshift/ overlay"), so the drift-check guarantee now lives nowhere. A grep across specs/ finds no other home for the drift check. Removing a CI drift gate silently is a real regression in the contract. Please either keep these sections in openshift-development.spec.md or state where they move. Confidence: High.

2. Dangling internal reference to the deleted requirement - Spec Consistency.
openshift-development.spec.md line ~186 still reads "through the namespace parameterization that the Blessed OpenShift Overlay requirement defines," but that requirement was deleted in this PR. e2e-testing.spec.md (e.g. its **Related:** header and Scope) also still references the "blessed deploy/openshift/ overlay." These references now point at content that no longer exists. Fix the cross-references (or restore the requirement per finding 1). Confidence: High.

Minor

3. Function-count wording may go stale - Spec Completeness.
e2e-testing.spec.md retains the sentence "The OpenShift driver implements the same ten functions ..." while this PR (and related in-flight driver work) continues to grow the driver interface table. If the interface grows, this literal count should be updated to avoid an inaccurate contract statement. Confidence: Medium.

Cross-PR coordination

Another open pull request revises the same e2e driver interface contract in e2e-testing.spec.md and reworks the OpenShift driver's token-acquisition path: it adds new driver functions (console discovery and namespace-GC timing helpers) to the interface table and required-functions set, and it unifies acquire_oidc_token / acquire_gateway_token_with_role onto a single resource-owner password-grant path in the shared suite. This PR edits the same interface table and narrative and, in the opposite direction, requires those two functions to become grant-agnostic (selected by a new E2E_OIDC_GRANT, with a client_credentials + token-exchange path for GitHub-brokered PR environments) while also keeping the "same ten functions" wording. These are competing edits to one contract with an ordering dependency: whichever lands second must reconcile the function count/table and decide how the grant-agnostic switch coexists with the unified password-grant implementation. The owner of that other PR and the maintainers should agree on merge order and the reconciled driver-function contract before both merge - PR #244.

Findings Summary (ordered by severity, highest first):

  1. [Major] "Blessed OpenShift Overlay" requirement + overlay drift check deleted with no re-home - Spec Consistency / Removed Guarantee (openshift-development.spec.md)
  2. [Major] Dangling reference to the deleted "Blessed OpenShift Overlay" requirement - Spec Consistency (openshift-development.spec.md L186; e2e-testing.spec.md header/Scope)
  3. [Minor] "same ten functions" wording risks going stale as the driver interface grows - Spec Completeness (e2e-testing.spec.md)

Convention Checklist:

Convention Result
No em dashes (use -) Pass
Conventional commit message Pass
Spec cross-references resolve Fail
No silent removal of a stated guarantee Fail
Ownership boundaries clearly stated Pass
Secrets absent from comments/logs/artifacts (as specified) Pass

- AND the gate fails when the OpenShift job fails
points to a removed path

### Requirement: Pull-Request CI Uses This Lifecycle

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.

This re-scoping edit deleted more than pull-request CI: the previous ### Requirement: Blessed OpenShift Overlay (with the "Drift check fails on unintended drift" CI-gate scenario and "Base domain comes from configuration"), the ## Deploy Directory Structure section, and ### Known Limitations in deploy/openshift/ are all gone. Those are overlay concerns, not pull-request CI, and ephemeral-pr-environments.spec.md explicitly does not redefine the overlay - so the overlay drift check now has no home in specs/. Please keep these in this spec or state where they move.

A side effect: line ~186 of this file still says "through the namespace parameterization that the Blessed OpenShift Overlay requirement defines," which is now a dangling reference. e2e-testing.spec.md also still references the "blessed deploy/openshift/ overlay." Restore the requirement or fix these cross-references.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants