refactor: migrate endpoints.py do_roadblock() to toolbox's shared implementation - #864
Merged
Merged
Conversation
…lementation endpoints.py maintained its own 110-line do_roadblock() that instantiated the roadblock class directly, diverging from toolbox.roadblock.do_roadblock (already used by engine_lib.py). Bug fixes to one were not guaranteed to reach the other. Replace it with a thin wrapper that delegates the actual roadblock mechanics to toolbox.roadblock.do_roadblock, keeping only the endpoint-specific pre-connect ping diagnostic and message-stream logging on top. The function signature is unchanged, so no call site (endpoints.py, kube.py, remotehosts.py) needed updating. The other half of PERFNFV-462 -- consolidating the duplicated ROADBLOCK_EXITS dict between toolbox's messages.py and roadblock.py -- was already fixed by a prior toolbox commit (b9b5a6d). Adds the first unit tests for endpoints.py's do_roadblock(), following the existing SourceFileLoader-based mocking pattern used for rickshaw-run.py's tests, since CI does not check out toolbox/roadblock/fabric/invoke/paramiko for this job. Refs: PERFNFV-462 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
atheurer
previously approved these changes
Aug 11, 2026
Adopts the same local-reusable-workflow gate pattern already proven on toolbox (PERFNFV-468): unittest.yaml becomes workflow_call-only, and crucible-ci.yaml's new call-unittest job runs it first, using the exact same if: condition as call-real-core-release-crucible-ci, with call-unittest added to both call-real's and crucible-ci-complete's needs: (the latter is required so a failing gate actually surfaces as crucible-ci-complete failing, not just as a skipped downstream job). Unlike toolbox, rickshaw already had a standalone, independently triggered unittest.yaml whose own unittest-complete job was a required branch-protection check running in parallel with, not gating, crucible-ci's expensive integration matrix. Removed that standalone pull_request trigger and its changes/faux-unittest/unittest-complete jobs entirely -- crucible-ci.yaml's own changes/faux-* logic already covers the docs-only fast path. This is a real merge to a clean gate-only shape, not an addition, so rickshaw's branch-protection ruleset (default-branch-required) was updated to drop unittest-complete from required status checks, leaving crucible-ci-complete as the sole required check -- confirmed via the toolbox rollout that this is sufficient, since crucible-ci-complete transitively depends on the gate. Also removed unittest.yaml from crucible-ci.yaml's own changes job skip-file list: it's the actual test payload now that it's wired into the gate, not orchestration, so a PR that only breaks the test-runner itself should not be classified as docs-only and skip validating that exact breakage (same reasoning applied when toolbox's version was deliberately left out of its own skip-list). Refs: PERFNFV-468 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Mirrors the live rickshaw branch-protection ruleset update made as part of gating crucible-ci on rickshaw's own unit tests: unittest-complete is no longer required now that it's been removed as a standalone top-level workflow (see previous commit), leaving crucible-ci-complete as the sole required status check. These files are documentation/tracking snapshots only (per .github/rulesets/README.md) -- they don't drive GitHub's actual enforcement, but should stay in sync with it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
atheurer
self-requested a review
August 11, 2026 17:29
atheurer
approved these changes
Aug 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PERFNFV-462 — migrate endpoints.py's do_roadblock() to toolbox's shared implementation
endpoints.pymaintained its own independentdo_roadblock()(~110 lines, directroadblockclass instantiation) that diverged fromtoolbox.roadblock.do_roadblock— the implementationengine_lib.pyalready uses. Bug fixes to one weren't guaranteed to reach the other.endpoints.py'sdo_roadblock()to a thin wrapper delegating totoolbox.roadblock.do_roadblock, keeping only the endpoint-specific pre-connect ping diagnostic and message-stream logging on top. Function signature unchanged, so no call site (endpoints.py,kube.py,remotehosts.py) needed updating.ROADBLOCK_EXITSdict between toolbox'smessages.pyandroadblock.py— was already fixed by a prior toolbox commit (b9b5a6d), so no toolbox changes are included here.endpoints.py'sdo_roadblock()(tests/test_endpoints_do_roadblock.py), mocking out toolbox/roadblock/fabric/invoke/paramiko via the existingSourceFileLoaderpattern since CI doesn't check those out for this job.PERFNFV-468 — gate crucible-ci on rickshaw's own unit tests
unittest.yamlbecomesworkflow_call-only, andcrucible-ci.yaml's newcall-unittestjob runs it first, using the exact sameif:condition ascall-real-core-release-crucible-ci, added to bothcall-real's andcrucible-ci-complete'sneeds:.unittest.yamlwhoseunittest-completejob ran in parallel with, not gating, the expensivecrucible-ciintegration matrix. Removed that standalone trigger and itschanges/faux-unittest/unittest-completejobs entirely —crucible-ci.yaml's ownchanges/faux-path logic already covers the docs-only fast path.default-branch-requiredbranch-protection ruleset to dropunittest-completefrom required status checks (it can no longer report, since the workflow that produced it is gone), leavingcrucible-ci-completeas the sole required check. Confirmed via the toolbox rollout that this is sufficient, sincecrucible-ci-completetransitively depends on the new gate. Synced.github/rulesets/branches/default-branch-required.json(documentation snapshot only) to match.unittest.yamlfromcrucible-ci.yaml's ownchangesjob skip-file list — it's the actual test payload now that it's wired into the gate, so a PR that only breaks the test-runner itself shouldn't be classified as docs-only and skip validating that exact breakage.Refs: PERFNFV-462, PERFNFV-468
Test plan
pytest -v tests/*.py— 15/15 passing (9 pre-existing + 6 new)py_compilecleanValueErrorguard still workredis_servervalue) to confirm it actually fails on regressions, not just passes triviallywait_fortoday (grepped all three call sites), so the signature-preserving wrapper approach has zero blast radius on callersyaml.safe_loadclean on both modified workflow filescall-unittestgatingcall-real-core-release-crucible-ci, and thatcrucible-ci-completeis the only check GitHub is waiting on🤖 Generated with Claude Code