search

zcash / librustzcash

Rust-language assets for Zcash
Other
339 stars 252 forks source link

Ensure that only tested merges to the default branch can happen #509

Open daira opened 2 years ago

daira commented 2 years ago

If there are multiple PRs that semantically conflict (in a way that causes build/test failures), but do not textually conflict, then with the current CI system it is possible for the default branch to get into a state that fails build/test. This can happen if both PRs had been tested, but one is merged and then the other is not re-tested for a merge against the new master.

This is a regression because it could not happen with homu. Fixing it does not preclude running CI immediately on changes and then merging immediately provided it has run; the requirement is just that the cache of the "passed" state is associated with a particular commit on the default branch, and is invalidated if the default branch changes.

"There are two hard problems in computer science: naming and cache invalidation." ― Phil Karlton

str4d commented 2 years ago

The specific case we saw was the merge of zcash/orchard#217. The last commit to that PR passed CI, because the version of main it was tested against was compatible. But between then and the PR being merged, another PR was merged that added an additional usage of a trait method removed in pasta_curves 0.3 (which zcash/orchard#217 was migrating to). In most cases this was visible as code merge conflicts in the imports, but in this case there was no code conflict, and GitHub reported the PR as fine to merge.

str4d commented 2 years ago

We don't want to run CI on all open PRs whenever there is a change to the main (currently master here) branch, because that leads to a thundering herd problem: we'll likely go over our free GitHub Actions quota.

Instead, what I propose is that we add a new CI workflow (that is required to merge), which does run on any changes to master (as well as any change to a PR), but all it does is check whether the PR has had CI run on it against the current master. Basically what that means is that when a PR merges to master, every other open PR will have a status check turn red, until either that PR is amended (pushing a commit, rebasing etc.) in order to trigger an automatic CI run, or has CI manually rerun on it.