mxnet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gautam <>
Subject Re: Protected master needs to be turned off
Date Mon, 20 Nov 2017 00:07:50 GMT

Please see inline.

On Nov 19, 2017 12:51 PM, "Eric Xie" <> wrote:

Hi all,
I'm starting this thread to vote on turning off protected master. The
reasons are:

1. Since we turned on protected master pending PRs has grown from 40 to 80.
It is severely slowing down development.

     Turning protection off, will give you ability to merge the code which
has build failure. How and more importantly who will be best judge to
figure out what's is wrong ? If CI is a problem then we have people from
infra team, who are sort of 'on call" day and night trying to fix.
Including me trying to fix the slave at 2 am in night. I have seen
commiters sending PRs and if it fails without even looking at the
reason,which could be just a temporary error, they reach out to infra team
immediately. So I don't agree that we should turn off the protected branch
just for the sake of speed. We should care more on quality than speed.

2. Committers, not CI, are ultimately responsible for the code they merge.
You should only override the CI when you are very confident that CI is the
problem, not your code. If it turns out you are wrong, you should fix it
ASAP. This is the bare minimum requirement for all committers: BE

How do we make sure that this in deed happens?

I'm aware of the argument for using protected master: It make sure that
master is stable.

Well, master will be most stable if we stop adding any commits to it. But
that's not what we want is it?

No we are not saying don't add anything in master, we are just saying
please don't add bad code to the master. And yes I have seen bad code has
been merged to the master when protected branch was not enabled.

Protected master hardly adds any stability. The faulty tests that breaks
master at random got merged into master because they happened to succeed

That's not true, it filter out one of the important aspect that at least
when code was merged it completed the whole cycle of build and test. Sure
flaky test we can track down.

Junyuan Xie

  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message