<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, May 6, 2013 at 7:44 PM, Adam Gandelman <span dir="ltr"><<a href="mailto:adamg@canonical.com" target="_blank">adamg@canonical.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="im">On 05/06/2013 04:43 PM, Alan Pevec wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
2013/5/6 John Griffith <<a href="mailto:john.griffith@solidfire.com" target="_blank">john.griffith@solidfire.com</a>>:<br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
[1] <a href="https://review.openstack.org/#/c/28212/" target="_blank">https://review.openstack.org/#<u></u>/c/28212/</a><br>
</blockquote>
acked, but I'd like to see one more review<br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
[2] <a href="https://review.openstack.org/#/c/28207/" target="_blank">https://review.openstack.org/#<u></u>/c/28207/</a><br>
</blockquote>
acked, but I'd like to see one more review<br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
[3] <a href="https://review.openstack.org/#/c/28210/" target="_blank">https://review.openstack.org/#<u></u>/c/28210/</a><br>
</blockquote>
+76, -68 seems big but not that bad upon closer look and it's<br>
localized to huawei driver<br>
acked, but I'd like to see one more review<br>
</blockquote>
<br></div>
Hey Guys-<br>
<br>
I'm probably being a stickler, and I trust John's judgment on proposing the patches as-is, but I'm a bit concerned about the following:<br>
<br>
<a href="https://review.openstack.org/#/c/28207/" target="_blank">https://review.openstack.org/#<u></u>/c/28207/</a><br>
<br>
<a href="https://review.openstack.org/#/c/28210/" target="_blank">https://review.openstack.org/#<u></u>/c/28210/</a><br>
<br>
Both commits contain a bug fix plus some other random stuff. The former seems trivial but the second not so much. I realize we've gotten comfortable with 'git cherry-pick' but should we be more strict about ensuring backported patches contain only the minimum required to fix the bug? IMHO, I think cherry-pick'ing only works for minimal patches from master and we should be diligent about NACKing anything into stable that brings unrelated/unwated changes.<span class=""><font color="#888888"><br>
<br>
Adam<br>
</font></span></blockquote></div>Hey Adam,<br><br></div><div class="gmail_extra">Thanks for the feed-back and I agree with your point. <br><br></div><div class="gmail_extra"><a href="https://review.openstack.org/#/c/28207/" target="_blank">https://review.openstack.org/#/c/28207/</a><br>
is the Cherry Pick and was in fact intended to include a fix for both items. In the future we (the Cinder team and more specifically me) should probably be better about separate commits for separate issues. That being said it's basically a one line change that needed to be made so it was included, but the issue here is the commit to master and not the commit to stable. <br>
<br><br>
<a href="https://review.openstack.org/#/c/28210/" target="_blank">https://review.openstack.org/#/c/28210/</a><br></div><div class="gmail_extra">is another story, and I agree the addition of the fix to the formatting of the log messages should probably not be included. It started off as a single log entry with a misspelling that earned it a -1 in the review and then as we looked closer we found more and more typos and formatting issues in the log statements. It's safe as it's isolated to the logging output, however you're probably right that it should not be in this particular patch. I'm happy to remove those *extras* and resubmit.<br>
<br></div><div class="gmail_extra">Thanks,<br>John<br></div><div class="gmail_extra"><br></div></div>