Repository navigation
ci: standardize success()/failure()/cancelled() idioms in ci.yml - #1071
Draft
kbattocchi wants to merge 1 commit into
Draft
kbattocchi wants to merge 1 commit into
kbattocchi wants to merge 1 commit into
Conversation
kbattocchi
force-pushed
the
kebatt/fix-merge-lkg-ci-condition
branch
from
October 7, 2026 17:51
43ee713 to
ffcc183
Compare
kbattocchi
force-pushed
the
kebatt/fix-merge-lkg-ci-condition
branch
3 times, most recently
from
October 15, 2026 19:54
190730b to
9a37f95
Compare
Member
Author
|
Reopening to retrigger CI (previous push's pull_request event appears to have been dropped due to concurrency-group contention). |
kbattocchi
force-pushed
the
kebatt/fix-merge-lkg-ci-condition
branch
from
October 16, 2026 14:34
9a37f95 to
28124d3
Compare
Jobs that should keep going after an upstream failure or skip now use a
single job-level pattern that names each direct dependency:
if: ${{ !cancelled() && (needs.a.result != 'skipped' || ...) }}
This replaces `success() || failure()` and job-level `needs.*.result`
checks, both of which also consider transitive ancestors (see
actions/runner#1540). That caused merge-lkg to run (and fail) on PRs
and on re-runs even though its only dependency was skipped. Since the
decision lives entirely in the job's `if:`, no per-step conditions are
needed. generate-lkg also regains `!cancelled()`, so nightly LKG
generation is no longer skipped after an upstream test failure.
The recurring patterns and their rationale are documented at the top of
the file.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Keith Battocchi <kebatt@microsoft.com>
kbattocchi
force-pushed
the
kebatt/fix-merge-lkg-ci-condition
branch
from
October 19, 2026 17:42
717e1a6 to
1eb91c6
Compare
kbattocchi
added this pull request to stack #1075
October 19, 2026 17:53
This branch has not been deployed
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.
Problem
merge-lkgwas incorrectly running (and failing) when an upstream test matrix entry failed (see, e.g., this run for PR #1067), even though its direct dependencygenerate-lkgwas correctly skipped (LKG regeneration only runs onschedule/workflow_dispatch, not PRs).Root cause: job-level
success()/failure()walk the entire transitive ancestor graph of a job, not just its directneeds. A failure several jobs upstream can leak through aneedsjob that was itself cleanly skipped.What changed
merge-lkgto gate on its direct dependency's result instead of the ad hocsuccess() || failure().success()/failure()/always()/cancelled()usage inci.ymland standardized on two documented idioms:!cancelled() && (contains(needs.*.result, 'success') || contains(needs.*.result, 'failure'))— run if at least one direct dependency actually ran, regardless of transitive ancestors. This is an existence check, not an "all succeeded" check (documented explicitly in the file).always() && contains(fromJSON('["success", "failure"]'), steps.<id>.outcome)— run cleanup steps regardless of outcome, but only if the referenced step actually executed.coverage-reportcould be silently skipped on notebooks-only PRs, becausesuccess()/failure()can't distinguish "a sibling dependency was skipped" from "everything failed".(success()||failure())check onstore-reqs-per-env, which has noneeds:for it to evaluate.No behavioral changes are intended for the common case (PRs where everything upstream succeeds); this only changes behavior when something upstream is skipped or fails.