master branch merges must pass unit tests

master branch merges must pass unit tests

[Sage Weil]

I enabled the github check that the unit tests pass in order to merge to 
master.  These tests still aren't completely reliable, but they're close, 
and we'll make better progress if we start enforcing it now.

Note that core developers can still override the check to merge if 
it's necessary, but I encourage you to avoid doing so!

sage

Re: master branch merges must pass unit tests

[Patrick Donnelly]

On Tue, Nov 8, 2016 at 4:49 PM, Sage Weil <sweil@redhat.com> wrote:
> I enabled the github check that the unit tests pass in order to merge to
> master.  These tests still aren't completely reliable, but they're close,
> and we'll make better progress if we start enforcing it now.
>
> Note that core developers can still override the check to merge if
> it's necessary, but I encourage you to avoid doing so!

Does this effect manual merges?

-- 
Patrick Donnelly

Re: master branch merges must pass unit tests

[Ken Dreyer]

On Tue, Nov 8, 2016 at 2:56 PM, Patrick Donnelly <pdonnell@redhat.com> wrote:
> On Tue, Nov 8, 2016 at 4:49 PM, Sage Weil <sweil@redhat.com> wrote:
>> I enabled the github check that the unit tests pass in order to merge to
>> master.  These tests still aren't completely reliable, but they're close,
>> and we'll make better progress if we start enforcing it now.
>>
>> Note that core developers can still override the check to merge if
>> it's necessary, but I encourage you to avoid doing so!
>
> Does this effect manual merges?

From what I've seen in GitHub, it is in force for both scenarios: the
web UI as well as "git push origin master"

- Ken

Re: master branch merges must pass unit tests

[Sage Weil]

On Tue, 8 Nov 2016, Sage Weil wrote:
> I enabled the github check that the unit tests pass in order to merge to 
> master.  These tests still aren't completely reliable, but they're close, 
> and we'll make better progress if we start enforcing it now.

I went a bit further and also checked the box requiring a review and 
preventing pushes directly to master branch.  These are already uneforced 
requirements so this shouldn't slow people down *except* that we need to 
start using the new github 'review' feature that lets you explicitly 
approve changes.

Again, the core developers can override these restrictions if necessary.

I haven't done anything to the stable branches, but we might want to do 
the same thing there...

sage

Re: master branch merges must pass unit tests

[Loic Dachary]

On 09/11/2016 00:34, Sage Weil wrote:
> On Tue, 8 Nov 2016, Sage Weil wrote:
>> I enabled the github check that the unit tests pass in order to merge to 
>> master.  These tests still aren't completely reliable, but they're close, 
>> and we'll make better progress if we start enforcing it now.
> 
> I went a bit further and also checked the box requiring a review and 
> preventing pushes directly to master branch.  These are already uneforced 
> requirements so this shouldn't slow people down *except* that we need to 
> start using the new github 'review' feature that lets you explicitly 
> approve changes.
> 
> Again, the core developers can override these restrictions if necessary.
> 
> I haven't done anything to the stable branches, but we might want to do 
> the same thing there...

Good move :-) I will do the same for the stable branches.

Cheers

-- 
Loïc Dachary, Artisan Logiciel Libre

Re: master branch merges must pass unit tests

[Igor Fedotov]

Sage,

looks like you made even stronger restrictions.

I'm unable to merge https://github.com/ceph/ceph/pull/11823 after 
approving the changes.

Thanks,
Igor

On 11/9/2016 2:34 AM, Sage Weil wrote:
> On Tue, 8 Nov 2016, Sage Weil wrote:
>> I enabled the github check that the unit tests pass in order to merge to
>> master.  These tests still aren't completely reliable, but they're close,
>> and we'll make better progress if we start enforcing it now.
> I went a bit further and also checked the box requiring a review and
> preventing pushes directly to master branch.  These are already uneforced
> requirements so this shouldn't slow people down *except* that we need to
> start using the new github 'review' feature that lets you explicitly
> approve changes.
>
> Again, the core developers can override these restrictions if necessary.
>
> I haven't done anything to the stable branches, but we might want to do
> the same thing there...
>
> sage
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: master branch merges must pass unit tests

