You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flink.apache.org by Zili Chen <wa...@gmail.com> on 2019/08/29 15:06:27 UTC
[PROPOSAL] Force rebase on master before merge
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
Re: [PROPOSAL] Force rebase on master before merge
Posted by Zili Chen <wa...@gmail.com>.
Hi all,
Thanks for your replies.
For Till's question, as Chesnay said if we cannot attach travis checks
via CIBot workflow the mechanism provided by GitHub doesn't work at all,
which states "This setting will not take effect unless at least one
status check is enabled".
Technically we can involve this up-to-date checker in CIBot workflow.
However,
Given the status that our project is currently under quite active
development
and it takes too long to run an extra, almost no implicit conflict build
pass,
I agree that it is not our case to enforce such rules.
Best,
tison.
Chesnay Schepler <ch...@apache.org> 于2019年8月30日周五 下午4:38写道：
> I think this is a non-issue; every committer I know checks beforehand if
> the build passes.
>
> Piotr has provided good arguments for why this approach isn't practical.
> Additionally, there are simply technical limitations that prevent this
> from working as expected.
>
> a) we cannot attach Travis checks via CiBot due to lack of permissions
> b) It is not possible AFAIK to force a PR to be up-to-date with current
> master when Travis runs. In other words, I can open a PR, travis passes,
> and so long as no new merge conflicts arise I could _still_ merge it 2
> months later.
>
> On 30/08/2019 10:34, Piotr Nowojski wrote:
> > 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 <tr...@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 <wa...@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 <yk...@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 <wa...@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
> >
>
>
Re: [PROPOSAL] Force rebase on master before merge
Posted by Chesnay Schepler <ch...@apache.org>.
I think this is a non-issue; every committer I know checks beforehand if
the build passes.
Piotr has provided good arguments for why this approach isn't practical.
Additionally, there are simply technical limitations that prevent this
from working as expected.
a) we cannot attach Travis checks via CiBot due to lack of permissions
b) It is not possible AFAIK to force a PR to be up-to-date with current
master when Travis runs. In other words, I can open a PR, travis passes,
and so long as no new merge conflicts arise I could _still_ merge it 2
months later.
On 30/08/2019 10:34, Piotr Nowojski wrote:
> 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 <tr...@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 <wa...@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 <yk...@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 <wa...@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
>
Re: [PROPOSAL] Force rebase on master before merge
Posted by Piotr Nowojski <pi...@ververica.com>.
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 <tr...@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 <wa...@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 <yk...@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 <wa...@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
>>>>
>>>
>>
Re: [PROPOSAL] Force rebase on master before merge
Posted by Till Rohrmann <tr...@apache.org>.
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 <wa...@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 <yk...@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 <wa...@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
> > >
> >
>
Re: [PROPOSAL] Force rebase on master before merge
Posted by Zili Chen <wa...@gmail.com>.
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 <yk...@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 <wa...@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
> >
>
Re: [PROPOSAL] Force rebase on master before merge
Posted by Kurt Young <yk...@gmail.com>.
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 <wa...@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
>