Skip to content

refactor(on-pr): run PR checks as a single job - #138

Merged
AlexanderLanin merged 6 commits into
mainfrom
on-pr
Jul 17, 2026
Merged

AlexanderLanin merged 6 commits into
mainfrom
on-pr

Conversation

@AlexanderLanin

Copy link
Copy Markdown
Member

Why

on-pr.yml is the main PR-check entry point for all S-CORE repositories, but it fans out into 8 separate jobs from a capability-detection job. For a typical repo this means:

  • Skipped-job clutter — a docs repo skips every Bazel job, a Python repo skips every Bazel job, leaving a wall of skipped entries in the PR checks UI.
  • Repeated overhead — every job that does run spins up its own runner and re-checks-out the repo, so a handful of ~3-second checks each pay runner spin-up + checkout cost.
  • Redundant setup — Bazel is installed three times (in detect-repo-bazel-targets, bazel-format-check, and bazel-copyright-check).

Few maintainers want independent pass/fail feedback from a dozen trivial jobs; a single consolidated result is more useful and much cheaper.

What

Rewrite on-pr.yml as one job whose steps are the individual checks.

Non-stopping, still per-step visible

Every check step is gated if: ${{ !cancelled() && <capability> }}. Because the condition isn't the implicit success(), a failing check does not skip the checks after it — each still shows its own red/green, and the job fails if any step failed. No shell "collect failures and exit at the end" aggregation is needed.

Failures vs. infrastructure

checkout and the detection steps keep the default success() gate. If either fails, the capability outputs are empty, so every <capability> == 'true' guard is false and all checks skip while the job is already red — a broken checkout can't run checks against a missing tree, but a broken check never blocks its siblings.

Nothing heavy installs unless needed

setup-uv and setup-bazel are themselves gated, so a docs-only repo installs neither, and Bazel is set up once — shared by target-detection + format + copyright + lockfile.

Trade-off

Checks now run sequentially in one runner instead of in parallel across runners (for a Bazel repo: format → copyright → lockfile one after another). This is the deliberate cost of eliminating per-job runner spin-up and skipped-job clutter.

Changes

  • .github/workflows/on-pr.yml — collapsed 8 jobs into a single pr-checks job; checks gated with !cancelled() && <capability> so failures don't stop siblings.
  • Inlined the two bzlmod-lock-check.yml checks (mod tidy + git diff, and mod deps --lockfile_mode=error) and dropped on-pr's internal uses: call. The standalone bzlmod-lock-check.yml is unchanged (still consumed directly by other repos).
  • Bazel now set up once and shared across all Bazel checks (was 3× separate setups).
  • bazel-module-name-check kept but disabled via the && false idiom (a bare if: false is rejected by actionlint) until its detection script is fixed.
  • Preserved the pull_request_target refusal guard (from bzlmod-lock-check.yml) as the first step, now covering the whole job.
  • Dropped the unused id-token: write request — no step consumes an OIDC token (sibling Bazel workflows use setup-bazel without it).

Note: scoped to on-pr.yml only. Not yet exercised on a live PR run — the !cancelled() non-stopping behavior and the single shared Bazel setup should be confirmed against a real Bazel repo before rollout.

on-pr.yml previously fanned out into 8 jobs from a capability-detection
job. Most were skipped per repo, every running job paid runner spin-up +
checkout overhead, and Bazel was set up three separate times.

Rewrite it as one job whose steps are the individual checks. Each check
step is gated with `if: ${{ !cancelled() && <capability> }}`, so a
failing check still reports its own pass/fail but does not stop the
checks after it, while the job fails if any step failed. setup-uv and
setup-bazel are themselves gated, so nothing heavy installs unless the
repo needs it, and Bazel is set up once.

The two bzlmod-lock-check.yml checks are inlined and on-pr's internal
`uses:` call is dropped; the standalone bzlmod-lock-check.yml is
unchanged. bazel-module-name-check is kept but disabled (&& false) until
its detection script is fixed. Also drops the unused id-token: write
request, as no step consumes an OIDC token.

Signed-off-by: Alexander Lanin <Alexander.Lanin@etas.com>
Callers can now force-skip an individual check even when its capability is
detected, e.g. a repo that has a .pre-commit-config.yaml but does not want
on-pr to run it.

Adds five optional boolean workflow_call inputs, all defaulting to false so
existing callers are unaffected:
  skip-pre-commit, skip-python-tests, skip-format, skip-copyright,
  skip-bzlmod-lock

Each check step's condition gains `&& !inputs.skip-<name>`.

Signed-off-by: Alexander Lanin <Alexander.Lanin@etas.com>
The top-level README is long and never documented on-pr at all, even though
it is the main PR entry point. Add docs/workflows/on-pr.md covering usage,
auto-detected checks, the skip-* inputs, behavior (non-stopping, sequential,
lazy tool setup, pull_request_target refusal), permissions and runner
selection. Link it as the first row of the README's workflow table.

Signed-off-by: Alexander Lanin <Alexander.Lanin@etas.com>
Move the doc from docs/workflows/ to .github/workflows/on-pr.md so it sits
right beside the workflow it documents. Keeping them together makes it far
more likely the docs get updated whenever the workflow changes. Update the
README link accordingly.

Signed-off-by: Alexander Lanin <Alexander.Lanin@etas.com>
A red check with no explanation leaves contributors guessing. Each check
now writes a tailored block to $GITHUB_STEP_SUMMARY on failure: what went
wrong, how to reproduce it locally, and how to fix it (e.g. `bazel run
//:format` for formatting, `bazel mod tidy` for the lockfile). The job
still fails as before. Guidance lives inside each step, next to the command
it explains. Document the behavior in on-pr.md.

Signed-off-by: Alexander Lanin <Alexander.Lanin@etas.com>

@MaximilianSoerenPollak MaximilianSoerenPollak left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For me this looks like a good improvement and logical

@AlexanderLanin

Copy link
Copy Markdown
Member Author

as the old on-pr is barely used, this change can only be an improvement. I'll merge this.

@AlexanderLanin
AlexanderLanin merged commit 88cb460 into main Jul 17, 2026
4 checks passed
@AlexanderLanin
AlexanderLanin deleted the on-pr branch July 17, 2026 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

2 participants