On Thu, Jul 23, 2015 at 8:40 PM, Andrew Woodward <xar...@gmail.com> wrote:
> 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/ > We will accept any patch that do not break backward compatibility for at least one release. 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 > If you consider to propose upstream, please follow this instructions to bootstrap the basic code structure: https://wiki.openstack.org/wiki/Puppet/New_module That will help you to have a compliant module from start. > [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 <degore...@mirantis.com> > 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 <xar...@gmail.com>: >> >>> 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: >>> openstack-dev-requ...@lists.openstack.org?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: >> openstack-dev-requ...@lists.openstack.org?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: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev > > -- Emilien Macchi
__________________________________________________________________________ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev