Denis, Now that I have better understanding of the history of the commit, I understand that this was the best way through. The Sahara and Murano team's effort was invaluable in getting these fixed up and in a good state. I apologize that I have raised this as an issue. I was very concerned with the commits before knowing theses details, It was necessary to get the clarification.
Let me clarify what I understand now was going on with them. Sahara. A )Fuel had a number of better parts of the fork. there where two commits [1][2] proposed to puppet-sahara from Fuel that where not merged that reflected the better side of Fuel's fork. B) The Sahara sync commit [3] into fuel represented upstream puppet-sahara C) The Adapt commit [4] contained the two commits listed prior in A, kilo support, stuff we needed to ensure it worked in fuel and Noop tests. [1] https://review.openstack.org/#/c/198744/ [2] https://review.openstack.org/#/c/192721/ [3] https://review.openstack.org/#/c/202045 [4] https://review.openstack.org/#/c/202195/ Murano D) Fuel has effectively the only usable Murano module E) The Adapt commit [5] represented * a major over hall of the code quality to make it suitable to propose upstream * fixes necessary to support kilo * cleanup for modular * Noop tests [5] https://review.openstack.org/#/c/203731/ With the improved clarity of what was going on, it made it much easier understand what I was reviewing and I'm glad of the current state. Here are my thoughts on what we can do better next time: * The commit and CR messages where not sufficient to understand entirely what was going on with the commits and how it was tested. * Separate out some of the changes into a commit chain to reduce the scope of each CR so that its easier to review. * For large reviews like this, we should let more reviewers know whats going on the ML early. These showed up on my radar late and of course, I freaked out. On Wed, Jul 22, 2015 at 6:51 AM Denis Egorenko <[email protected]> wrote: > Hi Andrew! > > Sahara already merged. All CI tests were succeeded, also was built custom > iso [1] and ran bvt tests [2], which also were succeeded and we got +1 from > QA team. > For Murano we will do the same: resolve all comments, build custom iso, > run custom bvt and wait +1 from Fuel CI and QA team. > > [1] > http://jenkins-product.srt.mirantis.net:8080/view/custom_iso/job/custom_7.0_iso/562/ > > [2] > http://jenkins-product.srt.mirantis.net:8080/view/custom_iso/job/7.0.custom.ubuntu.bvt_2/131/ > > 2015-07-22 0:41 GMT+03:00 Andrew Woodward <[email protected]>: > >> I was looped into reviewing the sync commits for Murano and Sahara. Both >> are in terrible shape and risk feature freeze at this point. >> >> We need feed back from the authors here. What is actually required for >> Kilo support (if any)from the Murano and Sahara modules? What will happen >> if these slip the release. What can you do to simplify the review scope. >> The most we can reasonably review is 500 LOC in any short time (and that's >> pushing it). >> >> Synopsis: >> murano [1] is -2, this can't be merged; there is a adapt commit with out >> any sync commit. The only way we will accept the fork method is a sync from >> upstream +adapt as documented in [2] also it's neigh impossible to review >> something this large with out the separation. >> -2 There is no upstream repo with content, so where did this even come >> from? We are/where the authority for murano at present so I'm baffled as to >> where this came from. >> >> Possible way through: A) Split sync from adapt, hopefully the adapt is >> small enough to to review. B)Make only changes necessary for kilo support. >> >> Sahara [3][4] >> This is a RED flah here, I'm not even sure to call it -1, -2 or something >> entirely else. I had with Serg M, This is a sync of upstream, plus the code >> on review from fuel that is not merged into puppet-sahara. I'm going to say >> that our fork is in much better shape at this moment, and we should just >> let it be. We shouldn't sync this until the upstream code is landed. >> >> Possible way through: C) The two outstanding commits inside the adapt >> commit need to be pulled out. They should be proposed right on top of the >> sync commit and should apply cleanly. I would prefer to see them as >> separate commits so they can be compared to the source more accurately. >> This should bring the adapt to something that could be reviewed. D) propose >> only the changes necessary to get kilo support. >> >> [1] https://review.openstack.org/#/c/203731/ >> [2] >> https://wiki.openstack.org/wiki/Fuel/How_to_contribute#Adding_new_puppet_modules_to_fuel-library >> [3] https://review.openstack.org/#/c/202045 >> [4] https://review.openstack.org/#/c/202195/ >> -- >> >> -- >> >> Andrew Woodward >> >> Mirantis >> >> Fuel Community Ambassador >> >> Ceph Community >> >> __________________________________________________________________________ >> OpenStack Development Mailing List (not for usage questions) >> Unsubscribe: >> [email protected]?subject:unsubscribe >> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev >> >> > > > -- > Best Regards, > Egorenko Denis, > Deployment Engineer > Mirantis > __________________________________________________________________________ > OpenStack Development Mailing List (not for usage questions) > Unsubscribe: [email protected]?subject:unsubscribe > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev > -- -- Andrew Woodward Mirantis Fuel Community Ambassador Ceph Community
__________________________________________________________________________ OpenStack Development Mailing List (not for usage questions) Unsubscribe: [email protected]?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
