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 <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