[Sage Weil]

On Wed, 9 Nov 2016, Igor Fedotov wrote:
> Sage,
> 
> looks like you made even stronger restrictions.
> 
> I'm unable to merge https://github.com/ceph/ceph/pull/11823 after approving
> the changes.

That's strange.  I merged this one.. next time it happens, can you do a 
screen cap and we can figure out why it's doing it?

sage

Re: master branch merges must pass unit tests

[Patrick Donnelly]

On Tue, Nov 8, 2016 at 6:34 PM, Sage Weil <sage@newdream.net> wrote:
> On Tue, 8 Nov 2016, Sage Weil wrote:
>> I enabled the github check that the unit tests pass in order to merge to
>> master.  These tests still aren't completely reliable, but they're close,
>> and we'll make better progress if we start enforcing it now.
>
> I went a bit further and also checked the box requiring a review and
> preventing pushes directly to master branch.  These are already uneforced
> requirements so this shouldn't slow people down *except* that we need to
> start using the new github 'review' feature that lets you explicitly
> approve changes.
>
> Again, the core developers can override these restrictions if necessary.
>
> I haven't done anything to the stable branches, but we might want to do
> the same thing there...

I went ahead and tried to merge a simple PR to see what would happen:

https://github.com/ceph/ceph/pull/11904

Before review:

$ git push upstream master
Counting objects: 9, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (2/2), done.
Writing objects: 100% (9/9), 1.57 KiB | 0 bytes/s, done.
Total 9 (delta 7), reused 8 (delta 7)
remote: Resolving deltas: 100% (7/7), completed with 7 local objects.
remote: error: GH006: Protected branch update failed for refs/heads/master.
remote: error: Required status check "default" is failing. At least
one approved review is required
To github.com:ceph/ceph.git
 ! [remote rejected] master -> master (protected branch hook declined)
error: failed to push some refs to 'git@github.com:ceph/ceph.git'

Not unexpected, then I did a review.

$ git push upstream master --force
Counting objects: 9, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (2/2), done.
Writing objects: 100% (9/9), 1.57 KiB | 0 bytes/s, done.
Total 9 (delta 7), reused 8 (delta 7)
remote: Resolving deltas: 100% (7/7), completed with 7 local objects.
remote: error: GH006: Protected branch update failed for refs/heads/master.
remote: error: Required status check "default" is failing.
To github.com:ceph/ceph.git
 ! [remote rejected] master -> master (protected branch hook declined)
error: failed to push some refs to 'git@github.com:ceph/ceph.git'

(With and without --force, same result.)

My understanding is that only users/teams with admin writes (or
organization owners) on ceph/ceph can push in this situation. Is that
what we want? I don't think we have anyone with admin on ceph/ceph but
maybe I somehow can't see those users/teams.

-- 
Patrick Donnelly

Re: master branch merges must pass unit tests

[Patrick Donnelly]

On Thu, Nov 10, 2016 at 9:14 PM, Patrick Donnelly <pdonnell@redhat.com> wrote:
> My understanding is that only users/teams with admin writes (or

s/writes/rights/

> I don't think we have anyone with admin on ceph/ceph but
> maybe I somehow can't see those users/teams.

I see the organization just has several owners which includes team
leads. Makes sense to me.

-- 
Patrick Donnelly

Re: master branch merges must pass unit tests

[Alfredo Deza]

On Tue, Nov 8, 2016 at 4:49 PM, Sage Weil <sweil@redhat.com> wrote:
> I enabled the github check that the unit tests pass in order to merge to
> master.

Thank you!

> These tests still aren't completely reliable, but they're close,
> and we'll make better progress if we start enforcing it now.
>

I would like to see some effort put into make it more reliable. One of
the things that it has to do is reboot the box where it runs
to ensure that processes are really killed. I would love to see some
effort put into the targets to make them robust.

There is nothing too complicated going on at the CI level, we just
call run-make-check.sh:

https://github.com/ceph/ceph-build/blob/master/ceph-pull-requests/config/definitions/ceph-pull-requests.yml#L49

> Note that core developers can still override the check to merge if
> it's necessary, but I encourage you to avoid doing so!
>
> sage
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html