ci: gate binding publishers on the core via a needs: job (#500 follow-up)#515
Merged
Conversation
…-up) PR #514 fixed the #500 release race with an inline crates.io sparse-index poll inside each binding build — but that runs redundantly once per matrix leg (5x in the node `build` matrix, 5x in ruby `cross-gems`). Realise the issue's option (3) `needs:`-based ordering instead: a single `wait-for-core` gating job per binding workflow that the build job depends on, so a build doesn't start until `disarm 0.<minor>` is on crates.io. Scope the `needs:` within each existing workflow rather than merging the three publishers into one file: publish-node.yml / publish-ruby.yml are registered as OIDC Trusted Publishers keyed to their exact filenames (npmjs.org / rubygems.org), so a merge would break trusted publishing until each registry is reconfigured out-of-band. Triggers, filenames, and environments are unchanged. - publish-node.yml: add `wait-for-core` (release/dispatch-gated), remove the inline poll from `build`, add `needs: wait-for-core` to `build`. - publish-ruby.yml: same, gating `cross-gems`. On push/PR the gating job is skipped; a skipped `needs:` dependency does not block the dependent, so the #374 in-repo-patched validation builds run unchanged. Assisted-by: Claude:claude-opus-4-8 Signed-off-by: Richard Quinn <[email protected]>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the binding publish workflows (Node and Ruby) to avoid redundant per-matrix polling for the core crate by introducing a single wait-for-core gating job per workflow, then wiring the build job to depend on it via needs:—while keeping workflow filenames/environments unchanged for OIDC Trusted Publishing compatibility.
Changes:
- Added a
wait-for-corejob topublish-node.ymlandpublish-ruby.ymlto poll crates.io once per workflow (release/dispatch only). - Removed the inline per-matrix poll from the Node/Ruby build jobs and replaced it with
needs: wait-for-coregating. - Updated job ordering so the release path waits for
disarm 0.<minor>to be available on crates.io before starting binding builds.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| .github/workflows/publish-node.yml | Adds wait-for-core and gates build via needs: (but currently breaks push-to-main validation runs unless adjusted). |
| .github/workflows/publish-ruby.yml | Adds wait-for-core and gates cross-gems via needs: on release/dispatch, eliminating redundant per-matrix polling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address Copilot review on #515: a job with the default `if: success()` is SKIPPED when a `needs:` dependency is skipped, so `build`'s plain `needs: wait-for-core` would suppress the push-to-main validation build (#374 patch path), where wait-for-core is release/dispatch-gated and thus skipped. Guard `build` with `if: ${{ !failure() && !cancelled() }}` so it runs when the gate SUCCEEDED (release/dispatch, core is up) or was SKIPPED (push), but not when it FAILED (core never landed in 10 min) — in which case `publish` (needs: build) is skipped too. This is safe under either GitHub skipped-dependency semantic. The ruby `cross-gems` job needs no change: it is release/dispatch-only, exactly when wait-for-core also runs, so it never faces a skipped gate. Also correct the misleading "push/PR" wording (publish-node.yml runs on push to main, not PRs). Assisted-by: Claude:claude-opus-4-8 Signed-off-by: Richard Quinn <[email protected]>
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.
Follow-up to #514 (which fixed #500). Implements the issue's "eventual clean architecture" (option 3) —
needs:-based ordering — adapted to respect the OIDC trusted-publishing constraint.What changed
#514 fixed the release race with an inline crates.io sparse-index poll inside each binding build, which runs redundantly once per matrix leg (5× in node
build, 5× in rubycross-gems). This replaces that with a singlewait-for-coregating job per binding workflow that the build job depends on vianeeds:, so a binding build doesn't start untildisarm 0.<minor>is on crates.io.publish-node.yml: addwait-for-core(release/workflow_dispatch-gated), remove the inline poll frombuild, addneeds: wait-for-coretobuild.publish-ruby.yml: same, gatingcross-gems.Why not literally merge into one workflow (option 3 as written)
publish-node.ymlandpublish-ruby.ymlare registered as OIDC Trusted Publishers keyed to their exact workflow filenames (npmjs.org / rubygems.org), and PyPI/RubyGems also key on thepypi/rubygemsenvironments. Merging the three publishers into one file would silently break trusted publishing until each registry's Trusted Publisher is reconfigured out-of-band. So theneeds:dependency is scoped within each existing workflow — triggers, filenames, and environments are all unchanged.workflow_runchaining (option 2) was also rejected: it flipsgithub.event_nametoworkflow_run, breaking the ~5release/workflow_dispatchguards per file (including the #374 drift-patch gate) and forcing manual release-ref handling.Behavior on non-release events
On push/PR the
wait-for-corejob is skipped (itsif:is false). A skippedneeds:dependency does not block the dependent, so the nodebuild/ rubytestvalidation jobs still run against the #374 in-repo-patched core exactly as before.Verification
wait-for-core → build/cross-gems → publish.sedreq-parse yields0.11from both manifests; the version regex matches0.11.0/0.11.2and rejects0.10.0/0.0.0/0.111.0.wait-for-coreis skipped and the node/ruby validation jobs still run green (confirms the skipped-needs:behavior and intact ci: add PR-time build + RSpec gate for the Ruby binding (no pre-release coverage today) #374 patching).Assisted-by: Claude:claude-opus-4-8