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.
+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.
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/
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.
I've suggested this before, but I will do it also publicly, invite @andrepereiradasilva to cms maintenance team!
Labels |
Added:
?
|
Status | New | ⇒ | Discussion |
Labels |
Added:
?
|
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.
+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.
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 ...
+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.
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/
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.
what we need is more people testing/reviewing ... JBS?
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/
it could be set RTC with two tests and them require a review to be merged
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.
@andrepereiradasilva I only merge what I understand and review (almost) all PR, that should be the default
RTC means ready to commit so if we add more steps between we first need the review than RTC.
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/
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.
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/
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.
Is there anything actionable here or can this now be closed?
maybe not the best time to make a process change, +1 for closing
Status | Discussion | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2016-12-11 21:36:42 |
Closed_By | ⇒ | mbabker |
+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.