Repository navigation
test(e2e): add suite coverage for areas 12-14 and console gaps - #417
Conversation
Close the e2e coverage gaps #413 opened by renumbering the core suite to 14 areas. The underlying API/control-plane/UI features already existed; only the e2e scenarios were missing. e2e-openshell.sh (new long-only areas 12-14): - Area 12: ManagedCluster self-registration (idempotency, missing-role 403, name-collision 409, unregistered-cluster 400), the gRPC watch identity boundary (UNAUTHENTICATED / PERMISSION_DENIED / INVALID_ARGUMENT via grpcurl), and reconnect convergence. - Area 13: gateway release promotion (observed_release_id advances to the serving release; a failed rollout never moves off the last-good release) and reconciled release/network status assertions. - Area 14: admin-only /v1/users boundary and unknown-phase rejection. e2e-console.sh: cluster typeahead + CLI install-docs link (area 7) and fleet-dashboard promotion (area 11, skip-gated on E2E_FLEET_DASHBOARD_URL). Support: shared helpers in lib.sh (e2e_json_field moved here from browser-lib.sh, e2e_poll_resource_status, status predicates, registrar / multi-cluster / qualification env defaults) and acquire_registrar_token in drivers/kind.sh. Fixed a set -e abort where a bare VAR=$(e2e_wait_gateway_running|e2e_poll_resource_status) assignment killed the whole run on timeout instead of recording a failure. Verified on a local Kind cluster (long run 90 passed; short 20/0; console unit test 56/0; all shell unit tests pass). Two behaviours diverge from the spec and are recorded in RECONCILE.md rather than forced: - Gateway DELETE performs service-account cleanup through the control plane and returns 503 while it is down, so "delete while disconnected" is infeasible; area 12 covers create-convergence instead. - A failed rollout keeps the last-good workload serving and stays Running (not Degraded); area 13 asserts the never-moves-to-bad safety invariant. Multi-cluster (E2E_MULTICLUSTER) and extended qualification stay deferred behind their flags, pending infrastructure. Assisted-by: Claude Opus 4.8
|
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 |
HyperShell environment destroyedThis ephemeral OpenShift environment has been destroyed. Comment |
Amber reviewStatus: Complete |
![hypershell-delivery[bot]](https://avatars.githubusercontent.com/u/327175833?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
This is a well-scoped, test-only expansion of the shell e2e suite (new API areas 12-14, console areas 7 and 11, shared helpers). The new code is correct under set -euo pipefail (the helpers that legitimately return non-zero are guarded with || true), passes bash -n, keeps secrets out of logged commands, and records its two divergences honestly; I have only minor cosmetic nits, but there is a material cross-PR contract conflict the maintainers should resolve before both land.
Findings
No Blocker/Critical/Major issues in this PR's own code. Two Minor documentation/cosmetic nits are filed inline.
Cross-PR coordination
A material contract conflict exists with #412 (gateway provisioning by placement intent). #412 removes cluster_id from GatewayCreateRequest and replaces it with a required placement intent that the server resolves into a cluster (see its model_gateway_create_request.go and handler.go/cluster_reference.go placement resolver). This PR's area 12e asserts the opposite, current contract: a gateway create with an empty or unregistered cluster_id returns 400 naming cluster_id, and it posts create bodies that carry cluster_id directly. Under #412 those requests would fail for a different reason (missing placement), and the 400 would name placement.network/placement.provider, not cluster_id; the shared e2e_gateway_create_body helper (used here by the reconnect and promotion areas) also still emits cluster_id. The console area 7 "Select a cluster" typeahead assertion rests on the same cluster-selection UX that #412 reworks toward placement availability.
The maintainers need to decide a merge order and ownership: if #412 lands first, area 12e, the e2e_gateway_create_body bodies, and the console cluster-typeahead assertion in this PR must be rewritten to the placement-intent contract; if this PR lands first, #412 must update these e2e assertions as part of its change. This requires a joint decision rather than an independent merge.
Previous concerns
No prior Amber findings exist in the review history for this PR, so there is nothing to reconcile.
Findings Summary (ordered by severity, highest first)
- [Minor]
e2e-console.shsection-divider comments still say "8" and "9" after the areas were renumbered to 9 and 10 - Readability (L873, L943) - [Minor] Area 13c leading comment asserts the gateway "must go Degraded," contradicting the implemented (and documented D-E2E-DEGRADED) accept-either-phase behavior - Doc Consistency (L2682-L2683)
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
N/A (shell test code) |
| No secrets in logs / echoed commands | Pass |
Safe under set -euo pipefail (non-zero-returning helpers guarded) |
Pass |
bash -n clean |
Pass |
| Test Diff Scrutiny (no silently flipped pre-existing assertions) | Pass |
| Opt-in/infra-gated paths record skips, not false passes | Pass |
| @@ -793,7 +873,7 @@ sep | |||
| # ── 8. Delete gateway ──────────────────────────────────────────────────── | |||
There was a problem hiding this comment.
Minor: this section-divider comment still reads 8. Delete gateway, but the area was renumbered - the e2e_area call just below is "9. Gateway Deletion...". The next divider at L943 has the same drift (9. vs e2e_area "10. ..."). Cosmetic only; worth syncing the comment numbers so the dividers match the printed area numbers.
| fi | ||
|
|
||
| # Failed rollout: repoint to a bad (unpullable) release. The gateway must go | ||
| # Degraded and keep the last-good observed_release_id (B), never report the |
There was a problem hiding this comment.
Minor: this leading comment states the gateway "must go Degraded and keep the last-good observed_release_id," but the implemented assertion (and the fuller comment a few lines down, plus the recorded D-E2E-DEGRADED divergence) accepts Degraded OR Running-on-last-good. Suggest softening this top comment to "must never report the bad release as serving" so it matches the actual check and avoids implying a Degraded-only requirement.
2 participants
What
Add comprehensive e2e test coverage for three new core test areas (ManagedCluster registration, gateway release promotion, admin API validation) and two console browser test areas (cluster typeahead + CLI install docs, fleet dashboard promotion map).
Highlights
e2e_json_field(moved from browser-lib.sh),e2e_http_status,e2e_poll_resource_status, status predicates; environment variables for registrar client, multi-cluster fleet opt-in, fleet dashboard URL, platform qualification flags;e2e_truthygate helperacquire_registrar_tokenfor control-plane client-credentials identityScope
E2E test suite expansion only; no API server, control plane, or UI feature changes (all underlying features already implemented). Browser test areas 7/11 are code-complete + unit-tested; CI covers the browser paths. Multi-cluster (E2E-15) and qualification (E2E-20) remain deferred.