<div dir="ltr"><div>But what if I want to update some module that consists of ten or even more files (like rpc or db) and each of these files has quite a long change log? In that case the commit message may turn out to be really long even if only commit ids and names are included. <br>
</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Nov 20, 2013 at 12:37 PM, Elena Ezhova <span dir="ltr"><<a href="mailto:eezhova@mirantis.com" target="_blank">eezhova@mirantis.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><font size="-1"> <br></font>
20.11.2013, 06:18, "John Griffith" <<a href="mailto:john.griffith@solidfire.com" target="_blank">john.griffith@solidfire.com</a>>:<br>
<br>
On Mon, Nov 18, 2013 at 3:53 PM, Mark McLoughlin <<a href="mailto:markmc@redhat.com" target="_blank">markmc@redhat.com</a>> wrote:<br>
<br>
> On Mon, 2013-11-18 at 17:24 +0000, Duncan Thomas wrote:<br>
>> Random OSLO updates with no list of what changed, what got fixed etc<br>
>> are unlikely to get review attention - doing such a review is<br>
>> extremely difficult. I was -2ing them and asking for more info, but<br>
>> they keep popping up. I'm really not sure what the best way of<br>
>> updating from OSLO is, but this isn't it.<br>
> Best practice is to include a list of changes being synced, for example:<br>
><br>
> <a href="https://review.openstack.org/54660" target="_blank">https://review.openstack.org/54660</a><br>
><br>
> Every so often, we throw around ideas for automating the generation of<br>
> this changes list - e.g. cinder would have the oslo-incubator commit ID<br>
> for each module stored in a file in git to tell us when it was last<br>
> synced.<br>
><br>
> Mark.<br>
><br>
> _____________________________<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">__________________<br>
> OpenStack-dev mailing list<br>
> <a href="mailto:OpenStack-dev@lists.openstack.org" target="_blank">OpenStack-dev@lists.openstack.org</a><br>
> <a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev" target="_blank">http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev</a><br>
<br>
Been away on vacation so I'm afraid I'm a bit late on this... but;<br>
<br>
I think the point Duncan is bringing up here is that there are some<br>
VERY large and significant patches coming from OSLO pulls. The DB<br>
patch in particular being over 1K lines of code to a critical portion<br>
of the code is a bit unnerving to try and do a review on. I realize<br>
that there's a level of trust that goes with the work that's done in<br>
OSLO and synchronizing those changes across the projects, but I think<br>
a few key concerns here are:<br>
<br>
1. Doing huge pulls from OSLO like the DB patch here are nearly<br>
impossible to thoroughly review and test. Over time we learn a lot<br>
about real usage scenarios and the database and tweak things as we go,<br>
so seeing a patch set like this show up is always a bit unnerving and<br>
frankly nobody is overly excited to review it.<br>
<br>
2. Given a certain level of *trust* for the work that folks do on the<br>
OSLO side in submitting these patches and new additions, I think some<br>
of the responsibility on the review of the code falls on the OSLO<br>
team. That being said there is still the issue of how these changes<br>
will impact projects *other* than Nova which I think is sometimes<br>
neglected. There have been a number of OSLO synchs pushed to Cinder<br>
that fail gating jobs, some get fixed, some get abandoned, but in<br>
either case it shows that there wasn't any testing done with projects<br>
other than Nova (PLEASE note, I'm not referring to this particular<br>
round of patches or calling any patch set out, just stating a<br>
historical fact).<br>
<br>
3. We need better documentation in commit messages explaining why the<br>
changes are necessary and what they do for us. I'm sorry but in my<br>
opinion the answer "it's the latest in OSLO and Nova already has it"<br>
is not enough of an answer in my opinion. The patches mentioned in<br>
this thread in my opinion met the minimum requirements because they at<br>
least reference the OSLO commit which is great. In addition I'd like<br>
to see something to address any discovered issues or testing done with<br>
the specific projects these changes are being synced to.<br>
<br>
I'm in no way saying I don't want Cinder to play nice with the common<br>
code or to get in line with the way other projects do things but I am<br>
saying that I think we have a ways to go in terms of better<br>
communication here and in terms of OSLO code actually keeping in mind<br>
the entire OpenStack eco-system as opposed to just changes that were<br>
needed/updated in Nova. Cinder in particular went through some pretty<br>
massive DB re-factoring and changes during Havana and there was a lot<br>
of really good work there but it didn't come without a cost and the<br>
benefits were examined and weighed pretty heavily. I also think that<br>
some times the indirection introduced by adding some of the<br>
openstack.common code is unnecessary and in some cases makes things<br>
more difficult than they should be.<br>
<br>
I'm just not sure that we always do a very good ROI investigation or<br>
risk assessment on changes, and that opinion applies to ALL changes in<br>
OpenStack projects, not OSLO specific or anything else.<br>
<br>
All of that being said, a couple of those syncs on the list are<br>
outdated. We should start by doing a fresh pull for these and if<br>
possible add some better documentation in the commit messages as to<br>
the justification for the patches that would be great. We can take a<br>
closer look at the changes and the history behind them and try to get<br>
some review progress made here. Mark mentioned some good ideas<br>
regarding capturing commit ID's from synchronization pulls and I'd<br>
like to look into that a bit as well.<br>
<br>
Thanks,<br>
John<br>
<br>
_______________________________________________<br>
OpenStack-dev mailing list<br>
<a href="mailto:OpenStack-dev@lists.openstack.org" target="_blank">OpenStack-dev@lists.openstack.org</a><br>
<a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev" target="_blank">http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev</a></blockquote><div><br><br></div><div>I see now that updating OSLO is a far more complex issue than it may seem from the first sight.<br>
But I would really like to do my best to make it look acceptable and possible to review.<br><br>It seems everybody agrees that commit messages should have change logs of the updated files.<br>It can be done by writing a shell script that will generate this logs by simply executing a git log command.<br>
But the problem is how to get the initial version of the file that is being updated.<br><br></div><div>As Mark McLoughlin said, it may be a good idea to store OSLO commit-ids for each module in some file.<br></div><div>This file will have to be changed every time an update is performed which implies changing update.py<br>
in oslo-incubator. The the shell script will just have to fetch a change-id from this file.<br><br></div><div>I will try to write the script and generate logs, but I'm not sure about how can I put all these logs in<br>
</div><div>a commit message because one patch usually changes more than one file and each file may have<br></div><div>quite a long log.<br><br></div><div>So I would really appreciate any comments or pieces of advice.<br>
<br>
</div><div>Thanks,<br>Elena<br> </div></div>
</blockquote></div><br></div>