You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@couchdb.apache.org by Joan Touzet <wo...@apache.org> on 2017/08/16 06:46:34 UTC

[DISCUSSION] Disallow all merges of PRs to master that cause tests to fail

Hi committers,

I'd like to propose a change to our policy on version control, namely
that no check-ins be allowed on the master branch unless CI test runs
against that PR are clean.

We've worked hard as a group to get runs clean. We need to protect
that achievement and investment in our test suite. That means not
letting rogue check-ins slip by because we are ignoring a red X in
GitHub (GH) from the Travis run.

Things I see as exceptions:
* Changes to things clearly not related to the test suite, i.e.
  documentation, support scripts, rel/overlay/etc/ files, etc.
* Changes already agreed upon in a previous PR/discussion for
  administrative tasks

Interesting situation right now for a discussion: Garren has a PR up[1]
that enables the mango tests to be part of the standard Travis/Jenkins
runs. Unfortunately, it doesn't pass on one of our platforms right now
and that needs investigation. Should we allow the PR to land and fix
the problems in master, or should the PR hold-up until it can land along
with the fixes for the failing mango tests? I can see both sides of this
argument.

It may or may not be possible for our GH setup to actually prevent such
checkins (the Apache GH setup is somewhat restricted, and various things
like commit hooks and webhooks have to be configured by INFRA, not us).

I'd like to further discuss whether people feel such a hook would be
acceptable, onerous or otherwise. Personally, I worry that such a setup
might prevent us from checking in some of the exceptions above, but if
there is a way around it, we could proceed down that path.

What do you think, sirs?[2]
Joan


[1]: https://github.com/apache/couchdb/pull/753
[2]: It's a Mystery Science Theatre 3000 Joel reference. :)

Re: [DISCUSSION] Disallow all merges of PRs to master that cause tests to fail

Posted by Nick North <no...@gmail.com>.

+1. Having put all the effort in to to get to a clean test suite, it would
be a great shame to let it slip. The effort to remain clean is much less
than the effort to get to that state in the first place: failing tests are
easier to fix when they first occur and everything is fresh in the author's
mind, than they are 6 months later. If you can't merge to master until the
tests are clean, there is a much greater incentive to take the high road
and fix problems immediately.

As a second point: if you come to an interesting Open Source project you've
never seen before, then a failing build or failing test suite is a good
reason to reject it without further thought. Cleanliness at that level is
an indication of a well-functioning and professional community, whose
software is worthy of further consideration.

Nick

On Wed, 16 Aug 2017 at 08:43 Dale Harvey <da...@arandomurl.com> wrote:

> +1, have found this useful / necessary with PouchDB over the years
>
> Describing our workflow as it may be useful (its also very similiar to
> mozillas). We have Tier 1
> supported platform, node stable, stable releases of browsers etc a checkin
> is not allowed to make
> those go red, if something lands that makes them red it gets immediately
> reverted
>
> Tier 2 systems, like CouchDB 2.0 for the period it was under development.
> We take a best effort
> at keeping it green but it doesnt prevent patches from landing. If it goes
> red it is someones job to get
> it green, if it stays red for a long period of time it becomes unsupported.
> (We are still in the process of moving
> couch2 to tier1)
>
> This allows us to stay pretty stable while not be too restrictive for
> building new features / supporting new
> platforms. The important thing however is the zero policy rule on Tier 1,
> once there is any reason for
> people to ignore failing tests they decay super quickly and it gets very
> hard to bring them back, for a while
> we had the policy of fixing things in master but there are always
> complications, backing out, fixing the tests
> and relanding is far more reliable
>
> On 16 August 2017 at 07:46, Joan Touzet <wo...@apache.org> wrote:
>
> > Hi committers,
> >
> > I'd like to propose a change to our policy on version control, namely
> > that no check-ins be allowed on the master branch unless CI test runs
> > against that PR are clean.
> >
> > We've worked hard as a group to get runs clean. We need to protect
> > that achievement and investment in our test suite. That means not
> > letting rogue check-ins slip by because we are ignoring a red X in
> > GitHub (GH) from the Travis run.
> >
> > Things I see as exceptions:
> > * Changes to things clearly not related to the test suite, i.e.
> >   documentation, support scripts, rel/overlay/etc/ files, etc.
> > * Changes already agreed upon in a previous PR/discussion for
> >   administrative tasks
> >
> > Interesting situation right now for a discussion: Garren has a PR up[1]
> > that enables the mango tests to be part of the standard Travis/Jenkins
> > runs. Unfortunately, it doesn't pass on one of our platforms right now
> > and that needs investigation. Should we allow the PR to land and fix
> > the problems in master, or should the PR hold-up until it can land along
> > with the fixes for the failing mango tests? I can see both sides of this
> > argument.
> >
> > It may or may not be possible for our GH setup to actually prevent such
> > checkins (the Apache GH setup is somewhat restricted, and various things
> > like commit hooks and webhooks have to be configured by INFRA, not us).
> >
> > I'd like to further discuss whether people feel such a hook would be
> > acceptable, onerous or otherwise. Personally, I worry that such a setup
> > might prevent us from checking in some of the exceptions above, but if
> > there is a way around it, we could proceed down that path.
> >
> > What do you think, sirs?[2]
> > Joan
> >
> >
> > [1]: https://github.com/apache/couchdb/pull/753
> > [2]: It's a Mystery Science Theatre 3000 Joel reference. :)
> >
>

