Skip to content

ci: add one job branch protection can require - #90

Open
Kartikey1306 wants to merge 1 commit into
embeddedos-org:masterfrom
Kartikey1306:ci/required-check-gate
Open

ci: add one job branch protection can require#90
Kartikey1306 wants to merge 1 commit into
embeddedos-org:masterfrom
Kartikey1306:ci/required-check-gate

Conversation

@Kartikey1306

Copy link
Copy Markdown
Contributor

required_status_checks is null on this repository's master — the same gap
eos#92 and ebuild#87 track. @srpatcha noted it applies here too:

The same gap is open in ebuild (#87 there) and eBoot — checks=null on all
three.

This repo has paid for it. #70 landed a TEST() macro that had lost a \ line
continuation; master was red from 08-31 08:07 until #77, and the 24 resulting
errors all pointed away from the cause. Nothing built the merge result before it
became master.

Why the existing checks cannot be required as they are

release is skipped on every pull request (if: startsWith(github.ref, 'refs/tags/v')), and a required check that is skipped never reports — the
pull request waits for a status that never arrives, which presents as
infrastructure flake rather than a policy.

Requiring test, build-arm and static-analysis individually works today and
stops working the next time a job is added, silently.

What this adds

One job, ci-gate, displayed as CI Gate — the single name to require:

  ci-gate:
    name: CI Gate
    needs: [test, build-arm, static-analysis]
    if: always()

The test is the part that keeps working

tests/unit/test_ci_gate.py (6 tests) parses ci.yml and fails if a job is
added without being wired into the gate.

Verified by mutation:

mutation result
drop build-arm from needs 2 failed
remove if: always() 1 failed
add a new job, forget to wire it in 2 failed
(restored) 6 passed

pyyaml is added to the pytest installer, which had only pytest pytest-cov.

Verification

item result
pytest tests/ PASS — 25 passed, 1 skipped
pytest tests/unit/test_ci_gate.py PASS — 6 passed
ctest PASS — 20/20
yaml.safe_load of the edited workflow PASS
mutation sweep above PASS — all three caught

Branches from master directly; unlike the other two repos, master here is
green.

What is still a maintainer action

Settings → Branches → master → Require status checks → add CI Gate. The
same PR is open on ebuild (#103) and eos (#121), so the name is identical in all
three.

`required_status_checks` is null on master here, the same gap eos#92 and
ebuild#87 track: nothing builds the merge result before it becomes master.
This repository has had the consequence twice -- embeddedos-org#70 landed a TEST() macro
missing a line continuation, and master was red from 08-31 08:07 until embeddedos-org#77.

Marking a check required is a settings change, but it needs a name worth
pointing at. `release` is skipped on every pull request, and a required check
that is skipped never reports, so the pull request waits for a status that
never arrives. Requiring the three real jobs individually works only until the
next one is added and silently sits outside the requirement.

`ci-gate` is one job, one stable name, that succeeds only if every other job
that runs on a pull request succeeded. `if: always()` so a real failure shows
as a red X rather than a pull request that hangs. Any non-success result fails
it, `skipped` included -- a job that did not run did not verify anything, which
is the fail-open shape embeddedos-org#38, embeddedos-org#59 and embeddedos-org#82 removed from the boot path.

tests/unit/test_ci_gate.py fails if a job is added without being wired into
the gate. Verified by mutation: dropping a job from `needs`, removing
`if: always()`, and adding an unwired job are each caught.

pyyaml added to the pytest installer, since the test parses the workflow.

Verified: 25 Python tests pass (1 skipped), ctest 20/20.
@codecov-commenter

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@Kartikey1306

Copy link
Copy Markdown
Contributor Author

The gate was run against a real failure, not just reasoned about

Pushed a deliberately failing test to a branch on my fork so the whole workflow
would execute with one job red, and watched what the gate did.

Job results in that run:

  failure  Build & Test (Linux x86_64)      <- the deliberate failure
  success  Static Analysis (cppcheck + clang-tidy)
  skipped  Cross-compile ARM Cortex-M4      <- skipped, it needs: test
  skipped  Create GitHub Release            <- tag-only, correctly outside the gate
  failure  CI Gate                          <- reported red

And the gate's own log:

##[error]CI Gate failed. These jobs did not succeed:
  test: failure
  build-arm: skipped
##[error]Process completed with exit code 1.

Three things that confirms, which the workflow file alone does not:

  1. if: always() works. The gate ran at all despite an upstream failure. Had
    it been omitted, the gate would have been skipped — and a skipped required
    check never reports, so the pull request would have sat waiting for a status
    that never arrives instead of showing a red X.
  2. skipped is treated as a failure. build-arm was skipped and the gate
    named it. Had skipped counted as a pass, a job that verified nothing would
    have been reported as verified.
  3. release being outside the gate is correct. It was skipped and the gate
    did not care, which is exactly why it cannot be a required check itself.

The self-test branch has been deleted; nothing from it is in this PR.

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