[HYPERSHELL-327] feat(ci): add unit test workflow gating between lint and e2e - #266
Conversation
Unit tests previously only ran locally (make test-all), so regressions in Go, frontend, or shell test suites could reach main undetected by CI. Add .github/workflows/unit-tests.yml, structured like lint.yml, with per-component detection and dedicated jobs for the API server, control plane, CLI/SDK generators, frontend packages, and auto-discovered *_test.sh shell tests. The workflow waits for the Lint CI gate to pass, and e2e.yml now waits for the new Unit Tests CI gate before creating the Kind cluster, so a broken unit test blocks e2e instead of wasting cluster time. Replace the old test-all Makefile target with ci-test (shell tests only, auto-discovered) and unit-test-all (Go, frontend, and shell), and split the web-console lint job's combined check:web script into static-only (check:web:lint) and test-only (test:web) variants so CI can run lint and unit tests as separate gated jobs. Assisted-by: Claude Sonnet 5
|
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 |
The unit-tests.yml CI workflow and the make unit-test-all / make ci-test Makefile targets landed without developer-facing documentation, so a developer had no single place to learn how to run the full local suite or how CI gates unit tests between lint and e2e. Add a Testing section to DEVELOPMENT.md covering make unit-test-all, make ci-test, and the per-component test commands, and extend the CI Unit Test Workflow requirement in e2e-testing.spec.md to specify the local make targets and a scenario for a full local run. Assisted-by: Claude Sonnet 5
squizzi
changed the title
The lint, unit-test, and e2e stages were three independently-triggered workflows chained with lewagon/wait-on-check-action poller jobs (wait-for-lint, wait-for-unit-tests). That kept a runner sitting idle polling a preceding stage's summary check for up to an hour before its work could start. Introduce a single .github/workflows/ci.yml orchestrator that owns the pull_request/push/merge_group/workflow_dispatch triggers, the concurrency group, and the stage ordering. It calls lint.yml, unit-tests.yml, and e2e.yml as reusable workflows (on: workflow_call) chained with native `needs:` edges (lint -> unit-tests -> e2e). Ordering is now a dependency edge, so a later stage never starts until the earlier one succeeds and nothing polls: a failing lint short-circuits unit-tests and e2e, and a failing unit-test short-circuits e2e without ever creating Kind. The stage workflows drop their own event triggers and concurrency blocks (now owned by the parent) so they never run as standalone duplicates. The two poller jobs are removed; the summary "gate" jobs are renamed to `CI gate` so the reusable-workflow prefix reads as `Lint / CI gate`, `Unit Tests / CI gate`, and `E2E / CI gate` rather than double-naming. The three Konflux image-build waits in e2e stay: they gate on an external build system that cannot be ordered with `needs:`. Register ci.yml in component-paths.json and update the e2e spec, maintain-ci skill, and DEVELOPMENT.md to describe the orchestrated pipeline. Branch protection required checks must be re-pointed to the new `<Stage> / CI gate` names. Assisted-by: Claude Opus 4.8
Restructure the CI orchestrator so change detection runs a single time and the expensive stage is gated behind the cheap ones without serializing them. - Detect changed components once in ci.yml (the detect-changes job) and pass the per-component flags into each stage as workflow_call inputs; lint, unit-tests, and e2e no longer detect changes internally. - Change the stage topology to fan-out then join: lint and unit each depend only on detect-changes and run concurrently, and e2e joins on both via needs: [detect-changes, lint, unit]. This gates only the expensive Kind run behind the two cheap stages, so a lint or unit failure surfaces as a clean red CI / Lint or CI / Unit check instead of a misleading e2e environment failure, while lint and unit never delay each other. The wall-clock cost is small because Konflux image builds run during lint/unit regardless of gating. - Drop the redundant per-stage summary/gate jobs: each reusable-workflow caller job (CI / Lint, CI / Unit, CI / E2E) is itself the aggregate check, so those three are the checks to mark required in branch protection. - Strip stage-word duplication from leaf job names (for example Go lint -> Go - API server). - Update check_ci_components.py to validate the new wiring (detector output and input pass-through in ci.yml; workflow_call input, job, and condition in lint.yml) and sync DEVELOPMENT.md, the e2e-testing spec, and the maintain-ci skill to the fan-out/join model. Assisted-by: Claude Opus 4.8
Amber reviewStatus: Failed The review stopped at 2026-09-10T18:07:47Z. A later job can retry this commit. |
Reusable-workflow stages surface their jobs as nested checks (Lint / <job>, Unit / <job>, E2E / <job>), so there is no single check named Lint, Unit, or E2E to require in branch protection - and requiring individual sub-jobs is fragile because a path-filtered skip leaves a required check pending forever. Add three rollup gate jobs to ci.yml - Lint CI Gate, Unit Tests CI Gate, and E2E CI Gate - that run with if: always(), read their stage's rolled-up result via needs, and fail unless detect-changes succeeded and the stage did not fail or cancel. A fully skipped stage passes its gate, and a stage skipped because an earlier stage failed is blocked by that earlier gate rather than double-reported. Because the gates always run, they are stable required checks that path-filtered skips never leave pending. Update the stage workflow header comments and the docs (e2e-testing spec, DEVELOPMENT.md, maintain-ci skill) to name the three gates as the required checks instead of the nested caller-job checks. Assisted-by: Claude Opus 4.8
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 is a well-documented, coherent restructuring of the CI pipeline into a single ci.yml orchestrator that runs change detection once, fans out lint and unit concurrently, and joins e2e on both. The code and spec changes are internally consistent and no HyperShell production conventions are at risk; my findings are minor CI-scoping/verification items, plus a cross-PR coordination point that needs a maintainer decision.
Scope of review
This change touches only CI workflows, tooling scripts, Makefile, docs, and a spec. There is no Go/production runtime code, no reconcilers, no pod specs, and no secret handling, so most HyperShell runtime conventions (panic, error wrapping, IsNotFound, SecurityContext, secret redaction) are not applicable here. I reviewed workflow correctness, shell/script quality, the check_ci_components.py wiring validator, and spec/doc consistency. No pre-existing test assertions were flipped, so Test Diff Scrutiny raised nothing.
Strengths
- Change detection is genuinely run once and passed through as inputs; the stage workflows correctly become
on: workflow_callonly, avoiding the double-run trap. This is also codified in themaintain-ciSKILL and the e2e spec, so the convention and its enforcement stay aligned. - The three
if: always()rollup gate jobs give branch protection a stable, always-present required check per stage, and the reasoning (skipped stage passes its gate; earlier gate failure blocks later stages) is sound and documented. check_ci_components.pywas updated to validate the new split wiring (detector output + pass-through inci.yml, input + job + condition in the stage workflow), somake checkstill enforces end-to-end registration.- The
.forbidden-terms-whitelist.jsonline reference for the literal em dash inCLAUDE.mdwas correctly bumped 127 -> 130 to match the three added command lines. Good attention to detail.
Findings
[Minor] Editing unit-tests.yml triggers a full Kind e2e run. In .github/component-paths.json, the e2e component's shared-path list now includes .github/workflows/unit-tests.yml (line 94). A change to the unit-test workflow does not affect e2e behavior, but it will mark e2e as changed and provision Kind + run the full matrix. Consider dropping unit-tests.yml from the e2e component's paths (keeping ci.yml, which does orchestrate e2e). Confidence: High.
[Minor] test-frontend runs every frontend suite regardless of which UI package changed. .github/workflows/unit-tests.yml runs pnpm run test:web (all of sdk/domain-probes/gateway-management-ui/operational-dashboard-ui/web-console/web-console-bff test:run) whenever any one of web_console, gateway_management_ui, or operational_dashboard_ui changed. That is safe (over-inclusive, not under-inclusive) but partially defeats the per-component gating goal for the UI packages. Acceptable as-is; flagging so it is a conscious choice. Confidence: High.
[Minor/Verify] Confirm a fully-skipped lint or unit stage does not skip e2e. In .github/workflows/ci.yml the e2e job declares needs: [detect-changes, lint, unit] with no if:. If a PR changes only e2e-relevant files, all inner lint/unit jobs are path-filtered out. Please confirm on a real run that a reusable-workflow caller whose inner jobs all skip reports success (so e2e still starts) rather than skipped (which would cascade and skip e2e). The e2e-gate treats a skipped stage as pass, which is correct for the gate, but the important behavior is that e2e itself still runs when only e2e files change. Confidence: Medium.
Cross-PR coordination
There is a competing CI-structure change that needs a maintainer decision before both land. PR #267 (ephemeral OpenShift e2e) introduces a new top-level, independently pull_request-triggered workflow (pr-environment.yml) with its own change-detection/plan-images logic and its own concurrency group. This PR establishes the opposite convention - all pipeline event triggers live only in ci.yml, stage workflows are workflow_call-only, and change detection runs exactly once - and codifies that rule in the maintain-ci SKILL. Maintainers should decide whether the ephemeral OpenShift environment is an intentionally separate pipeline (allowed to keep its own trigger) or should be folded into the new ci.yml orchestration model, so the documented convention stays true. Relatedly, both PRs edit the same e2e component block of .github/component-paths.json in structurally incompatible ways: this PR rewrites the shared-workflow lists to add ci.yml/unit-tests.yml, while PR #267 adds pr-environment.yml plus new path globs (deploy/openshift/**, deploy/e2e/**, scripts/ci/**) to that same block - these must be reconciled by hand and a merge order agreed.
A second, lower-stakes coordination point with the same PR #267: this PR adds a repo-wide *_test.sh auto-discovery runner (scripts/run-shell-unit-tests.sh, make ci-test) and a Shell unit-test CI job, while PR #267 adds new *_test.sh files (scripts/ci/pr-env-lib_test.sh, scripts/ci/reap-pr-environments_test.sh, tests/e2e/openshift_driver_test.sh). Whoever merges second inherits the other side. Maintainers should confirm merge order so PR #267's shell unit tests get a CI home via this PR's runner instead of being orphaned or wired to a competing runner.
Findings Summary (ordered by severity, highest first)
- [Minor]
unit-tests.ymladded to thee2ecomponent's paths triggers full Kind runs on unit-workflow edits - CI Scoping (.github/component-paths.jsonL94) - [Minor]
test-frontendruns all frontend suites regardless of which UI package changed - CI Scoping (.github/workflows/unit-tests.ymlL66) - [Minor/Verify] Confirm skipped lint/unit stage does not cascade-skip
e2e- CI Correctness (.github/workflows/ci.ymlL112-L117)
Convention Checklist
| Convention | Result |
|---|---|
| No em dashes (hyphens only) | Pass |
CI component registration wiring enforced (make check) |
Pass |
| Actions pinned to commit SHAs | Pass |
| Conventional commit message | Pass |
| Spec/docs kept consistent with workflow changes | Pass |
| Test Diff Scrutiny (no flipped pre-existing assertions) | Pass |
| ".github/scripts/detect-components.sh", | ||
| ".github/workflows/e2e.yml" | ||
| ".github/workflows/e2e.yml", | ||
| ".github/workflows/unit-tests.yml", |
There was a problem hiding this comment.
[Minor] Editing unit-tests.yml triggers a full Kind e2e run. Adding .github/workflows/unit-tests.yml to the e2e component's shared paths means a change to the unit-test workflow marks e2e as changed and provisions Kind + runs the full e2e matrix, even though unit-tests.yml does not affect e2e behavior. Consider dropping it here and keeping only ci.yml (which does orchestrate e2e). Confidence: High.
| - name: Install dependencies | ||
| run: pnpm install --frozen-lockfile | ||
| - name: Run frontend unit tests | ||
| run: pnpm run test:web |
There was a problem hiding this comment.
[Minor] test-frontend runs every frontend suite regardless of which UI package changed. pnpm run test:web runs all frontend package test:run suites (sdk, domain-probes, gateway-management-ui, operational-dashboard-ui, web-console, web-console-bff) whenever any one of web_console/gateway_management_ui/operational_dashboard_ui changed. Safe (over-inclusive), but it partially defeats per-component gating for the UI packages. Flagging so it is a conscious choice. Confidence: High.
| e2e: | ||
| name: E2E | ||
| needs: | ||
| - detect-changes | ||
| - lint | ||
| - unit |
There was a problem hiding this comment.
[Minor/Verify] Confirm a fully-skipped lint or unit stage does not cascade-skip e2e. e2e has needs: [detect-changes, lint, unit] with no if:. For a PR that changes only e2e-relevant files, all inner lint/unit jobs are path-filtered out. Please confirm on a real run that a reusable-workflow caller whose inner jobs all skip reports success (so e2e still starts), not skipped (which would cascade and skip e2e). The e2e-gate correctly treats a skipped stage as pass, but the key behavior is that e2e itself still runs when only e2e files change. Confidence: Medium.
repository-policy.yml and sdk-drift-check.yml were independently triggered workflows that each re-ran component-change detection on their own. Fold both in as jobs inside the renamed checks stage (formerly lint.yml) so they share the orchestrator's single detect-changes pass instead: repository-policy (`make check`) runs unconditionally, sdk-drift gates on the sdk_go/sdk_typescript detection outputs. Rename the orchestrator ci.yml -> tests.yml (name: Tests) and the lint stage lint.yml -> checks.yml (name: Checks), reflecting that checks now covers more than just linting. Consolidate the three rollup gate jobs into two required checks: `Checks CI Gate` covers the checks stage alone, `Tests CI Gate` covers unit and e2e together since both are "running the code" stages rather than static checks. Also add components/api-server/Dockerfile.openapi to the sdk_typescript component's detection paths, matching the old sdk-drift-check.yml path filter that component-paths.json was missing. Assisted-by: Claude Sonnet 5
The prior commit renamed the workflow files (ci.yml -> tests.yml, lint.yml -> checks.yml) and deleted the folded-in standalone workflows, but the actual content changes were left unstaged. This commit adds them: - tests.yml: checks/unit fan out from detect-changes, e2e joins on both; two rollup gate jobs (`Checks CI Gate`, `Tests CI Gate`) replace the old three. - checks.yml: adds the repository-policy (`make check`, unconditional) and sdk-drift (gated on sdk_go/sdk_typescript) jobs, plus the sdk_go workflow_call input. - unit-tests.yml / e2e.yml: header comments and `needs:` updated to reference the checks stage and the new gate names. - component-paths.json: workflow path references updated to the renamed files, and Dockerfile.openapi added to sdk_typescript's detection paths. - check_ci_components.py: validates against tests.yml/checks.yml. - DEVELOPMENT.md, specs/platform/e2e-testing.spec.md, skills/tooling/maintain-ci/SKILL.md, skills/RECONCILE.md: synced to the renamed files, the checks stage's added jobs, and the two-gate branch-protection model. Assisted-by: Claude Sonnet 5
Amber reviewStatus: Complete |
amber-review-bot
left a comment
amber-review-bot
left a comment
There was a problem hiding this comment.
Verdict
COMMENT. This is a clean, well-documented CI restructuring: it consolidates lint/policy/SDK-drift into a reusable checks stage, adds a reusable unit stage with auto-discovered shell tests, converts e2e to a reusable stage, and wires them under a single tests.yml orchestrator with native needs: gating and two stable rollup gates. I verified the change detection, gate logic, component-registration check (make check passes), and the new shell-test runner (make ci-test discovers and passes all 4 *_test.sh files) all behave as documented; findings below are non-blocking.
What I verified
- Gate logic is sound.
Checks CI Gatecoverschecks;Tests CI Gatecoversunit+e2e. A failingchecksskipse2e(vianeeds) but still failsChecks CI Gate; a failingdetect-changesfails both gates; path-filtered skips pass their gate. Every failure path stays blocking. - Change detection runs once in
tests.ymland flows into each stage aswith:inputs;e2e.ymlno longer referencessteps.detect.*and correctly readsinputs.*.detect-components.shstill handlespull_request/push/merge_group/workflow_dispatch, so moving detection out ofe2e.yml(which previously only detected onpull_request) is safe. - Security posture is good. Triggers use
pull_request(neverpull_request_target);permissionsare minimal (contents: read, pluschecks: readonly where the Konflux wait needs it); the caller ceiling covers the reusable stages;github.token(notsecrets.*) is used, so nosecrets: inheritis required; the upstream framework checkout usespersist-credentials: false; all actions are SHA-pinned. - Registration check passes.
scripts/check_ci_components.pywas updated to validate the newtests.yml/checks.ymlwiring and reports all 9 components registered.
Findings
[Major] Branch-protection required checks must be migrated in lockstep with merge. This PR renames lint.yml -> checks.yml, deletes repository-policy.yml and sdk-drift-check.yml, and removes e2e.yml's own triggers/summary. That retires the old status-check names (Lint CI gate, E2E CI gate, Repository policy, OpenAPI SDK drift) and introduces Checks CI Gate / Tests CI Gate. If branch protection is not updated at merge time, PRs (including the merge queue) will block waiting on checks that never report again. The PR body and DEVELOPMENT.md do call this out ("Mark those two gates as required"), so this is a coordination/admin action to sequence with merge, not a code defect. Confidence: High.
[Minor] make unit-test-all diverges from CI by omitting -count=1. The spec addition states make unit-test-all "runs the same unit test suites as the CI jobs," but the CI jobs run Go tests with -count=1 (cache-busting) while Makefile:312-315 run plain go test ./.... Go's test cache can make a local run pass on stale results where CI (with -count=1) fails. Consider go test -count=1 ./... for the control-plane/CLI/generator targets to keep local parity honest. Confidence: Medium.
Cross-PR coordination
A design and ordering decision is needed with the ephemeral OpenShift e2e work (PR #267). This PR codifies, as SHALL requirements in specs/platform/e2e-testing.spec.md and in the maintain-ci skill, a single-orchestrator CI model: stage workflows are on: workflow_call only, change detection runs exactly once in tests.yml, and new standalone, independently-triggered CI workflows should not be added because they re-duplicate change detection. PR #267 adds .github/workflows/pr-environment.yml, a new standalone pull_request-triggered workflow that provisions an environment and runs its own OpenShift e2e suite outside that orchestrator. Maintainers should decide whether that OpenShift e2e belongs as a stage under the new tests.yml model (consistent with the convention this PR establishes) or is an intentional exception, and should sequence the merges: both PRs edit the e2e entry in .github/component-paths.json, and this PR rewrites scripts/check_ci_components.py and maintain-ci to expect the checks.yml/tests.yml reusable-stage wiring, so #267 will need to adopt that wiring if it lands afterward. (Note: #267's new scripts/ci/*_test.sh and tests/e2e/openshift_driver_test.sh will be auto-run by this PR's new test-bash job, so they should conform to this PR's self-contained shell-test contract.)
Findings Summary (ordered by severity, highest first)
- [Major] Branch-protection required checks must switch to
Checks CI Gate/Tests CI Gatein lockstep with merge or PRs will block on retired checks - CI / Operational (.github/workflows/tests.ymlL141, L164) - [Minor]
make unit-test-allomits-count=1, diverging from CI and Go's test cache can mask failures - Test Parity (MakefileL312-315)
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
N/A (no Go production code changed) |
Register every component in CI (make check) |
Pass |
| Actions pinned to full commit SHA | Pass |
| Minimal workflow permissions | Pass |
No secrets in logs / no pull_request_target |
Pass |
| Reusable-workflow secret/permission ceiling correct | Pass |
| Conventional commit messages | Pass |
| No em dashes in text files | Pass |
| Spec/docs updated for behavior change | Pass |
| Test Diff Scrutiny (flipped assertions) | N/A (no existing assertions modified) |
| $(PNPM) --filter @openshift-online/hypershell-gateway-management-ui test:run | ||
| $(PNPM) --filter @openshift-online/hypershell-web-console test:run | ||
| $(PNPM) --filter @openshift-online/hypershell-web-console-bff test:run | ||
| cd components/control-plane && go test ./... |
There was a problem hiding this comment.
[Minor] Local/CI parity: the new spec text says make unit-test-all runs "the same unit test suites as the CI jobs," but CI runs Go tests with -count=1 while these targets run plain go test ./.... Go's test cache can let a local run pass on stale results where CI (with -count=1) fails. Consider go test -count=1 ./... for the control-plane/CLI/generator lines to keep parity honest.
| # check pending. A stage that was skipped (result 'skipped') passes its | ||
| # gate; only an actual 'failure'/'cancelled' -- or a detect-changes that did | ||
| # not succeed -- fails it. | ||
| checks-gate: |
There was a problem hiding this comment.
[Major] Branch-protection migration must be sequenced with merge. These Checks CI Gate / Tests CI Gate jobs replace the previously-required Lint CI gate, E2E CI gate, Repository policy, and OpenAPI SDK drift checks (the latter two workflows are deleted, and e2e.yml loses its own triggers). If branch protection is not updated to require these two gates at merge time, PRs and the merge queue will block waiting on checks that no longer report. This is documented in the PR body/DEVELOPMENT.md, so it is a coordination item, not a code defect - just flagging so it is not missed on merge.
Checks was a stage called from tests.yml via workflow_call, so GitHub nested it as one "Tests" run containing a "Checks / <job>" job group instead of two separate entries in the PR checks list. Split them into two top-level workflows so Checks and Tests each run as their own concurrent entry, as intended. GitHub Actions `needs:` only orders jobs within a single workflow file, so this trades away e2e's gate on Checks: e2e (inside tests.yml) now only waits on Unit (`needs: [detect-changes, unit]`), not on Checks. Reintroducing that cross-workflow gate would require a wait-on-check-action poller, the pattern this branch removed earlier. checks.yml also gains its own detect-changes job since it can no longer receive detection as workflow_call inputs from tests.yml. checks.yml's lint jobs now gate on `needs.detect-changes.outputs.*` directly instead of `inputs.*`, and it has its own `checks-gate` job (Checks CI Gate) covering every job in the workflow. tests.yml keeps its `tests-gate` job (Tests CI Gate) covering unit and e2e. Branch protection required checks stay Checks CI Gate and Tests CI Gate, now each from its own top-level workflow run. Updated check_ci_components.py, DEVELOPMENT.md, specs/platform/e2e-testing.spec.md, and maintain-ci/SKILL.md to match. Assisted-by: Claude Sonnet 5
Amber reviewStatus: Complete |
amber-review-bot
left a comment
amber-review-bot
left a comment
There was a problem hiding this comment.
Verdict
This is a well-structured, carefully documented CI refactor that splits the pipeline into two concurrent top-level workflows (Checks and Tests), makes unit-tests.yml/e2e.yml reusable stages gated by native needs:, and adds auto-discovered shell unit tests. The design and its trade-offs are sound; my one substantive concern is that the refactor quietly removes a check_ci_components.py guardrail that used to guarantee every component's lint job is wired into the rollup gate.
Summary
The pipeline reorganization is coherent: Checks CI Gate and Tests CI Gate are the two if: always() rollup jobs meant for branch protection, change detection is deduplicated per workflow, e2e is correctly gated behind unit via needs: [detect-changes, unit], and the spec/docs (e2e-testing.spec.md, DEVELOPMENT.md, CLAUDE.md, maintain-ci) were updated in step with the workflow changes. No stale references to lint.yml, repository-policy.yml, or sdk-drift-check.yml remain. The reusable-workflow event inheritance for merge_group/push is preserved.
The gate jobs only observe jobs that appear in their static needs: list. The registration validator previously enforced that each component's lint job was listed in the summary gate's needs: ("summary dependency"); this PR drops that assertion, so a future component whose lint job is added but omitted from checks-gate.needs would fail silently while Checks CI Gate still reports green. See the inline note.
Findings
[Major] scripts/check_ci_components.py no longer verifies that each component lint_job is a dependency of the rollup gate. With checks-gate/tests-gate reading only needs.*.result, a lint job that is registered but never added to checks-gate.needs will not be able to fail the required check. Restore an assertion that each lint_job appears in the checks-gate needs: list (and, for stages, that unit-test jobs feed tests-gate), and add the corresponding "add the job to the gate's needs" step back to the maintain-ci guidance. CI safety net (Confidence: Medium)
[Minor] The two top-level workflows each run their own detect-changes job (duplicated checkout + script run per PR). This is a deliberate, documented trade-off of the split, not a defect - noting only so maintainers weigh the extra runner minutes against the concurrency benefit. Observation (Confidence: High)
Cross-PR coordination
Two items require maintainer coordination with the ephemeral OpenShift PR-environment work (#267):
-
Merge order for shell-test execution. That PR adds shell tests (
scripts/ci/pr-env-lib_test.sh,scripts/ci/reap-pr-environments_test.sh,tests/e2e/openshift_driver_test.sh) but ships no runner or CI wiring for them; it relies on this PR's*_test.shauto-discovery (make ci-test/run-shell-unit-tests.sh/ thebash_testsdetection +Shelljob). Those tests are only exercised in CI once this PR merges. Maintainers should land this PR first (or rebase #267 on it) so #267's shell tests are actually gated rather than dead. -
Branch-protection / required-checks model and the shared
e2ecomponent entry. This PR convertse2e.ymlinto a reusable stage and defines exactly two required gates (Checks CI Gate,Tests CI Gate), while #267 introduces a new independently-triggeredpr-environment.ymlonpull_requestthat runs the OpenShift e2e suite outside this two-workflow gating model. Both PRs also edit the samee2eentry in.github/component-paths.jsonin different ways. Maintainers need to decide howpr-environment.ymlfits the new required-checks set (gating vs. informational) and reconcile the overlappingcomponent-paths.jsonchange so the two CI redesigns compose rather than diverge.
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
N/A |
| Conventional commit message | Pass |
| Image references consistent across manifests | Pass |
| Component registered in CI (maintain-ci) | Pass |
| CI guardrail completeness (validator coverage) | Fail |
| Docs/specs updated with workflow changes | Pass |
| CHECKS_WORKFLOW_PATH, | ||
| rf"needs\.detect-changes\.outputs\.{re.escape(component)}\b", | ||
| ), | ||
| ) |
There was a problem hiding this comment.
This refactor drops the previous summary dependency assertion. The old validator required each component's lint job to appear in the summary gate's needs:; now only detector output, lint job, and job condition are checked.
Because checks-gate (and tests-gate) evaluate results via join(needs.*.result, ' '), a job that is not in the gate's needs: list is invisible to the gate. So a future component could register a lint job, pass this validator, and still leave Checks CI Gate green even when that lint job fails - the exact silent-required-check gap this script exists to prevent.
Suggest re-adding an assertion that each lint_job is listed under the checks-gate job's needs: in checks.yml (and, correspondingly, that unit-test jobs roll up into tests-gate).
| # or a detect-changes that did not succeed -- fails it. | ||
| checks-gate: | ||
| name: Checks CI Gate | ||
| needs: |
There was a problem hiding this comment.
This static needs: list is the sole source of truth for what Checks CI Gate can fail on - any lint job omitted here is silently excluded from the required check. Since check_ci_components.py no longer enforces that every component lint_job is present here, please keep this list and the validator in sync (see the validator comment).
Since checks.yml and tests.yml are now two independent top-level
workflows, a job's own name is not prefixed with its workflow's
name (only reusable-workflow-call jobs get a prefix, from the
calling job). Naming the gate jobs "Checks CI Gate" and "Tests CI
Gate" read redundantly in the PR checks list; the workflow run
each belongs to already disambiguates them, so both are now
named plain "CI Gate".
Also fix the same dual-naming in e2e.yml: the e2e-kind job was
named "E2E Kind (${{ matrix.database-provider }})", which read as
"E2E / E2E Kind (cnpg)" since it's called from tests.yml's "E2E"
stage. Renamed to "Kind (${{ matrix.database-provider }})".
Updated DEVELOPMENT.md, specs/platform/e2e-testing.spec.md, and
maintain-ci/SKILL.md to match; branch-protection required checks
are unaffected in substance (still checks.yml's and tests.yml's
gate jobs), only their display name changes.
Assisted-by: Claude Sonnet 5
Amber reviewStatus: Complete |
amber-review-bot
left a comment
amber-review-bot
left a comment
There was a problem hiding this comment.
Verdict
This is a well-constructed, internally consistent CI restructuring that splits the pipeline into a Checks workflow (lint, repository policy, SDK drift) and a Tests orchestrator (unit -> e2e) with a stable, always-present CI Gate in each. The change is coherent across workflows, component-paths.json, check_ci_components.py, the maintain-ci skill, the spec, and docs; findings are limited to minor coverage/robustness notes plus a cross-PR coordination item that needs a maintainer decision.
I verified: no dangling lint.yml / repository-policy.yml / sdk-drift-check.yml references remain, make check's component registration validator was updated to the renamed workflow, detect-components.sh handles all four trigger events, and no secrets/panics/em-dashes are introduced.
Cross-PR coordination
Two coordination items with the ephemeral-OpenShift-e2e work (the PR that adds .github/workflows/pr-environment.yml, scripts/ci/*, and their *_test.sh files) require a maintainer decision:
-
Change order for shell-test enforcement. This PR is the one that introduces the shell-unit-test runner and CI job:
scripts/run-shell-unit-tests.sh,make ci-test, thebash_testsauto-discovery indetect-components.sh, and thetest-bashjob inunit-tests.ymlthat executes every*_test.sh. The ephemeral-environments PR adds hermetic*_test.shunit tests (scripts/ci/pr-env-lib_test.sh,scripts/ci/reap-pr-environments_test.sh, and extendstests/e2e/openshift_driver_test.sh) but ships no runner of its own; its workflow only runs the fulle2e-openshell.sh. So those unit tests are executed by CI only once this PR merges. Maintainers should decide the merge order (this PR first) or have the other PR not assume the runner exists. -
Canonical CI model vs. a third top-level workflow. This PR codifies a two-top-level-workflow model (
checks.yml+tests.yml, e2e demoted to a reusable stage gated behindunit) and rewritesmaintain-ci/SKILL.mdande2e-testing.spec.mdas the authoritative CI structure and governance. The ephemeral-environments PR introduces a third independently-triggered top-level workflow (pr-environment.yml) and separately edits the sharede2eentry incomponent-paths.json. A design decision is needed on howpr-environment.ymlfits the model this PR establishes (in particular whether the OpenShift e2e should be gated on theunitstage the way the Kind e2e now is) and how the two edits to thee2ecomponent registration reconcile.
Findings
Minor - test selection coverage. unit-tests.yml's test-frontend job gates only on web_console, gateway_management_ui, and operational_dashboard_ui. pnpm run test:web builds hypershell-sdk and runs web-console-bff tests too, so a PR that changes only components/sdk-typescript will not run the frontend unit suite against the rebuilt SDK. Drift + lint still cover SDK changes, so impact is low, but consider adding inputs.sdk_typescript to the condition (or documenting the intentional exclusion). Confidence: High.
Minor - hermeticity assumption in the shell runner. scripts/run-shell-unit-tests.sh runs every discovered *_test.sh unconditionally via bash "${testfile}", and make unit-test-all / the test-bash CI job invoke it with no filtering. Any future *_test.sh that needs a cluster or network will break both the local target and CI with no allowlist to opt out. Worth stating the "must be hermetic" contract in the runner header and/or the maintain-ci skill. Confidence: Medium.
Findings Summary (ordered by severity, highest first):
- [Minor]
test-frontendunit job omitssdk_typescript, so SDK-only changes skip the frontend unit suite - Test Coverage (unit-tests.yml L52-55) - [Minor] Shell-test runner assumes every
*_test.shis hermetic; no opt-out for future non-hermetic tests - Robustness (run-shell-unit-tests.sh L14, L31)
Convention Checklist (only conventions evaluated for this diff):
| Convention | Result |
|---|---|
No panic() / no secrets in logs or CI output |
Pass |
| Image/action references pinned and consistent (checkout/setup pinned to SHA) | Pass |
Component registered in CI (detector output, job, gate, make check validator) |
Pass |
| Config separate from code (change detection via inputs, no hardcoded allowlists) | Pass |
| Conventional commit messages | Pass |
| No em dashes in text files | Pass |
| Spec/docs kept consistent with workflow changes | Pass |
| if: >- | ||
| inputs.web_console == 'true' || | ||
| inputs.gateway_management_ui == 'true' || | ||
| inputs.operational_dashboard_ui == 'true' |
There was a problem hiding this comment.
The test-frontend job gates only on web_console, gateway_management_ui, and operational_dashboard_ui, but pnpm run test:web builds @openshift-online/hypershell-sdk and also runs the web-console-bff suite. A PR that changes only components/sdk-typescript will skip the frontend unit tests, so a SDK change that breaks a consumer's tests would not be caught in the unit stage (only lint/drift run). Consider adding inputs.sdk_typescript to this condition, or note the exclusion is intentional. (Minor)
|
|
||
| while IFS= read -r testfile; do | ||
| echo "==> ${testfile}" | ||
| if ! bash "${testfile}"; then |
There was a problem hiding this comment.
This runner executes every discovered *_test.sh unconditionally, and make unit-test-all / the test-bash CI job call it with no filter or allowlist. That bakes in an assumption that all *_test.sh files are hermetic (no cluster/network). A future non-hermetic *_test.sh would break both the local unit-test-all target and the CI shell job with no way to opt out. Worth stating the hermetic contract in this file's header and in the maintain-ci skill so contributors know the boundary before adding shell tests under, e.g., scripts/ci/ or tests/e2e/. (Minor)
The embedded-spec contract test hardcoded a total operation count of 37, set when the Fleet entity was removed. Several endpoints (service accounts, revoke, roles, role bindings, users, and now registerManagedCluster) landed since without the constant being bumped, so the test only started failing once CI began actually gating on api-server unit tests. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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 cleanly splits CI into two independently-triggered top-level workflows -- Checks (lint, repository policy, SDK drift) and Tests (unit + e2e) -- with e2e gated behind a fast unit stage, and it correctly repairs a contract test that has been silently stale on main. The change is well-documented, internally consistent, and touches no production code paths; I have no blocking findings, only a small set of coordination and hardening notes.
Context verified
openapi_embed_test.gobumps the asserted operation count37 -> 40. The embedded spec (components/api-server/pkg/api/openapi/api/openapi.yaml) already contains 40 operations onmain, while the assertion still said 37. The test is therefore currently failing/stale onmainand this PR is a legitimate correction, not a loosened guarantee. This is the only modified pre-existing assertion in the diff; it tightens the test to match reality.- The em-dash whitelist line update (
127 -> 130in.forbidden-terms-whitelist.json) correctly tracks the em dash that moved toCLAUDE.md:130. - No stray references to the removed/renamed workflows (
lint.yml,repository-policy.yml,sdk-drift-check.yml) remain in scripts, specs, or docs beyond intentional explanatory mentions.check_ci_components.py,maintain-ci/SKILL.md,component-paths.json,CLAUDE.md,DEVELOPMENT.md, ande2e-testing.spec.mdwere all updated to the new structure.
Notable findings (detail in inline comments)
- [Major / operational] Renaming the gate jobs to plain
CI Gate(x2) and removing the oldLint CI gate/E2E CI gate/ standalone policy+drift workflows changes the set of check names GitHub reports. Branch-protection required-checks must be re-pointed at merge time or PRs will either hang on now-absent required checks or merge unprotected. - [Minor]
scripts/check_ci_components.pydrops the previous "summary dependency" assertion, so it no longer verifies a component's lint job is actually wired into the gate'sneeds. This weakens the very guardrailmaintain-cirelies on. - [Minor / design] The contract test still hardcodes an absolute operation count, which turns every endpoint-adding PR into a serialized edit of the same literal once this gate is enforced.
Cross-PR coordination
- PR #237 (
feat(release): add managed source releases) edits.github/workflows/lint.yml(adds alint-pull-request-titlejob, acheck-build-metadatastep, and newLint CI gatewiring) and edits.github/workflows/e2e.yml'splan-imageschange-detection logic, and it also modifies the very sameopenapi_embed_test.gooperation-count assertion (extending the test and changing the count). This PR renameslint.ymltochecks.yml(restructuring the gate) and convertse2e.ymlinto a reusableworkflow_callstage whose change detection moves intotests.yml. These are not mere text collisions: whichever lands second must re-target its lint-job/gate additions fromlint.ymlontochecks.yml, re-apply itsplan-imageslogic against the new reusable-workflow input model, and reconcile the operation-count literal. Maintainers should pick an explicit merge order and assign the rebase. - PR #267 (
feat(ci): ephemeral PR environments for OpenShift e2e) introduces another independently-triggered top-level workflow (pr-environment.yml) with its ownplan-images/change-detection, and edits the samee2eblock in.github/component-paths.jsonthat this PR edits. This PR establishes the opposite consolidation principle (single shareddetect-changes, e2e as a reusable stage gated on unit, deliberately no cross-workflow gating). Maintainers should decide the ordering and whether the ephemeral-environment workflow adopts this PR's Checks/Tests gating model and shared detection, so the two CI orchestration efforts converge rather than diverge.
Findings Summary (ordered by severity, highest first):
- [Major] Gate-job rename to
CI Gatex2 changes required-check names; branch protection must be re-pointed at merge - CI / Operational (.github/workflows/tests.ymlL123,.github/workflows/checks.ymlL358) - [Minor]
check_ci_components.pyno longer asserts each lint job is wired into the gate'sneeds- CI Guardrail (scripts/check_ci_components.pyL91) - [Minor] Contract test hardcodes an absolute operation count, serializing every endpoint-adding PR on one literal - Test Design (
components/api-server/pkg/api/openapi_embed_test.goL58)
Convention Checklist
| Convention | Result |
|---|---|
| Test Diff Scrutiny (modified assertion justified) | Pass |
| Image/workflow references consistent across the stack | Pass |
| Register every component in CI (maintain-ci) | Pass |
| Config separated from code | Pass |
| Conventional commit message | Pass |
| No em dashes in text files | Pass |
| CI guardrail coverage preserved | Fail |
| # gate; only an actual 'failure'/'cancelled' -- or a detect-changes that did | ||
| # not succeed -- fails it. | ||
| tests-gate: | ||
| name: CI Gate |
There was a problem hiding this comment.
[Major / operational] Both this job and checks.yml's checks-gate are now named CI Gate, and the old required checks (Lint CI gate, E2E CI gate, plus the standalone repository-policy / sdk-drift-check workflow checks) disappear. GitHub branch protection keys required checks by reported name, so on merge an admin must update the required-checks list to the two new CI Gate entries (disambiguated only by workflow run: Tests vs Checks). Until that happens, PRs will either block on now-absent required checks or merge with no required gate. Please call this out in the PR description / merge runbook, and consider whether two identically-named CI Gate checks are distinguishable enough in the branch-protection picker.
| # called from tests.yml) with its own detect-changes job. Verify the | ||
| # whole wiring lives there: detector output, lint job, and the job's | ||
| # gating condition on that same job's output. | ||
| component_checks = ( |
There was a problem hiding this comment.
[Minor] The previous validation included a summary dependency pattern that asserted each component's lint job was listed in the gate job's needs:. The new component_checks tuple only verifies the detector output, the lint job's existence, and its if: condition - it no longer confirms the job is wired into checks-gate's needs. Since the gate now rolls up join(needs.*.result, ' '), a lint job that is defined but omitted from checks-gate.needs would silently not be gated, and this script (the guardrail maintain-ci depends on) would not catch it. Consider re-adding an assertion that each lint_job appears under checks-gate.needs.
| } | ||
| if operationCount != 37 { | ||
| t.Fatalf("embedded operation count = %d, want 37", operationCount) | ||
| if operationCount != 40 { |
There was a problem hiding this comment.
Confirmed correct: the embedded spec already has 40 operations on main while this assertion said 37, so this repairs a stale/failing test rather than loosening it. Note for coordination: this remains a hardcoded absolute count. Once the new unit-test gate actually enforces it, every in-flight PR that adds an endpoint (there are several open) will have to edit this same literal, and only one can win the merge race before the others must re-bump it. Consider deriving/asserting the count from a per-tag/per-resource expectation, or at least flag the collision in the description so endpoint-adding PRs know to update it.
`Unit / Go - API server` failed on PR #266 with "connection refused" to localhost:5432 -- test-api-server ran `make test`, but rh-trex-ai's test.NewHelper connects to a real PostgreSQL rather than mocking it, and no database was ever started in that job. This has been broken since this branch's first commit added unit-tests.yml; it only surfaced now that Unit is a required gate. Add a `services:` postgres container to test-api-server matching the image, user, password, and database name from components/api-server/Makefile's db/setup target, so the job's localhost:5432 matches what test.NewHelper expects. Verified locally: running the same postgres image via `make db/setup` then `make test` gets past the connection error and into the actual test suite (87 tests passed before an unrelated macOS/arm64 cgo crash local to this machine, not the ubuntu-24.04 CI runner). Also add a checklist note to maintain-ci/SKILL.md: a unit-test job for a component with real (non-mocked) dependencies needs a matching `services:` container, since its absence fails as a connection error rather than a build error and can go unnoticed until the job's `if:` first evaluates to true. Assisted-by: Claude Sonnet 5
Amber reviewStatus: Complete |
amber-review-bot
left a comment
amber-review-bot
left a comment
There was a problem hiding this comment.
Verdict
Solid, well-documented CI restructure: it splits static checks (checks.yml) from the unit/e2e pipeline (tests.yml), converts unit-tests.yml/e2e.yml into reusable stages gated with native needs:, and adds *_test.sh auto-discovery plus matching local make targets. The design and its trade-offs are captured thoroughly in the workflow headers, the spec, and the maintain-ci skill; my main concern is a possible coverage gap where the E2E stage is skipped for changes that touch only e2e-relevant paths.
Findings
[Major] E2E stage may be skipped for e2e-only / pr_test-only changes (.github/workflows/tests.yml, e2e job)
The e2e job declares needs: [detect-changes, unit] with no if:. When a PR changes only e2e-relevant paths that map to no unit component (e.g. tests/e2e/**, scripts/kind/**, or components/pr-test/**), every job inside the unit reusable workflow is path-filtered out. A reusable-workflow caller job whose child jobs are all skipped resolves to skipped, and by default a job whose needs dependency is skipped is itself skipped. The result: the whole E2E stage would be skipped and CI Gate (which treats a skipped stage as passing) would go green, so e2e never runs for a pure e2e/kind/pr-test change - a coverage regression versus the previous on: pull_request e2e workflow, whose plan-images.should_run still fires on inputs.e2e/inputs.pr_test.
Please verify this against a real e2e-only PR before merge. If confirmed, gate the stage on the unit result rather than its completion, e.g.:
e2e:
needs: [detect-changes, unit]
if: >-
always() &&
needs.detect-changes.result == 'success' &&
needs.unit.result != 'failure' &&
needs.unit.result != 'cancelled'e2e.yml already self-gates internally via plan-images.outputs.should_run, so running the stage whenever unit did not fail is safe. Confidence: Medium (depends on GitHub's skipped-propagation semantics for reusable-workflow calls, which is why I ask you to confirm).
[Minor] make check no longer enforces gate wiring / unit-test registration (scripts/check_ci_components.py)
The rewrite drops the old summary dependency assertion and adds no equivalent for the new topology. check_ci_components.py now verifies only detector output, lint job, and job condition in checks.yml; it never confirms a component's lint job is listed in checks-gate.needs, nor that a component with unit tests has a unit-tests.yml job / tests.yml input. A component added per the updated maintain-ci steps could therefore be omitted from the gate's needs (its failures would not fail CI Gate) or from the unit-test wiring, with make check still passing. Consider re-adding a gate-membership check for checks.yml and a minimal wiring check for unit-tests.yml/tests.yml.
[Minor] Exact operation-count contract assertion is now an enforced gate (components/api-server/pkg/api/openapi_embed_test.go:58)
Bumping the magic number to 40 is correct for current main, but this PR is what first runs make test as a required check, turning this hardcoded count into a serialization point: every PR that adds or removes an OpenAPI operation must update the same literal or the gate breaks. Consider asserting a derived invariant (e.g. count matches the number of declared operationIds) instead of a literal, or at least document the requirement near the assertion. See also the Cross-PR section.
Cross-PR coordination
Two coordination points need a maintainer decision:
-
#267 (ephemeral PR environments for OpenShift e2e) - This PR and #267 both restructure the e2e CI plan and edit the same coordination surfaces (
.github/component-paths.json,DEVELOPMENT.md,skills/RECONCILE.md, and the e2e testing story). This PR convertse2e.ymlinto a reusable stage gated behindunitunder a two-top-level-workflow topology and codifies new rules in the maintain-ci skill (stages must beon: workflow_callonly; no third concurrent poller), while #267 introduces a new independently-triggeredpr-environment.ymlplus newscripts/ci/**andtests/e2e/***_test.shfiles that this PR's auto-discovery would begin executing. This PR'se2e-testing.spec.mdalso forward-references theephemeral-pr-environments.spec.md/pr-testdeprecation that #267 owns. Maintainers should decide how #267's new workflow fits the checks/tests topology (stage vs third top-level workflow) and fix a merge order so the shared spec/registration and the newly-auto-discovered shell tests land consistently. -
#264 (user registrations) and #237 (managed source releases) - Both add new OpenAPI operations (
getUserActivityStats;ServiceMetadata) and thus change the embedded operation count that this PR pins to40and, for the first time, enforces in a required CI gate viamake test. Whichever of these three merges last must reconcile theopenapi_embed_test.gocount (and #237 also edits that same test file). Maintainers should sequence these merges and agree on who updates the shared count so the newly-gating contract test does not go red onmain.
Findings Summary (ordered by severity, highest first)
- [Major] E2E stage can be skipped for e2e-only/pr_test-only changes (
e2eneeds a skippedunitstage) - CI Correctness (tests.yml) - [Minor]
make checkno longer enforces gate wiring / unit-test registration - Spec Completeness / CI Safety (check_ci_components.py) - [Minor] Enforced exact operation-count assertion is a fragile serialization point - Test Robustness (openapi_embed_test.go:58)
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
N/A |
| Actions pinned to commit SHA, images to digest | Pass |
Minimal workflow permissions (contents: read) |
Pass |
| No secrets in logs or workflow output | Pass |
| Reusable-workflow gating correct across skips | Needs verification |
| Component registered in CI (paths + jobs) | Pass |
CI enforcement (make check) coverage preserved |
Fail |
| Conventional commit messages | Pass |
| No em dashes in text files | Pass |
| sdk_typescript: ${{ needs.detect-changes.outputs.sdk_typescript }} | ||
| web_console: ${{ needs.detect-changes.outputs.web_console }} | ||
|
|
||
| e2e: |
There was a problem hiding this comment.
The e2e stage has needs: [detect-changes, unit] and no if:. If a PR changes only e2e-relevant paths that map to no unit component (tests/e2e/**, scripts/kind/**, components/pr-test/**), all unit child jobs are path-filtered out, the unit reusable-workflow caller job resolves to skipped, and this e2e job is then skipped by default - so the e2e suite never runs and CI Gate still passes. Consider gating on the unit result instead:
if: >-
always() &&
needs.detect-changes.result == 'success' &&
needs.unit.result != 'failure' &&
needs.unit.result != 'cancelled'e2e.yml already self-gates via plan-images.should_run, so running the stage whenever unit did not fail is safe. Please confirm against a real e2e-only PR before merge.
| rf"needs\.detect-changes\.outputs\.{re.escape(component)}\b", | ||
| ), | ||
| ) | ||
| for description, text, path, pattern in component_checks: |
There was a problem hiding this comment.
The old summary dependency assertion was removed and nothing replaces it for the new topology. This loop now checks only detector output, lint job, and job condition in checks.yml; it never verifies the lint job is present in checks-gate.needs, nor that a component with unit tests has a unit-tests.yml job and a tests.yml with: input. A component could be added per the maintain-ci steps yet be missing from the gate's needs (its failures would not fail CI Gate) with make check still green. Recommend re-adding a gate-membership check and a minimal unit-test wiring check.
| } | ||
| if operationCount != 37 { | ||
| t.Fatalf("embedded operation count = %d, want 37", operationCount) | ||
| if operationCount != 40 { |
There was a problem hiding this comment.
Bumping to 40 is correct for current main, but this PR is what first runs make test as a required gate, so this literal becomes a serialization point across every PR that adds/removes an OpenAPI operation (see #264 and #237, which change the count). Consider asserting a derived invariant (count equals number of declared operationIds) rather than a hardcoded literal, or document the requirement here.
My previous fix (a `services:` postgres container in the CI job)
treated the symptom, not the cause, and the real CI run still failed
after it -- now with `panic: ResetDB is not implemented for
non-integration-test env` instead of a connection refused.
The actual cause: `make test` ran `go test -v ./...` with no API_ENV
set, so it defaulted to the Development environment (an external
Postgres at a fixed host/port). But every plugins/* package mixes
plain unit test files with *_test.go files that call
RegisterIntegration -> ResetDB, which rh-trex-ai only permits under
API_ENV=integration_testing. Under that env, HyperShell's own
IntegrationTestingEnvImpl.OverrideDatabase spins up PostgreSQL
automatically via testcontainers-go on an ephemeral port -- exactly
what components/api-server/CLAUDE.md already documented ("Tests use
testcontainers-go to spin up PostgreSQL per test package"), just
with a stale env var name (HYPERSHELL_ENV instead of API_ENV) and a
`test` Makefile target that didn't match its own docs.
Fix `test` to match `test-integration`'s invocation
(API_ENV=integration_testing, -p 1 to avoid concurrent
testcontainer/DB contention, -count=1 since the env var isn't part
of Go's test cache key) but scoped to ./... instead of ./plugins/...
so it covers the whole component. This needs only Docker on the
runner (already present on ubuntu-24.04), so the manual `services:`
postgres container from the previous commit is removed as
unnecessary and, worse, misleading about how these tests actually
provision their database.
Updated CLAUDE.md, DEVELOPMENT.md, components/api-server/CLAUDE.md
(env var typo), and maintain-ci/SKILL.md's checklist note to match.
Assisted-by: Claude Sonnet 5
Amber reviewStatus: Stopped The pull request head changed before Amber posted the review. A later job can review the new head. |
The previous commit's assumption that API_ENV=integration_testing alone was sufficient turned out to be almost right: 12 of 13 components/api-server packages now self-provision PostgreSQL via testcontainers-go and pass. The one holdout is plugins/users -- its testmain_test.go hardcodes DB_FACTORY_MODE=external in-process (overriding whatever the job's environment says), which forces rh-trex-ai's OverrideDatabase to use NewTestFactory against a real, externally reachable PostgreSQL instead of a testcontainer. Without one, it fails with the original "connection refused" to localhost:5432. Restore the `services:` postgres container removed in the prior commit, scoped to test-api-server. It coexists fine with the other packages' ephemeral testcontainers (different ports); the comment explains which package it's actually for so a future reader isn't tempted to remove it again. Assisted-by: Claude Sonnet 5
Amber reviewStatus: Complete |
amber-review-bot
left a comment
amber-review-bot
left a comment
There was a problem hiding this comment.
Verdict
COMMENT. This is a clean, unusually well-documented CI restructuring: it splits the pipeline into an independently-triggered Checks workflow (lint, repository policy, SDK drift) and a Tests workflow (unit -> e2e) with reusable stage workflows, adds an auto-discovered shell-unit-test harness, and correctly folds the deleted repository-policy.yml/sdk-drift-check.yml into checks.yml. I found no blockers and no convention violations; the two Major items below are design/verification questions for maintainers rather than definite defects, and there is cross-PR coordination needed before merge.
What I verified (no action needed)
- The
openapi_embed_test.gochange37 -> 40is a legitimate correction, not a weakened guarantee: the embeddedopenapi.yamlcontains exactly 40 operations/operationIds, so the hard-coded expectation now matches reality (the contract test simply drifted while it was not being run in CI). make test(API_ENV=integration_testing go test -p 1 -v ./... -count=1) broadens coverage rather than removing any;run-shell-unit-tests.shcollects and re-raises per-file failures instead of swallowing them.- No dangling references remain to
lint.yml,make test-all,repository-policy.yml, orsdk-drift-check.yml(outside explanatory prose).detect-components.sh's newbash_testslogic reuses the already-definedall_components_changed.
Findings
[Major] E2E stage may be skipped for e2e-only changes. In tests.yml the e2e job declares needs: [detect-changes, unit] with no if:/always(). When a PR touches only e2e-owned paths (e.g. tests/e2e/**) and no unit-tested component, every job inside the reusable unit workflow is if:-skipped. If the reusable unit caller job then resolves to skipped (rather than success), the default needs semantics will skip e2e too, and an e2e-only change would never run Kind - a regression versus the old top-level e2e.yml, which ran plan-images unconditionally. Please confirm with an e2e-only test PR that the E2E stage still runs; if it does not, add if: ${{ !cancelled() && needs.unit.result != 'failure' }} (or equivalent) to the e2e job. Confidence: Medium.
[Major] Two jobs share the name CI Gate. checks.yml's checks-gate and tests.yml's tests-gate both use name: CI Gate. The inline rationale assumes the branch-protection picker disambiguates by workflow. That is reliably true only for repository rulesets (which let you scope a required check to a workflow file); classic branch-protection required checks match by context name alone, so a single required "CI Gate" can be satisfied ambiguously and one workflow's green gate could mask the other's failure. Either give the gates distinct names (Checks CI Gate / Tests CI Gate) or confirm this repo uses rulesets with per-workflow required checks before relying on it. Also note that required-check names change across this PR (old Lint CI gate, E2E CI gate, standalone Repository policy/OpenAPI SDK drift), so branch protection must be updated at merge. Confidence: Medium.
[Minor] Unit-test wiring is not enforced by make check. check_ci_components.py validates each component's detector output, lint job, and gate condition in checks.yml, but nothing enforces the parallel wiring in unit-tests.yml/tests.yml (input, with: pass-through, inputs.<component> gate). A future component could gain a lint job but silently lack unit-test gating. The maintain-ci SKILL now documents the manual steps well; consider extending the checker to close the gap. Confidence: High.
Cross-PR coordination
Two open pull requests require maintainer coordination before or alongside this one:
-
#237 makes competing edits to the same CI structure this PR replaces. It adds a
lint-pull-request-titlejob and acheck-build-metadatastep tolint.ymland wires them into theLint CI gatejob, while this PR renameslint.ymltochecks.yml, deletesrepository-policy.yml/sdk-drift-check.yml, and replacesLint CI gatewith thechecks-gate(CI Gate) rollup. Both also edit the sameplan-imagesjob ine2e.yml(this PR convertse2e.ymlinto a reusableworkflow_callstage; #237 adds release-version detection to that job) and both touch the two component Makefiles. Maintainers must decide a merge order and which PR ports its additions into the newchecks.yml/CI Gatestructure and the reusablee2e.yml; a straight merge will strand #237's job/step on a deleted workflow. -
#267 depends on the shell-unit-test contract this PR introduces. This PR adds
*_test.shauto-discovery (run-shell-unit-tests.sh/make ci-test), thebash_testsdetector output, and thetest-bashjob inunit-tests.yml. #267 authors new*_test.shfiles (its reaper/pr-env predicate tests and openshift driver test) whose CI execution and gating exist only once this harness lands, and both PRs add entries to the samee2ecomponent in.github/component-paths.json. Maintainers should sequence the merges (land this harness first, or explicitly wire #267 otherwise) so #267's shell unit tests are actually gated, and reconcile the overlappingcomponent-paths.jsonedits.
Findings Summary (ordered by severity, highest first)
- [Major] E2E stage may be skipped for e2e-only changes due to
needs: unitwithoutalways()- CI Correctness (tests.yml L102-L106) - [Major] Two jobs named
CI Gaterisk branch-protection ambiguity - CI / Branch Protection (tests.yml L123, checks.yml L358) - [Minor] Unit-test wiring not enforced by
make check- Spec Completeness (check_ci_components.py L87)
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass (N/A - CI/YAML) |
| Errors/failures propagated, none swallowed | Pass |
| No secrets in logs or responses | Pass |
| Image references pinned/consistent (actions by SHA, images by digest) | Pass |
| Conventional commit message | Pass |
| Test Diff Scrutiny (modified assertions justified) | Pass |
CI component registration consistent (make check) |
Pass |
| name: E2E | ||
| needs: | ||
| - detect-changes | ||
| - unit |
There was a problem hiding this comment.
The e2e job needs [detect-changes, unit] with no if:/always(). For a change that only touches e2e-owned paths (no unit-tested component), every job inside the reusable unit workflow is if:-skipped. If the unit caller job then resolves to skipped rather than success, default needs semantics will also skip this e2e job, so an e2e-only change would never run Kind - a regression versus the old top-level e2e.yml that ran plan-images unconditionally.
Please verify with an e2e-only PR. If the stage is skipped, add an explicit condition such as if: ${{ !cancelled() && needs.unit.result != 'failure' }}.
| # gate; only an actual 'failure'/'cancelled' -- or a detect-changes that did | ||
| # not succeed -- fails it. | ||
| tests-gate: | ||
| name: CI Gate |
There was a problem hiding this comment.
This gate and checks.yml's checks-gate are both named CI Gate. The header comment assumes the branch-protection picker disambiguates by workflow, which is reliably true only for repository rulesets (scoped to a workflow file). Classic branch-protection required checks match by context name alone, so a single required CI Gate can be satisfied ambiguously and one workflow's green gate could mask the other's failure.
Either name them distinctly (Checks CI Gate / Tests CI Gate) or confirm this repo uses rulesets with per-workflow required checks. Also update branch protection at merge: several required-check names change (old Lint CI gate, E2E CI gate, standalone Repository policy/OpenAPI SDK drift).
| # called from tests.yml) with its own detect-changes job. Verify the | ||
| # whole wiring lives there: detector output, lint job, and the job's | ||
| # gating condition on that same job's output. | ||
| component_checks = ( |
There was a problem hiding this comment.
This enforces each component's wiring in checks.yml (detector output, lint job, gate condition) but nothing enforces the parallel wiring in unit-tests.yml/tests.yml (the workflow_call input, the with: pass-through, and the inputs.<component> gate). A future component could gain a lint job yet silently lack unit-test gating. The maintain-ci SKILL documents the manual steps, but consider extending this checker to guard the unit-test path too.
Three issues flagged in PR #266 review: 1. e2e's `needs: [detect-changes, unit]` had no `if:`. GitHub Actions skips a job by default if any needed job failed OR was skipped, so a PR touching only e2e-owned paths (every job inside `unit` path-filtered away, making the `unit` caller job itself resolve to `skipped`) would silently skip `e2e` too -- a regression versus the old top-level e2e.yml, which ran unconditionally. Add `if: ${{ !cancelled() && needs.detect-changes.result == 'success' && needs.unit.result != 'failure' }}` so only an actual unit failure blocks e2e, not a skip. 2. checks.yml and tests.yml's gate jobs were both named plain "CI Gate". Verified via `gh api .../rulesets`: this repo's branch protection is a ruleset whose required_status_checks match by (context name, integration_id) only, not by workflow file, and both workflows' checks share the "GitHub Actions" integration_id. Two identically-named gates from different workflows are indistinguishable to it, so either succeeding could satisfy a required "CI Gate" check while the other silently failed. Reverted to distinct names: `Checks CI Gate` / `Tests CI Gate`. 3. check_ci_components.py only validated a component's checks.yml (lint) wiring, not its unit-tests.yml wiring, so a component could gain a lint job while its unit-test input/gate silently rotted or was never added. Added a `unit_tested` boolean field to component-paths.json and a matching validation pass: for any component with `unit_tested: true`, verify the detection output is passed to `unit` in tests.yml, declared as a `workflow_call` input in unit-tests.yml, and referenced in some job's `if:` there. Verified the new check fails correctly when a component falsely claims unit_tested. Also confirmed via the real PR #266 CI run that the ruleset's currently-configured required checks ("Lint CI gate", "E2E CI gate") are already stale from before this branch's restructure and will need to be updated to "Checks CI Gate" / "Tests CI Gate" regardless -- that's a repo-settings change outside this branch's scope. Assisted-by: Claude Sonnet 5
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 cleanly splits CI into a concurrent Checks workflow and a Tests orchestrator that gates the expensive Kind e2e stage behind cheap unit tests, with matching spec, skill, and check_ci_components.py updates that keep component registration enforceable. I verified the repository-policy check still passes, the workflow/script bash parses, and the one modified test assertion is a legitimate stale-count fix, so I'm approving with only minor, non-blocking observations.
Amber Assessment
Scope is CI plumbing plus supporting docs/specs; there is no production Go/pod-spec change, so the security-context, secret-handling, and reconcile conventions are largely N/A. The design is well-reasoned and thoroughly documented in-workflow, and the two-gate branch-protection naming rationale (Checks CI Gate vs Tests CI Gate) is sound.
Verification performed
python3 scripts/check_ci_components.py-> passes (9 components registered, unit-test wiring validated).bash -nonscripts/run-shell-unit-tests.shand.github/scripts/detect-components.sh-> OK.- Confirmed the embedded OpenAPI spec now has 40 operations (37 was stale after #265 added 3), so
openapi_embed_test.go37->40 is a correct fixup, not a loosened guarantee. - Confirmed
API_ENV=integration_testingis the env var the framework/tests actually read (plugins/users/testmain_test.go), so themake testand CLAUDE.md updates are consistent. - Confirmed
e2e.ymluses nosecrets.*(onlygithub.token), so calling it as a reusable workflow withoutsecrets: inheritis safe.
Minor findings (non-blocking)
- The
bash_testsdetector output and thetest-bashgate are not covered bycheck_ci_components.py(it only validates entries registered incomponent-paths.json), so a future rename of that output/input would silently disable shell-test gating without the policy check catching it. Consider a light assertion for the shell-test wiring. - Shell-test auto-discovery runs every
*_test.shin the singleShellunit job. Today all four discovered files are self-contained with mocked externals, but there is no marker/convention preventing a future*_test.shthat needs a live cluster/network from being pulled into the unit job. Worth a note in the runner or the maintain-ci skill.
Cross-PR coordination
PR #267 (ephemeral OpenShift PR environments) modifies .github/workflows/e2e.yml on the assumption that it stays a top-level, independently-triggered workflow: it adds a workflow_run trigger, pull-requests: read permission, a workflow_run-based concurrency group, an if: github.event_name != 'workflow_run' guard on plan-images, and a new e2e-openshift job whose logic depends on github.event.workflow_run.* context and secrets.OPENSHIFT_PR_ENV_*. This PR instead converts e2e.yml into a reusable workflow (on: workflow_call only), removes its event triggers and concurrency block, drops its summary gate, and moves triggering/sequencing/secrets ownership to a new tests.yml orchestrator. These are competing, mutually incompatible designs for the same file: once e2e.yml is workflow_call-only, #267's workflow_run trigger and github.event.workflow_run context no longer apply, and its secret usage would need to be threaded through the orchestrator. Maintainers need to decide a merge order and reconcile the two e2e-pipeline designs; whichever lands second must be reworked onto the other's structure.
Findings Summary (ordered by severity, highest first):
- [Minor]
bash_testsshell-test wiring is not covered bycheck_ci_components.py, so a rename could silently disable it - CI Coverage (unit-tests.yml L164) - [Minor] Shell-test auto-discovery runs every
*_test.shin one unit job with no guard against future cluster-dependent tests - CI Robustness (run-shell-unit-tests.sh L14)
Convention Checklist (omit conventions not applicable to the diff):
| Convention | Result |
|---|---|
| Modified test assertion justified (no removed guarantee) | Pass |
| No secrets in logs / workflow outputs (secrets masked) | Pass |
| Image/action references pinned to digests/SHAs | Pass |
| Conventional commit messages | Pass |
| Spec + skill updated to match implementation | Pass |
Component CI registration enforced (make check) |
Pass |
|
|
||
| test-bash: | ||
| name: Shell | ||
| if: inputs.bash_tests == 'true' |
There was a problem hiding this comment.
Minor: this test-bash gate on inputs.bash_tests is the one CI wiring not verified by scripts/check_ci_components.py. That policy check only validates components registered in component-paths.json; bash_tests is synthesized in detect-components.sh and has no registration, so renaming the detector output or this input would silently stop shell tests from ever running without make check failing. Consider a small assertion covering the shell-test path.
| failed_tmp="$(mktemp)" | ||
| trap 'rm -f "${tmp}" "${failed_tmp}"' EXIT | ||
|
|
||
| find . -type d \( -name .git -o -name node_modules -o -name vendor \) -prune -o \ |
There was a problem hiding this comment.
Minor: this discovers and runs every *_test.sh in one Shell unit job. All four current files are self-contained (mocked oc, etc.), so this is fine today. There is no convention/marker preventing a future *_test.sh that needs a live cluster or network from being auto-pulled into the unit job, where it would fail without any opt-in. Worth documenting the "self-contained unit test only" expectation here or in the maintain-ci skill.
2 participants
What
Add a new GitHub Actions workflow that runs unit tests (Go, frontend, shell) after the lint check passes and before E2E tests start, with smart component-based change detection.
Highlights
.github/workflows/unit-tests.ymlworkflow with conditional jobs for frontend, Go (API server, control plane, CLI, SDK generator), and shell unit tests*_test.shpattern (no allowlist needed)Unit Tests CI gatebefore creating the Kind clusterunit-test-allandci-testtargets for local testingScope
This PR establishes the unit-test gating layer in the CI pipeline. It is a prerequisite for running robust unit tests before E2E, reducing flaky E2E failures due to untested unit-level regressions.