Repository navigation
ci: add a single aggregate required-check job to unit-tests.yml - #856
Conversation
|
NEEDS CHANGES on 1. The five A different workflow file, which 2. What is confirmed, and the sharp one went the author's way. I flagged Also confirmed: context string The result-checking step was exercised, not eyeballed — eight fabricated One latent trap worth recording: under |
2dbc790 to
a0fc156
Compare
|
NEEDS CHANGES on 1. The cell count contradicts its own quantifier. 36 cell names exist; 30 is the gating subset. "every" and "30" cannot both be true. Either the number 2. "Splitting the 3.15 legs is the only alternative" is overstated. But it is not the only route. Moving The documentation finding is the valuable part of this round, and it cuts toward the risk being real. GitHub Confirmed this round: the 17/22 arithmetic is exact in both comment and body (5+5+5+2 = 17, 5 The |
a0fc156 to
aa5d7ac
Compare
|
NEEDS CHANGES on 1. The comment says the The omitted sentence is the scoping one — the sentence a checking reader notices first, because it is the one 2. The PR body asserts "140 insertions" under "Verified:". Actual at this head is +166/−0 Two cosmetics worth taking in the same pass: Everything substantive is confirmed, and the numbers I asked to be attacked all held exactly:
One caveat recorded, not a required change: "growing or shrinking with the matrix" over-claims in exactly one |
Ruleset 23497679's required_status_checks names 22 exact check contexts by
literal string, and GitHub rulesets support no wildcard. Of those 22, only
the five `Engines Python <version>` names are actually dead -- `engine-tests`
stopped producing them once it became a Python x engine matrix. The five
`Compat Python <version>` names are live, emitted by the `compatibility` job
in python-compatibility.yml, and must stay required as-is; nothing in this
file can stand in for a job in a different workflow file.
- Add a `required-checks` job that `needs: [test, integration, engine-tests,
pypcap-parity]` and reports one stable check context, `Required checks
passed`, that the ruleset can require in place of the 17 `test`/
`integration`/`Engines`/`pypcap-parity` slots (not the five `Compat` ones).
- Guard it with `if: ${{ always() && inputs.gate-only != true }}`: without
`always()`, a plain `needs:` job is *skipped* -- not failed -- the moment
any dependency fails, and GitHub's own docs list a skip as a passing
status for required checks, which would make this gate worse than none.
`gate-only != true` keeps it from spuriously failing on the release-only
`gate-only: true` call path, where its four dependencies are themselves
skipped by design.
- The job's own step inspects each dependency's `needs.<job>.result`
explicitly, printing every value and failing loudly (naming the job) on
anything other than `success`.
- Comment documents one open, unverified risk: whether a `continue-on-error`
3.15 `engine-tests` cell's real failure is neutralised to `success` in
`needs.engine-tests.result` the way GitHub documents for a step's
`conclusion` (but not for a job's `result`) is unstated for jobs. Weighed
two fixes -- step-level `continue-on-error` (hides the 3.15 regression
instead of merely not blocking on it) and splitting the 3.15 legs into
their own job (182 raw lines of duplication, no YAML anchors available)
-- and took neither, flagging the risk in the comment instead.
YAML verified with `yaml.safe_load` via the repo venv; `git diff --numstat`
against `origin/main` shows one file, 167 insertions, 0 deletions.
aa5d7ac to
28e4b88
Compare
|
GOOD TO GO on Verified by me: That last one is the load-bearing check: the Three cross-review rounds on opus against a sonnet author. What each round cost, for the record: round 1 caught Your step once this mergesEdit ruleset
Net: 22 contexts → 6. One open risk, stated plainly because it is unverifiable without a real run. |
|
Merged — here is the exact ruleset edit I owed you, now that this is on Ruleset Add — exactly this string: Remove these 17: Keep these 5 — do not remove them: They come from The one open risk, restated so it is not a surprise. |
…ck job Ruleset 23497679 names 22 exact check contexts with no wildcard support, so the required list needs hand-editing on every matrix change; five of them (the old single-cell Engines Python <version> names) are already dead since #849, and the five Compat Python 3.10-3.14 names stay live and untouched. Adds one job, required-checks (context "Required checks passed"), depending on test/integration/engine-tests/pypcap-parity with if: always() so it cannot itself be silently skipped -- the failure mode GitHub's own troubleshooting docs warn a bare needs: list falls into -- plus an explicit per-dependency check that names which job broke. Not breaking: workflow-only, no library or test code changes. The 17-of-22 ruleset replacement it enables is a separate repository setting this PR does not itself touch.
…rated template
pcapkit/vendor/default.py's generated template now emits
class {NAME}(EnumRegistry, IntEnum) and drops its own get/register/
_unregistered_member entirely, so every const-enum class it produces
inherits #855's guard instead -- e.g. TransType.register(6,
'TOTALLY_NEW_NAME') now raises ValueError naming the existing member and
pointing at register_alias(), where it used to mint nothing and raise
nothing.
Census re-derived independently by AST over the merge commit rather than
taken from the PR table: 121 const modules hold 127 enum classes (three
modules define more than one class each). 6 classes already used
EnumRegistry from #855; of the other 115 modules, 105 share the generated
template byte-for-byte and are converted here, and the remaining 10 keep
their own bespoke __new__ and are left alone -- ftp/command (4 classes),
ftp/return_code (3), http/method, http/status_code, pcapng/option_type,
reg/apptype/apptype.py (2) and its four transport subclasses. Total now
inheriting EnumRegistry: 111 of 127 classes, up from 6. Also renames
pcapkit/corekit/enums.py to enum.py (no -s), the maintainer's ruling.
util/changelog_md.py regenerated CHANGELOG.md for all three entries added
across this and the two preceding commits (#855, #856, #858); --check
exit 0.
…ck job Ruleset 23497679 names 22 exact check contexts with no wildcard support, so the required list needs hand-editing on every matrix change; five of them (the old single-cell Engines Python <version> names) are already dead since #849, and the five Compat Python 3.10-3.14 names stay live and untouched. Adds one job, required-checks (context "Required checks passed"), depending on test/integration/engine-tests/pypcap-parity with if: always() so it cannot itself be silently skipped -- the failure mode GitHub's own troubleshooting docs warn a bare needs: list falls into -- plus an explicit per-dependency check that names which job broke. Not breaking: workflow-only, no library or test code changes. The 17-of-22 ruleset replacement it enables is a separate repository setting this PR does not itself touch.
…rated template
pcapkit/vendor/default.py's generated template now emits
class {NAME}(EnumRegistry, IntEnum) and drops its own get/register/
_unregistered_member entirely, so every const-enum class it produces
inherits #855's guard instead -- e.g. TransType.register(6,
'TOTALLY_NEW_NAME') now raises ValueError naming the existing member and
pointing at register_alias(), where it used to mint nothing and raise
nothing.
Census re-derived independently by AST over the merge commit rather than
taken from the PR table: 121 const modules hold 127 enum classes (three
modules define more than one class each). 6 classes already used
EnumRegistry from #855; of the other 115 modules, 105 share the generated
template byte-for-byte and are converted here, and the remaining 10 keep
their own bespoke __new__ and are left alone -- ftp/command (4 classes),
ftp/return_code (3), http/method, http/status_code, pcapng/option_type,
reg/apptype/apptype.py (2) and its four transport subclasses. Total now
inheriting EnumRegistry: 111 of 127 classes, up from 6. Also renames
pcapkit/corekit/enums.py to enum.py (no -s), the maintainer's ruling.
util/changelog_md.py regenerated CHANGELOG.md for all three entries added
across this and the two preceding commits (#855, #856, #858); --check
exit 0.
Please follow the guide below
make pylint,make mypy,make isort)make testpasses, and a test case covers the change — N/A, workflow-only; no library code changesWhat is the purpose of your pull request?
ci— workflows or build toolingDescription of your pull request and other information
The problem
Ruleset
23497679'srequired_status_checksnames 22 exact check contexts by literalstring, and GitHub rulesets support no wildcard: 5 for
test, 5 forintegration, 5 forCompat Python 3.10..3.14, 5 for the old single-cellEngines Python <version>name, and2 for
pypcap-parity. Only the 5Enginesnames are actually dead —engine-testsstopped producing them once it became a Python × engine matrix. The 5
Compatnames arelive, emitted by
python-compatibility.yml's owncompatibilityjob on the samepush/pull_request triggers, confirmed by reading that file — a
needs:in this file cannever stand in for a job in a different workflow file, so those 5 must stay required as-is.
Growing or shrinking the matrix still means hand-editing the required list — the owner's
framing: "CI all passed but russet gates not getting their expected results? Actually are
those ruleset things necessary? We can already gate on CI passing."
What this adds
One new job,
required-checks, emitting a single stable check context:That is the exact string to add to ruleset
23497679, in place of the 17test/integration/Engines/pypcap-parityentries — not the 5Compatones, which stay.It
needs: [test, integration, engine-tests, pypcap-parity]; notgate(release-only,permanently skipped on a PR) and not
changelog(a drift check, not currently one of the22, left for a separate call).
Why it isn't just a plain
needs:jobA job with a bare
needs:list is skipped, not failed, the moment a dependency fails —and GitHub's own "Troubleshooting required status
checks"
page lists
skippedas a passing status, and says a job skipped because it depends on afailed job "may not block merging." Its own recommended fix is this job's shape:
if: always()plus an explicit per-dependency check, printing everyneeds.<job>.resultand failing loudly (naming the job) on anything but
success.engine-tests's 6unsupported+ 4not-installablecells don't need special-casing: that job's own installstep exits 0 for a cell that declined exactly as expected, so those cells still report
successat the job level, neverskipped.One risk is flagged rather than resolved:
engine-testsalso carriescontinue-on-errorfor its 3.15 legs (non-blocking, per #845), and GitHub's docs don't saywhether a real 3.15 failure is neutralised to
successinneeds.engine-tests.resulttheway it is for the run's own conclusion. See the job's own comment for why splitting 3.15
into its own job was judged too much churn to close a single undocumented edge.
Verified:
yaml.safe_loadparses the file;git diff --numstatagainstorigin/mainshowsone file, 167 insertions, 0 deletions. Unverified: the run itself — GitHub Actions can't be
exercised locally.