[asterisk-dev] Proposed Review board Usage policy change

[Jonathan Rose]

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.

> 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."

> 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.

> 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.

--
Jonathan R. Rose
Digium, Inc. | Software Engineer
445 Jan Davis Drive NW - Huntsville, AL 35806 - US
direct +1 256 428 6139