Re: [DISCUSSION] Disallow all merges of PRs to master that cause tests to fail

Posted by Dale Harvey <da...@arandomurl.com>.

+1, have found this useful / necessary with PouchDB over the years

Describing our workflow as it may be useful (its also very similiar to
mozillas). We have Tier 1
supported platform, node stable, stable releases of browsers etc a checkin
is not allowed to make
those go red, if something lands that makes them red it gets immediately
reverted

Tier 2 systems, like CouchDB 2.0 for the period it was under development.
We take a best effort
at keeping it green but it doesnt prevent patches from landing. If it goes
red it is someones job to get
it green, if it stays red for a long period of time it becomes unsupported.
(We are still in the process of moving
couch2 to tier1)

This allows us to stay pretty stable while not be too restrictive for
building new features / supporting new
platforms. The important thing however is the zero policy rule on Tier 1,
once there is any reason for
people to ignore failing tests they decay super quickly and it gets very
hard to bring them back, for a while
we had the policy of fixing things in master but there are always
complications, backing out, fixing the tests
and relanding is far more reliable

On 16 August 2017 at 07:46, Joan Touzet <wo...@apache.org> wrote:

> Hi committers,
>
> I'd like to propose a change to our policy on version control, namely
> that no check-ins be allowed on the master branch unless CI test runs
> against that PR are clean.
>
> We've worked hard as a group to get runs clean. We need to protect
> that achievement and investment in our test suite. That means not
> letting rogue check-ins slip by because we are ignoring a red X in
> GitHub (GH) from the Travis run.
>
> Things I see as exceptions:
> * Changes to things clearly not related to the test suite, i.e.
>   documentation, support scripts, rel/overlay/etc/ files, etc.
> * Changes already agreed upon in a previous PR/discussion for
>   administrative tasks
>
> Interesting situation right now for a discussion: Garren has a PR up[1]
> that enables the mango tests to be part of the standard Travis/Jenkins
> runs. Unfortunately, it doesn't pass on one of our platforms right now
> and that needs investigation. Should we allow the PR to land and fix
> the problems in master, or should the PR hold-up until it can land along
> with the fixes for the failing mango tests? I can see both sides of this
> argument.
>
> It may or may not be possible for our GH setup to actually prevent such
> checkins (the Apache GH setup is somewhat restricted, and various things
> like commit hooks and webhooks have to be configured by INFRA, not us).
>
> I'd like to further discuss whether people feel such a hook would be
> acceptable, onerous or otherwise. Personally, I worry that such a setup
> might prevent us from checking in some of the exceptions above, but if
> there is a way around it, we could proceed down that path.
>
> What do you think, sirs?[2]
> Joan
>
>
> [1]: https://github.com/apache/couchdb/pull/753
> [2]: It's a Mystery Science Theatre 3000 Joel reference. :)
>

Re: [DISCUSSION] Disallow all merges of PRs to master that cause tests to fail

Posted by Robert Samuel Newson <rn...@apache.org>.

+1

