Re: [openstack-dev] [fuel][puppet] Module Sync for Murano and Sahara
On Thu, Jul 23, 2015 at 8:40 PM, Andrew Woodward 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 > 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 : >> >>> 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. >>> >>>
Re: [openstack-dev] [fuel][puppet] Module Sync for Murano and Sahara
On Wed, Jul 22, 2015 at 9:48 AM, Denis Egorenko 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/ > > Private URL, we can't read the logs from Internet. > [2] > http://jenkins-product.srt.mirantis.net:8080/view/custom_iso/job/7.0.custom.ubuntu.bvt_2/131/ > Same here. > 2015-07-22 0:41 GMT+03:00 Andrew Woodward : > >> 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 > > -- 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
Re: [openstack-dev] [fuel][puppet] Module Sync for Murano and Sahara
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 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 : > >> 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: >> op
Re: [openstack-dev] [fuel][puppet] Module Sync for Murano and Sahara
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 : > 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
[openstack-dev] [fuel][puppet] Module Sync for Murano and Sahara
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
