Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ci] invert the condition on all-jobs-succeeded #457

Closed
wants to merge 1 commit into from

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