> On 16 Aug 2017, at 15:55, Paul Davis <pa...@gmail.com> wrote:
> 
> On Wed, Aug 16, 2017 at 1:46 AM, Joan Touzet <wo...@apache.org> wrote:
>> Hi committers,
>> 
>> I'd like to propose a change to our policy on version control, namely
>> that no check-ins be allowed on the master branch unless CI test runs
>> against that PR are clean.
>> 
>> We've worked hard as a group to get runs clean. We need to protect
>> that achievement and investment in our test suite. That means not
>> letting rogue check-ins slip by because we are ignoring a red X in
>> GitHub (GH) from the Travis run.
>> 
>> Things I see as exceptions:
>> * Changes to things clearly not related to the test suite, i.e.
>>  documentation, support scripts, rel/overlay/etc/ files, etc.
>> * Changes already agreed upon in a previous PR/discussion for
>>  administrative tasks
>> 
> 
> I'd be +1 for requiring a clean Travis-CI run before something can be
> merged. I'm pretty sure we can configure that easily enough (via INFRA
> as you mention). However I'd make that absolutely no exceptions. I'd
> also disable commits directly to master not coming through PR so that
> it can't be bypassed. This is especially important for when merge PRs
> to dependencies we'd need to require the rebar.config.script update to
> PR'ed so it goes through CI.
> 
>> Interesting situation right now for a discussion: Garren has a PR up[1]
>> that enables the mango tests to be part of the standard Travis/Jenkins
>> runs. Unfortunately, it doesn't pass on one of our platforms right now
>> and that needs investigation. Should we allow the PR to land and fix
>> the problems in master, or should the PR hold-up until it can land along
>> with the fixes for the failing mango tests? I can see both sides of this
>> argument.
>> 
>> It may or may not be possible for our GH setup to actually prevent such
>> checkins (the Apache GH setup is somewhat restricted, and various things
>> like commit hooks and webhooks have to be configured by INFRA, not us).
>> 
>> I'd like to further discuss whether people feel such a hook would be
>> acceptable, onerous or otherwise. Personally, I worry that such a setup
>> might prevent us from checking in some of the exceptions above, but if
>> there is a way around it, we could proceed down that path.
>> 
> 
> I don't think we should entertain any gray area as it muddies the
> waters. I'd say we either make it 100% or leave it in the current
> state of "best effort" which is what I believe your current example
> falls into which is the status quo.
> 
>> What do you think, sirs?[2]
>> Joan
>> 
>> 
>> [1]: https://github.com/apache/couchdb/pull/753
>> [2]: It's a Mystery Science Theatre 3000 Joel reference. :)

Re: [DISCUSSION] Disallow all merges of PRs to master that cause tests to fail

Posted by Paul Davis <pa...@gmail.com>.

On Wed, Aug 16, 2017 at 1:46 AM, Joan Touzet <wo...@apache.org> wrote:
> Hi committers,
>
> I'd like to propose a change to our policy on version control, namely
> that no check-ins be allowed on the master branch unless CI test runs
> against that PR are clean.
>
> We've worked hard as a group to get runs clean. We need to protect
> that achievement and investment in our test suite. That means not
> letting rogue check-ins slip by because we are ignoring a red X in
> GitHub (GH) from the Travis run.
>
> Things I see as exceptions:
> * Changes to things clearly not related to the test suite, i.e.
>   documentation, support scripts, rel/overlay/etc/ files, etc.
> * Changes already agreed upon in a previous PR/discussion for
>   administrative tasks
>

I'd be +1 for requiring a clean Travis-CI run before something can be
merged. I'm pretty sure we can configure that easily enough (via INFRA
as you mention). However I'd make that absolutely no exceptions. I'd
also disable commits directly to master not coming through PR so that
it can't be bypassed. This is especially important for when merge PRs
to dependencies we'd need to require the rebar.config.script update to
PR'ed so it goes through CI.

> Interesting situation right now for a discussion: Garren has a PR up[1]
> that enables the mango tests to be part of the standard Travis/Jenkins
> runs. Unfortunately, it doesn't pass on one of our platforms right now
> and that needs investigation. Should we allow the PR to land and fix
> the problems in master, or should the PR hold-up until it can land along
> with the fixes for the failing mango tests? I can see both sides of this
> argument.
>
> It may or may not be possible for our GH setup to actually prevent such
> checkins (the Apache GH setup is somewhat restricted, and various things
> like commit hooks and webhooks have to be configured by INFRA, not us).
>
> I'd like to further discuss whether people feel such a hook would be
> acceptable, onerous or otherwise. Personally, I worry that such a setup
> might prevent us from checking in some of the exceptions above, but if
> there is a way around it, we could proceed down that path.
>

