Hi, Thanks for the proposal. I have similar concerns as Kurt.
If we enforced such rule I would be afraid that everybody would be waiting for tests on his PR to complete, racing others committers to be “the first guy that clicks the merge button”, then forcing all of the others to rebase manually and race again. For example it wouldn’t be possible to push a final version of the PR, wait for the tests to complete overnight and merge it next day. Unless we would allow for merging without green travis after a final rebase, but that for me would be almost exactly what we have now. Is this a big issue in the first place? I don’t feel it that way, but maybe I’m working in not very contested parts of the code? If it’s an issue, I would suggest to go for the merging bot, that would have a queue of PRs to be: 1. Automatically rebased on the latest master 2. If no conflicts in 1., run the tests 3. If no test failures merge Piotrek > On 30 Aug 2019, at 09:38, Till Rohrmann <trohrm...@apache.org> wrote: > > Hi Tison, > > thanks for starting this discussion. In general, I'm in favour of > automations which remove human mistakes out of the equation. > > Do you know how these status checks work concretely? Will Github reject > commits for which there is no passed Travis run? How would hotfix commits > being distinguished from PR commits for which a Travis run should exist? So > I guess my question is how would enabling the status checks change how > committers interact with the Github repository? > > Cheers, > Till > > On Fri, Aug 30, 2019 at 4:46 AM Zili Chen <wander4...@gmail.com> wrote: > >> Hi Kurt, >> >> Thanks for your reply! >> >> I find two concerns about the downside from your email. Correct >> me if I misunderstanding. >> >> 1. Rebase times. Typically commits are independent one another, rebase >> just fast-forward changes so that contributors rarely resolve conflicts >> by himself. Reviews doesn't get blocked by this force rebase if there is >> a green travis report ever -- just require contributor rebase and test >> again, which generally doesn't involve changes(unless resolve conflicts). >> Contributor rebases his pull request when he has spare time or is required >> by reviewer/before getting merged. This should not inflict too much works. >> >> 2. Testing time. It is a separated topic that discussed in this thread[1]. >> I don't think we finally live with a long testing time, so it won't be a >> problem then we trigger multiple tests. >> >> Simply sum up, for trivial cases, works are trivial and it >> prevents accidentally >> failures; for complicated cases, it already requires rebase and fully >> tests. >> >> Best, >> tison. >> >> [1] >> >> https://lists.apache.org/x/thread.html/b90aa518fcabce94f8e1de4132f46120fae613db6e95a2705f1bd1ea@%3Cdev.flink.apache.org%3E >> >> >> Kurt Young <ykt...@gmail.com> 于2019年8月30日周五 上午9:15写道： >> >>> 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 >>>> >>> >>
