Remove broken ci-success check #2084
Merged
Conversation
This file contains 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
Merged
|
It's so unfortunate that this mechanism fails in this way, it was so handy 😭 |
|
Ah, it's hasn't re-merged the merge commit with the fixed main. So I'll have to manually rebase. |
Previously, Zebra made ci-success a required check for merges to main. And then we made ci-success depend on a bunch of other CI checks. But this doesn't work as expected, because if the dependent checks fail, ci-success is skipped, and the branch protection rules allow the branch to be merged to main.
teor2345
force-pushed
the
remove-ci-success
branch
from
63389d5 to
e50a1df
Compare
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.
Motivation
Previously, Zebra made ci-success a required check for merges to main. And then we made ci-success depend on a bunch of other CI checks.
But this doesn't work as expected, because if the dependent checks fail, ci-success is skipped, and the branch protection rules allow the branch to be merged to main.
As a side-effect, CI will be slightly faster, because the number of jobs is reduced by 1. (Even though it is a short job, it still needs an extra VM.)
Solution
In this PR: delete the broken ci-success check
In the GitHub admin interface: make the branch protection rule depend directly on the 5 CI checks we intended to require
I also opened a GitHub help issue, to get them to update their documentation to includes skipped tasks.
Review
@dconnolly can review this PR as a routine cleanup.
Related Issues
We discovered this issue because #2075 unexpectedly merged with broken tests.