I don't think we should entertain any gray area as it muddies the
waters. I'd say we either make it 100% or leave it in the current
state of "best effort" which is what I believe your current example
falls into which is the status quo.

> What do you think, sirs?[2]
> Joan
>
>
> [1]: https://github.com/apache/couchdb/pull/753
> [2]: It's a Mystery Science Theatre 3000 Joel reference. :)

Re: [DISCUSSION] Disallow all merges of PRs to master that cause tests to fail

Posted by Andy Wenk <an...@apache.org>.

very late here but I am strongly +1. Here at sum.cumo we do not allow pushes to master directly and PR’s are only merged, when the feature branch is marked green by our CI. This improved the code quality massively and makes all developers life easier, when one is deploying to production.

All the best

Andy

-- 
Andy Wenk
Hamburg - Germany
RockIt!

GPG public key: 
http://pgp.mit.edu/pks/lookup?op=get&search=0x45D3565377F93D29



> On 17. Aug 2017, at 00:49, Joan Touzet <wo...@apache.org> wrote:
> 
> Seems there is general consensus.
> 
> Now, how do people feel about me asking Infra to make this change to the
> main repos (couchdb, couchdb-fauxton, etc.):
> 
> https://help.github.com/assets/images/help/repository/protecting-branch-loose-status.png
> 
> Specifically:
> 
>  Protect master branch (and any release branches like 2.1.x)
>  Require status checks to pass before merging
>  Require branches be up to date before merging
> 
> We can have an optional secondary discussion around enforcing:
> 
>  Require pull request reviews before merging
> 
> This would enforce our RTC model, but we *need* more active devs if this
> is going to pass. I've had to beg multiple times for many of my PRs in
> the 2.1.0 release cycle to be approved...even trivial documentation
> changes. It was very frustrating.
> 
> -Joan
> 
> ----- Original Message -----
> From: "Nick Vatamaniuc" <va...@gmail.com>
> To: dev@couchdb.apache.org
> Sent: Wednesday, 16 August, 2017 6:01:34 PM
> Subject: Re: [DISCUSSION] Disallow all merges of PRs to master that cause tests to fail
> 
> +1
> 
> On Aug 16, 2017 15:50, "Alexander Shorin" <kx...@gmail.com> wrote:
> 
>> It's strange to say something else than +1 or question the topic in any
>> way.
>> 
>> Good call, Joan!
>> --
>> ,,,^..^,,,
>> 
>> 
>> On Wed, Aug 16, 2017 at 6:46 AM, Joan Touzet <wo...@apache.org> wrote:
>>> Hi committers,
>>> 
>>> I'd like to propose a change to our policy on version control, namely
>>> that no check-ins be allowed on the master branch unless CI test runs
>>> against that PR are clean.
>>> 
>>> We've worked hard as a group to get runs clean. We need to protect
>>> that achievement and investment in our test suite. That means not
>>> letting rogue check-ins slip by because we are ignoring a red X in
>>> GitHub (GH) from the Travis run.
>>> 
>>> Things I see as exceptions:
>>> * Changes to things clearly not related to the test suite, i.e.
>>>  documentation, support scripts, rel/overlay/etc/ files, etc.
>>> * Changes already agreed upon in a previous PR/discussion for
>>>  administrative tasks
>>> 
>>> Interesting situation right now for a discussion: Garren has a PR up[1]
>>> that enables the mango tests to be part of the standard Travis/Jenkins
>>> runs. Unfortunately, it doesn't pass on one of our platforms right now
>>> and that needs investigation. Should we allow the PR to land and fix
>>> the problems in master, or should the PR hold-up until it can land along
>>> with the fixes for the failing mango tests? I can see both sides of this
>>> argument.
>>> 
>>> It may or may not be possible for our GH setup to actually prevent such
>>> checkins (the Apache GH setup is somewhat restricted, and various things
>>> like commit hooks and webhooks have to be configured by INFRA, not us).
>>> 
>>> I'd like to further discuss whether people feel such a hook would be
>>> acceptable, onerous or otherwise. Personally, I worry that such a setup
>>> might prevent us from checking in some of the exceptions above, but if
>>> there is a way around it, we could proceed down that path.
>>> 
>>> What do you think, sirs?[2]
>>> Joan
>>> 
>>> 
>>> [1]: https://github.com/apache/couchdb/pull/753
>>> [2]: It's a Mystery Science Theatre 3000 Joel reference. :)
>>