On Thu, Dec 13, 2012 at 11:58 AM, Mark Michelson <span dir="ltr"><<a href="mailto:mmichelson@digium.com" target="_blank">mmichelson@digium.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
At Astricon in October, one policy that came up was the idea to give everyone around the world a fair shot at reading the content on the review board. As such, I am proposing the following to be added to the "Reviewboard Usage" wiki page:<br>
<br>
"When someone approves of a review request, they will click the "Ship it!" button as a way of signing off on the review. When you have received a "Ship it!" you can feel confident that in the opinion of one person, your code has met approval.<br>
<br>
Since Asterisk is a global project, it is important that people around the world get a fair chance to review your code before the review is closed. If your code receives a "Ship it!" but has not been on the review board for at least 24 hours, please refrain from committing. It may be that a reviewer on the opposite side of the planet has a valid objection to your code but has not had the chance to express himself/herself. If the code has been up for at least 24 hours, it is reasonable to assume that the code has been given a fair chance to be reviewed by anyone interested. Ideally, you should give another 24 hours after the initial "Ship it!" so that people can have one last opportunity to bring up any objections.<br>
<br>
Exceptions to this policy may be enacted if the fix is time-sensitive, such as if the fix is determined to be security-related or if the fix is for a widely-felt catastrophic regression."<br>
<br>
<br>
Please let me know what you think.<br></blockquote><div><br></div><div>-1</div><div><br></div><div>My opinion is that this is overkill and makes the contribution process unnecessarily complicated.</div><div><br></div><div>
If there is a legitimate objection or problem with a commit, it's easy enough to change further or revert as appropriate.</div><div><br></div><div>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:</div>
<div><br></div><div> <a href="https://reviewboard.asterisk.org/r/2230/">https://reviewboard.asterisk.org/r/2230/</a></div><div> <a href="https://reviewboard.asterisk.org/r/2245/">https://reviewboard.asterisk.org/r/2245/</a></div>
<div><br></div><div>Further, if you want to talk about patch process, I think there are much more important and more fundamental patch process things that should be documented/decided before discussing such a detail. Examples:</div>
<div><br></div><div>1) Which patches must go through reviewboard vs JIRA vs both vs nothing (straight to commit)?</div><div><br></div><div>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.</div>
<div><br></div><div>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.</div><div>
<br></div><div>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.</div>
<div><br></div><div>-- </div><div>Russell Bryant</div></div></div>