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