Skip to content

Conversation

@jswrenn
Copy link
Collaborator

@jswrenn jswrenn commented Oct 4, 2023

Previously, a failing job could cause all-jobs-succeeded to be skipped, and branch protections interpreted a skipped job as being vacuously successful. This failure mode caused #454 to be merged to main, despite failing many CI checks.

This PR inverts the if condition of this job: It only runs on failure, and upon failure, it fails.

joshlf
joshlf previously approved these changes Oct 4, 2023
@jswrenn jswrenn force-pushed the task-failed-successfully branch from 5aa913e to 254be7b Compare October 4, 2023 00:42
@jswrenn jswrenn enabled auto-merge October 4, 2023 00:51
@jswrenn jswrenn force-pushed the task-failed-successfully branch from 254be7b to 2f54837 Compare October 4, 2023 00:59
Previously, a failing job could cause all-jobs-succeed to be
skipped, and branch protections interpreted a skipped job as
being vacuously successful. This failure mode caused #454 to be
merged to main, despite failing many CI checks.

This PR inverts the `if` condition of this job: It only runs on
failure, and upon failure, it fails.
@jswrenn jswrenn force-pushed the task-failed-successfully branch from 2f54837 to cddaacb Compare October 4, 2023 01:02
@joshlf
Copy link
Member

joshlf commented Oct 4, 2023

Looks like success() isn't available in the context it's used here. I put up a slight modification of this idea in #460.

@joshlf
Copy link
Member

joshlf commented Oct 4, 2023

Confirmed in #461 (comment) that #460 fixed the issue.

@joshlf joshlf closed this Oct 4, 2023
auto-merge was automatically disabled October 4, 2023 15:54

Pull request was closed

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