Open Stack

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 <degorenko at 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 <xarses at 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-request at 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-request at lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
-- 

--

Andrew Woodward

Mirantis

Fuel Community Ambassador

Ceph Community
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstack.org/pipermail/openstack-dev/attachments/20150724/28ffd687/attachment.html>