? ?
avatar mbabker
mbabker
19 Oct 2016

Using GitHub's reviews feature, I propose we require an approved review before a pull request is merged. I also propose that this review come from someone who was not one of the minimum two testers to get the PR to RTC nor the person who merges the PR (who should also be doing their own test/review). This in essence mandates at least four people have looked at most PRs and have performed a proper code review and tested its functionality.

avatar mbabker mbabker - open - 19 Oct 2016
avatar andrepereiradasilva
andrepereiradasilva - comment - 19 Oct 2016

+1.

but i also think joomla really needs more people testing and reviewing things in the CMS.

There are a huge number of working groups, and i understand there other areas (not cms related) to consider in this working groups, but the cms should never be putted in sidelines, is really the core of joomla, without it, there is almost nothing.
So, imho, people involved in joomla should really dedicate more time testing and reviewing the cms issues and code.

avatar mbabker
mbabker - comment - 19 Oct 2016

+1 on that.

https://volunteers.joomla.org/leadership/production-leadership-team indicates that of the highest level production team, one is actively engaged with the full PR process and two others participate semi-regularly. This is somewhat concerning as it seems to indicate less than half the leadership is actively engaged in what should be one of their most important tasks.

https://volunteers.joomla.org/teams/cms-maintenance-team adds six additional members to that list who if I'm not mistaken are charged with the maintenance of this repo (to include pull request review and merges).

There are at least 16 people between those two teams who should be involved at the highest levels and it seems like there is about a 50% participation rate. I'd suggest on that alone there's a top down issue.

avatar brianteeman
brianteeman - comment - 19 Oct 2016

Dont forget https://volunteers.joomla.org/teams/cms-release-team

On 19 October 2016 at 15:30, Michael Babker notifications@github.com
wrote:

+1 on that.

https://volunteers.joomla.org/leadership/production-leadership-team
indicates that of the highest level production team, one is actively
engaged with the full PR process and two others participate semi-regularly.
This is somewhat concerning as it seems to indicate less than half the
leadership is actively engaged in what should be one of their most
important tasks.

https://volunteers.joomla.org/teams/cms-maintenance-team adds six
additional members to that list who if I'm not mistaken are charged with
the maintenance of this repo (to include pull request review and merges).

There are at least 16 people between those two teams who should be
involved at the highest levels and it seems like there is about a 50%
participation rate. I'd suggest on that alone there's a top down issue.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#12476 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8Xwvq2R5oeH94eQsANG_sQRQea8Kks5q1ilxgaJpZM4KbBLN
.

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
https://brian.teeman.net/ http://brian.teeman.net/

avatar mbabker
mbabker - comment - 19 Oct 2016

This incarnation of it is still relatively new and 3.6.3 was their first release, so I purposefully left them out for the moment since the role is still being grown into. The others I mentioned though are well established.

avatar dgt41
dgt41 - comment - 19 Oct 2016

I've suggested this before, but I will do it also publicly, invite @andrepereiradasilva to cms maintenance team!

avatar zero-24 zero-24 - change - 19 Oct 2016
Labels Added: ?
avatar brianteeman brianteeman - change - 19 Oct 2016
Status New Discussion
avatar brianteeman brianteeman - change - 19 Oct 2016
Labels Added: ?
avatar Stevec4
Stevec4 - comment - 20 Oct 2016

There has to be better communication in order to get people testing. From what I have seen that is going to be the difficult part.

If people don't know what's going on how can they help.

avatar xtech86
xtech86 - comment - 20 Oct 2016

+1

Also I didn't test the last beta/rc releases too much because I couldn't easily see what had been changed? If with the releases we can have a list of changes / additions we can then test more accurately, opposed to trawling through loads of closed commits.

avatar andrepereiradasilva
andrepereiradasilva - comment - 20 Oct 2016

since all PR have a milestone, github gives that info. what was changed in 3.6.3 https://github.com/joomla/joomla-cms/milestone/16?closed=1

For other previous versions see also https://github.com/joomla/joomla-cms/milestones?state=closed

for more simple information you would need some volunteer going through all pull requests and writing in a more simple way what changed ...

avatar rdeutz
rdeutz - comment - 21 Nov 2016

+1

The easiest way to do it is that the person who set the RTC flag isn't a tester and the author of the PR and isn't allowed to merge the PR.

Done :-)

But anything fails when the persons at the end of the process not put some time in the review.

avatar brianteeman
brianteeman - comment - 21 Nov 2016

The problem with it relying on the person who set the issue RTC is that
people already complain when its not set RTC within minutes of the 2nd
test. If the person setting it to be RTC also has to do a review its going
to be worse (especially speaking personally I would not be able to do the
review so its going to have to wait for someone else.)

On 21 November 2016 at 20:08, Robert Deutz notifications@github.com wrote:

+1

The easiest way to do it is that the person who set the RTC flag isn't a
tester and the author of the PR and isn't allowed to merge the PR.

Done :-)

But anything fails when the persons at the end of the process not put some
time in the review.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#12476 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8WugiLq15RR5s_F4a9Il0BZOuTy5ks5rAfohgaJpZM4KbBLN
.

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
https://brian.teeman.net/ http://brian.teeman.net/

