[asterisk-dev] Proposed Review board Usage policy change

[Paul Belanger]

On 12-12-13 02:08 PM, Jonathan Rose wrote:
> Russell Bryant wrote on December 13, 2012 12:35:08 PM:
>
>> My opinion is that this is overkill and makes the contribution
>> process unnecessarily complicated.
>>
>> If there is a legitimate objection or problem with a commit, it's
>> easy enough to change further or revert as appropriate.
>
> This is true, but once something has been committed it's not quite in
> the foreground as far as attention is concerned at that point. This
> sort of thing has happened in the past and caused regressions which
> went unnoticed for a while... otherwise we wouldn't really be
> considering this rule.
>
Personally, we do two things.

1. Remove the ability for people to directly commit code, and use some 
sort of CI tool, after code review.

2. Fixes go into trunk, and then backported into release branches. 
Again, CI tool can handle this once code review has been done.

>> I think the amount of time a patch needs to be up for comment should
>> be based on how invasive and/or controversial a change is. I
>> understand that not everyone has the same eyes for this sort of
>> thing, but I still don't think complicating the process is
>> worthwhile. As examples, the last two reviews I put up were simple
>> and non-controversial changes. I don't see *any* reason to
>> arbitrarily slow down getting a simple patch in like this:
>>
>> https://reviewboard.asterisk.org/r/2230/
>> https://reviewboard.asterisk.org/r/2245/
>
> Well, that's the thing.  People can't always tell how invasive their
> patches are going to be before people have looked at it. By having
> people who receive ship it wait a day (which isn't a long span of time
> by any stretch of the imagination), that gives someone familiar with
> whatever the patch is changing time to get to the issue and raise
> objections if necessary.
>
> I do agree that this practice doesn't really seem necessary for some
> patches, but to try and figure out what constitutes a non-invasive
> patch and come up with rules for each case seems to me like it would
> be a greater complication of the rules than simply saying "Got ship
> it?  Sit on that for a day please."
>
Again, this is an issue with our testing.  We only test code once it has 
been committed.. this in my eyes is wrong.  Could _should_ be tested in 
our tools, before committed and if the tests fail, the patch gets rejected.

>> 2) Take #1 and consider the process for someone with commit access as
>> well as someone without. I get the feeling that the process for
>> submitting patches as someone without commit access is confusing and
>> unclear right now because of the multiple paths patches take.
>
> https://wiki.asterisk.org/wiki/display/AST/Asterisk+Issue+Guidelines#AsteriskIssueGuidelines-PatchandCodesubmission
> We should probably update this section to make it clearer that patches
> should be submitted to the relevant issue on the issue tracker for
> people without commit access. It hasn't been a huge issue since people
> without reviewboard access don't really have any other avenue for
> submitting a patch in the first place. Once something is on reviewboard,
> it will probably stay on reviewboard regardless of differences from
> the original patch until it receives ship it, but I don't think there
> are really any rules regarding that. People with reviewboard access
> probably have a feel for when they should be updating patches on the
> issue anyway though.
>
>> 3) When a patch is on reviewboard, what "Ship It" matters? The tool
>> allows anyone to say "Ship It", but clearly just anyone shouldn't be
>> able to approve code going in.
>
> We tried to come up with an answer for this at one point in time, but
> we didn't really want to just outright say who can say ship it since
> we don't want to exclude the community from this process. I think the
> idea was that if someone abuses 'ship it' to give commit permission
> frivolously they'll be warned and if they continue to do so eventually
> they might lose review board use. At the same time though such a system
> discourages people from reviewing since it implies their reviews are
> under strict scrutiny.
>
> That's really the whole point of the 1 day rule though. It gives us
> an opportunity to put a check on ships given without entering a gray
> time between when something has received permission for being shipped
> and when they have actually already been committed and the review
> is closed.
>
Ship it should not mean merge it, they are two different thinks.  If 
with code reviews, we need to have people review code, and testing tools 
also review code.  No where do we do that now.  So if the patch sits for 
1 hour or 1 days, we have zero way to confirm the patch was tested properly.

If the goal is to _check_ 'ship its', then you need to review that 
functionality to a smaller group of people.

>> Some of this may seem obvious to people that work full-time on
>> Asterisk every day, but it's not obvious to newcomers, and it's even
>> not obvious to me since I'm not looking at Asterisk every day
>> anymore.
>
> Yeah, it's not entirely obvious. We need to get the relevant wiki
> pages up to date and in an easily digestible format. We also need
> to increase their visibility to users who would be involved in
> these processes somehow. Presentation is probably one of the biggest
> problems we have with our documentation right now unfortunately.
>

-- 
Paul Belanger | PolyBeacon, Inc.
Jabber: paul.belanger at polybeacon.com | IRC: pabelanger (Freenode)
Github: https://github.com/pabelanger | Twitter: 
https://twitter.com/pabelanger