Hi Zili, Thanks for the proposal, I had similar confusion in the past with your point #2. Force rebase to master before merging can solve some problems, but it also introduces new problem. Given the CI testing time is quite long (couple of hours) now, it's highly possible that before your test which triggered by rebasing finishes, the master will get some more new commits. This situation will get worse if more people are doing this. One possible solution is let the committer decide what should do before he/she merges it. If it's a trivial issue, just merge it if travis passes is fine. But if it's a rather big one, and some related codes just got merged in to master, I will choose to rebase to master and push it to my own repo to trigger my personal CI test on it because this can guarantee the testing time.
To summarize: I think this should be decided by the committer who is merging the PR, but not be forced. Best, Kurt On Thu, Aug 29, 2019 at 11:07 PM Zili Chen <wander4...@gmail.com> wrote: > Hi devs, > > GitHub provides a mechanism which is able to require branches to be > up to date before merged[1](point 6). I can see several advantages > enabling it. Thus propose our project to turn on this switch. Below are > my concerns. Looking forward to your insights. > > 1. Avoid CI failures in pr which fixed by another commit. We now merge a > pull request even if CI fails but the failures knowns as flaky tests. > We doesn't resolve this by turn on the switch but it helps to find any > other potential valid failures. > > 2. Avoid CI failures in master after pull request merged. Actually, CI > tests the branch that pull request bind exactly. Even if it gave green > it is still possible a systematic failure introduced because conflicts > with another new commit merged in master but not merged in this branch. > > For the downside, it might require contributors rebase his pull requests > some times before getting merged. But it should not inflict too much > works. > > Best, > tison. > > [1] https://help.github.com/en/articles/enabling-required-status-checks >