avatar rdeutz
rdeutz - comment - 21 Nov 2016

The problem with it relying on the person who set the issue RTC is that
people already complain when its not set RTC within minutes of the 2nd
test.

We need to change that behauviour, we all should aim for quality and not merges per hour.

avatar andrepereiradasilva
andrepereiradasilva - comment - 21 Nov 2016

what we need is more people testing/reviewing ... JBS?

avatar brianteeman
brianteeman - comment - 21 Nov 2016

I am talking about it being marked RTC not about it being merged

On 21 November 2016 at 20:18, Robert Deutz notifications@github.com wrote:

The problem with it relying on the person who set the issue RTC is that
people already complain when its not set RTC within minutes of the 2nd
test.

We need to change that behauviour, we all should aim for quality and not
merges per hour.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#12476 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8VQdt4zH4BkS_wcKtmR1PVpd1Usfks5rAfypgaJpZM4KbBLN
.

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
https://brian.teeman.net/ http://brian.teeman.net/

avatar andrepereiradasilva
andrepereiradasilva - comment - 21 Nov 2016

it could be set RTC with two tests and them require a review to be merged

avatar mbabker
mbabker - comment - 21 Nov 2016

We aren't automatically marking items as RTC though based on the number of successful tests or pull request reviews (and last I looked pull request reviews weren't easily distinguishable through GitHub's API). It still requires a human to do it (as it should). I think that's where some of the misunderstanding/groaning comes from.

avatar rdeutz
rdeutz - comment - 21 Nov 2016

@andrepereiradasilva I only merge what I understand and review (almost) all PR, that should be the default

avatar zero-24
zero-24 - comment - 21 Nov 2016

RTC means ready to commit so if we add more steps between we first need the review than RTC.

avatar brianteeman
brianteeman - comment - 21 Nov 2016

Robert you are confusing merging and setting something RTC

On 21 November 2016 at 20:22, Robert Deutz notifications@github.com wrote:

@andrepereiradasilva https://github.com/andrepereiradasilva I only
merge what I understand and review (almost) all PR, that should be the
default


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#12476 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8evj9EZjenB0_8G4GxungPWX9RYOks5rAf2igaJpZM4KbBLN
.

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
https://brian.teeman.net/ http://brian.teeman.net/

avatar andrepereiradasilva
andrepereiradasilva - comment - 21 Nov 2016

@rdeutz when i said review i was talking about a github code review by someone, not counting on merger review

avatar rdeutz
rdeutz - comment - 21 Nov 2016

No I am aware about the difference, but both is part of the process. Set to RTC needs some kind of review and merge also. The proposal here suggest to make an additional review, and I am saying let us combine this with the setting to RTC. So setting to RTC will require a more deep review as it does today.

The review feature from github seems to me good for minor code changes but not for a functional review to find out if the patch does the right thing in the right way.

I would say 70% of the PR are so small changes or just language changes so pretty much anybody involved can do the RTC review and even the merge. If we all only merge/set RTC stuff we 100% understand then we are making a good step in the right direction.

avatar brianteeman
brianteeman - comment - 21 Nov 2016

I guess I am going to have to step back from marking anything RTC. If I
100% understand it I will almost certainly be one of the people who was
tested it so quite correctly dont mark it RTC but where I have 90%
understood it and trusted the experience of the people doing the tests I
have in the past marked it RTC.

On 21 November 2016 at 20:34, Robert Deutz notifications@github.com wrote:

No I am aware about the difference, but both is part of the process. Set
to RTC needs some kind of review and merge also. The proposal here suggest
to make an additional review, and I am saying let us combine this with the
setting to RTC. So setting to RTC will require a more deep review as it
does today.

The review feature from github seems to me good for minor code changes but
not for a functional review to find out if the patch does the right thing
in the right way.

I would say 70% of the PR are so small changes or just language changes so
pretty much anybody involved can do the RTC review and even the merge. If
we all only merge/set RTC stuff we 100% understand then we are making a
good step in the right direction.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#12476 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8a9WXLofOB2nbB00ugwJYK_Tof-nks5rAgBYgaJpZM4KbBLN
.

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
https://brian.teeman.net/ http://brian.teeman.net/

avatar Bakual
Bakual - comment - 21 Nov 2016

The GitHub "review" feature needs to be independant from the RTC label we set. Otherwise one of the is useless if it is supposed to be RTC after the review.
I think that new feature is helpful since anyone can do such a review and approve it. Doens't even have to be a maintainer.
I wouldn't make it a requirement for all PRs because I also think there should be some basic review before merging. But for bigger PRs, it could be useful and should be required that someone does some in deep review of the code. Not sure how to handle it though.

avatar mbabker
mbabker - comment - 11 Dec 2016

Is there anything actionable here or can this now be closed?

avatar rdeutz
rdeutz - comment - 11 Dec 2016

maybe not the best time to make a process change, +1 for closing

avatar mbabker mbabker - change - 11 Dec 2016
Status Discussion Closed
Closed_Date 0000-00-00 00:00:00 2016-12-11 21:36:42
Closed_By mbabker
avatar mbabker mbabker - close - 11 Dec 2016

Add a Comment

Login with GitHub to post a comment