<div dir="ltr">Documented in the Wiki:<br><a href="https://wiki.openstack.org/wiki/Fuel/Code_Review_Rules#Merging_commits">https://wiki.openstack.org/wiki/Fuel/Code_Review_Rules#Merging_commits</a><br></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Oct 9, 2014 at 1:19 AM, Mike Scherbakov <span dir="ltr"><<a href="mailto:mscherbakov@mirantis.com" target="_blank">mscherbakov@mirantis.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>This discussion should go in the public, as not every developer is here, and other opinions are important.</div><div><br></div><div>I believe that we are again diverting the conversation into a few directions. Let's address things one by one, and feel free to spin off new discussions out of this thread. After original email from Sergii, what I'm trying to achieve here - is to formalize new requirements for code review process. I suggest to start with something very small and limited - approving the code into master. I think rules should change, and reviews should have:</div><ul><li>at least one Subject Matter Expert (SME)'s +1</li><li>Core developer should not merge if there are no other +1th or +2th</li><li>Recommended to have two core reviews of complicated / suspicious patches</li><li>Must have at least two core reviews for patches which change reference architecture</li></ul><div class="gmail_quote">Thanks,</div><div class="gmail_quote"><br>From: <b class="gmail_sendername">Vladimir Kuklin</b> <span dir="ltr"><<a href="mailto:vkuklin@mirantis.com" target="_blank">vkuklin@mirantis.com</a>></span><br>Date: Mon, Oct 6, 2014 at 6:12 PM<br>Subject: Re: Contribution to fuel-library<br>To: Anastasia Urlapova <<a href="mailto:aurlapova@mirantis.com" target="_blank">aurlapova@mirantis.com</a>><br>Cc: Aleksandr Didenko <<a href="mailto:adidenko@mirantis.com" target="_blank">adidenko@mirantis.com</a>>, Andrey Danin <<a href="mailto:adanin@mirantis.com" target="_blank">adanin@mirantis.com</a>>, Partner Integrations Team <<a href="mailto:pi@mirantis.com" target="_blank">pi@mirantis.com</a>>, Mike Scherbakov <<a href="mailto:mscherbakov@mirantis.com" target="_blank">mscherbakov@mirantis.com</a>>, Sergii Golovatiuk <<a href="mailto:sgolovatiuk@mirantis.com" target="_blank">sgolovatiuk@mirantis.com</a>>, fuel-core-team <<a href="mailto:fuel-core-team@mirantis.com" target="_blank">fuel-core-team@mirantis.com</a>><br><br><br><div dir="ltr">Nastya,<br><br>I do not think modular testing blueprint is the right place to write this checklist down. I think, we need, first of all, create corresponding jenkins jobs and make them dry-run until there is implementation ready for them. E.g. we almost do already have --noop implementation support. Let's finally add it as a job to CI. And let's do not run actual deployments until CI passes syntax and noop verification, for example.<br></div><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Oct 6, 2014 at 6:07 PM, Anastasia Urlapova <span dir="ltr"><<a href="mailto:aurlapova@mirantis.com" target="_blank">aurlapova@mirantis.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div>Alex,</div><div>could you update <a href="https://blueprints.launchpad.net/fuel/+spec/fuel-library-modular-testing" target="_blank">https://blueprints.launchpad.net/fuel/+spec/fuel-library-modular-testing</a> </div><div>according your ideas and suggestions.</div><div><br></div><div>Thanks,</div><div> Nastya.</div></div><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Oct 6, 2014 at 3:53 PM, Aleksandr Didenko <span dir="ltr"><<a href="mailto:adidenko@mirantis.com" target="_blank">adidenko@mirantis.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">I would also add rspec testing to the CI as well:<br><div><span><div style="margin-left:40px"><br>a) code linting<br></div><div style="margin-left:40px">b) syntax checking<br></div></span><div style="margin-left:40px">c) [in future] rake spec testing<br></div><span><div style="margin-left:40px">d) noop dry run test<br></div><div style="margin-left:40px">e) [in future] deployment modules tests<br></div></span><div style="margin-left:40px">f) system tests<br>g) [in future] integration tests</div></div></div><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Oct 6, 2014 at 2:34 PM, Vladimir Kuklin <span dir="ltr"><<a href="mailto:vkuklin@mirantis.com" target="_blank">vkuklin@mirantis.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div><div><div>Ok, guys, I would suggest to start with formalization of requirements for code review in Fuel Library, as there is no such page on Fuel wiki. I think that workflow should be as following:<br><br></div>1) Code itself should be reviewed manually <br></div>2) Unit tests should be written by the developer himself. Other tests should be written by QA and/or developers<br></div>3) All the other criteria should be checked by CI automatically - there MUST be no manual process:<br><div style="margin-left:40px">a) code linting<br></div><div style="margin-left:40px">b) syntax checking<br></div><div style="margin-left:40px">c) noop dry run test<br></div><div style="margin-left:40px">d) [in future] deployment modules tests<br></div><div style="margin-left:40px">e) system tests<br></div><div style="margin-left:40px">f) [in future] integration tests<br></div>4) [In Future] each request can be accepted only after related upstream manifests bug is created and a review request for particular change is opened<br><div><div><div class="gmail_extra"><br></div><div class="gmail_extra">Some of these points are marked for future. But we can enable them as soon as corresponding testing code is written and CI infrastructure is ready.<br></div><div class="gmail_extra"><div><div><br><div class="gmail_quote">On Mon, Oct 6, 2014 at 1:28 PM, Andrey Danin <span dir="ltr"><<a href="mailto:adanin@mirantis.com" target="_blank">adanin@mirantis.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div><div><br></div>+PI team<br></div>Please, keep us in this discussion. We are interested in it a lot.<br></div><div class="gmail_extra"><div><div><br><div class="gmail_quote">On Mon, Oct 6, 2014 at 12:31 AM, Aleksandr Didenko <span dir="ltr"><<a href="mailto:adidenko@mirantis.com" target="_blank">adidenko@mirantis.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div>Hi,<span><br><br><div>> On #1, I'd love to hear more information and suggested workflow. I
know only something short like "propose contribution to upstream module
first, then to fuel-library"; I think we need more details, and make
sure everyone knows and follows.<br><br></div></span><div>First of all, I think everybody who wants to contribute into fuel-library should know some Puppet DSL basics and also read puppet-openstack dev docs [1] and our dev docs [2]<br></div><div><br></div><div>Since we start merging upstream manifests, the most common and general rule is: upstream modules should be modified only with bugfixes and improvements everyone in the community could benefit from. And appropriate patch should be proposed to the upstream project. In other cases (like applying our own logic or settings) we should do it in our own modules or in "openstack::*" classes (fuel-library/deployment/puppet/openstack), like: openstack::compute, openstack::cinder. I think, our main goal should be: use all the community modules "as is". This is why we need to contribute to the upstream projects.<br></div><div><br></div><div></div><div>So the workflow for contributing into fuel-library should be something like this:<br><br></div><div>1. Check what module you're contributing to and where is its upstream (if any). You can check this via Modulefile located in the most of puppet modules we use:<br> fuel-library/deployment/puppet/<b>MODULE</b>/Modulefile<br></div><div>For example:<br> fuel-library/deployment/puppet/ceilometer/Modulefile<br></div><div><br></div><div>2. If the module does not have Modulefile, then it's our own module and contribution to the upstream is not needed since there's no upstream. Exception here is "openstack" module
(fuel-library/deployment/puppet/openstack) - we've modified it a lot, so
we're not going to sync it with upstream. Especially taking into account our
future granular deployment model.<br><br></div><div>3. In other cases (when module has upstream), check if the upstream version already has needed functionality and we just need to upgrade our fork of the module or back-port needed changes. If it does not, then prepare a patch to the upstream following upstream module contribution rules. For example puppet-openstack rules [1]<br><br></div><div>4. Create a gerrit review for fuel-library and put the upstream commit ID or link to github pull-request (if the module is not on stackforge) into commit message.<br></div><div></div><div></div><div><br>[1] <a href="https://wiki.openstack.org/wiki/Puppet-openstack#Developer_documentation" target="_blank">https://wiki.openstack.org/wiki/Puppet-openstack#Developer_documentation</a><br>[2] <a href="http://docs.mirantis.com/fuel-dev/develop/module_structure.html" target="_blank">http://docs.mirantis.com/fuel-dev/develop/module_structure.html</a><br><br></div><div>Regards,<br>Alex<br></div></div></div><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Oct 6, 2014 at 8:17 AM, Mike Scherbakov <span dir="ltr"><<a href="mailto:mscherbakov@mirantis.com" target="_blank">mscherbakov@mirantis.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">Hi Sergii, Alex,<div>this is an important topic. I would split it into 2:</div><div><ol><li>Changes in the workflow of Puppet Modules contribution, after we've merged upstream ones</li><li>Revision of code review rules in fuel-library repository</li></ol><div>On #1, I'd love to hear more information and suggested workflow. I know only something short like "propose contribution to upstream module first, then to fuel-library"; I think we need more details, and make sure everyone knows and follows.</div></div><div><br></div><div>On #2, a bit of history and details. We've started with a small team and 1 core reviewer there. Then another one was added, but team was still small to comply OpenStack standards of having at least 2 core reviews per patch [1], [2]. Now, informally, we are trying to follow rules:</div><div><ul><li>SME should +1 (or +2 if he is core)</li><li>Core can give his +2 and merge</li></ul><div>Sometimes we have only Core member immediately accepting a changeset.</div></div><div><br></div><div>We have larger team now. In order to gradually improve over time, my +1 on Sergii's and Alex's suggestions of making criteria for code acceptance more strict. I'm wondering if we can expand the list written by Sergii into more details, discussing and finally accepting it into the workflow we follow, with publishing it to Fuel wiki.</div><div><br></div><div>[1] <a href="https://wiki.openstack.org/wiki/NeutronDevelopment#Being_a_Good_Neutron_Developer_.28and_becoming_a_Core_Developer.29" target="_blank">https://wiki.openstack.org/wiki/NeutronDevelopment#Being_a_Good_Neutron_Developer_.28and_becoming_a_Core_Developer.29</a>, " since two core developers are needed to approve any patch..."</div><div>[2] <a href="https://wiki.openstack.org/wiki/ReviewChecklist" target="_blank">https://wiki.openstack.org/wiki/ReviewChecklist</a>, Nova Review Checklist section, #6</div><div><br></div></div><div class="gmail_extra"><div><div><br><div class="gmail_quote">On Fri, Oct 3, 2014 at 2:06 PM, Aleksandr Didenko <span dir="ltr"><<a href="mailto:adidenko@mirantis.com" target="_blank">adidenko@mirantis.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div><div><div>+100500 to Sergii.<span><br><br>> I would recommend to have at least +1 from Fuel Library Core for Community patches before we merge.<br><br></span></div>Better for any patch in fuel-library, not just community ones :)<br><br></div>Regards,<br></div>Alex<br></div><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Oct 3, 2014 at 11:25 AM, Sergii Golovatiuk <span dir="ltr"><<a href="mailto:sgolovatiuk@mirantis.com" target="_blank">sgolovatiuk@mirantis.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div>Hi crew!<br><br></div><div>I would like to bring to your attention the topic we need to discuss. We merged a lot of Puppet upstream manifests and sill in process of merging. We should be very careful when we merge patches from non-core members as<br><br></div><div>1. This patch may correlate with puppet upstream manifest. Fuel Library checks if it's already applied to upstream.<br></div><div>2. Fuel Library Core Team tries to cover patches by tests<br></div><div>3. Fuel Library Core Team checks the linting and code standards<br></div><div>4. Fuel Library Core Team does all the best to backport patchsets to puppet upstream manifests<br><br></div><div>I would recommend to have at least +1 from Fuel Library Core for Community patches before we merge. Thank you very much!<br><br></div><div><div><div><div dir="ltr">--<br>
Best regards,<br>
Sergii Golovatiuk,<br>
Skype #golserge<br>
IRC #holser<span><font color="#888888"><br></font></span></div></div><span><font color="#888888">
</font></span></div></div></div><span><font color="#888888">
<p></p>
-- <br>
You received this message because you are subscribed to the Google Groups "fuel-core-team" group.<br>
To unsubscribe from this group and stop receiving emails from it, send an email to <a href="mailto:fuel-core-team+unsubscribe@mirantis.com" target="_blank">fuel-core-team+unsubscribe@mirantis.com</a>.<br>
For more options, visit <a href="https://groups.google.com/a/mirantis.com/d/optout" target="_blank">https://groups.google.com/a/mirantis.com/d/optout</a>.<br>
</font></span></blockquote></div><br></div>
<p></p>
-- <br>
You received this message because you are subscribed to the Google Groups "fuel-core-team" group.<br>
To unsubscribe from this group and stop receiving emails from it, send an email to <a href="mailto:fuel-core-team+unsubscribe@mirantis.com" target="_blank">fuel-core-team+unsubscribe@mirantis.com</a>.<br>
For more options, visit <a href="https://groups.google.com/a/mirantis.com/d/optout" target="_blank">https://groups.google.com/a/mirantis.com/d/optout</a>.<br>
</div></div></blockquote></div><br><br clear="all"><div><br></div></div></div><span><font color="#888888">-- <br><div dir="ltr">Mike Scherbakov<br>#mihgen<br><br></div>
</font></span></div>
</blockquote></div><br></div>
<p></p>
-- <br>
You received this message because you are subscribed to the Google Groups "fuel-core-team" group.<br>
To unsubscribe from this group and stop receiving emails from it, send an email to <a href="mailto:fuel-core-team+unsubscribe@mirantis.com" target="_blank">fuel-core-team+unsubscribe@mirantis.com</a>.<br>
For more options, visit <a href="https://groups.google.com/a/mirantis.com/d/optout" target="_blank">https://groups.google.com/a/mirantis.com/d/optout</a>.<br>
</div></div></blockquote></div><br><br clear="all"><br></div></div><span><font color="#888888">-- <br>Andrey Danin<br><a href="mailto:adanin@mirantis.com" target="_blank">adanin@mirantis.com</a><br>skype: gcon.monolake<br>
</font></span></div><div><div>
<p></p>
-- <br>
You received this message because you are subscribed to the Google Groups "fuel-core-team" group.<br>
To unsubscribe from this group and stop receiving emails from it, send an email to <a href="mailto:fuel-core-team+unsubscribe@mirantis.com" target="_blank">fuel-core-team+unsubscribe@mirantis.com</a>.<br>
For more options, visit <a href="https://groups.google.com/a/mirantis.com/d/optout" target="_blank">https://groups.google.com/a/mirantis.com/d/optout</a>.<br>
</div></div></blockquote></div><br><br clear="all"><br>-- <br></div></div><div dir="ltr">Yours Faithfully,<br>Vladimir Kuklin,<br>Fuel Library Tech Lead,<br>Mirantis, Inc.<br><a href="tel:%2B7%20%28495%29%20640-49-04" value="+74956404904" target="_blank">+7 (495) 640-49-04</a><br><a href="tel:%2B7%20%28926%29%20702-39-68" value="+79267023968" target="_blank">+7 (926) 702-39-68</a><br>Skype kuklinvv<br>45bk3, Vorontsovskaya Str.<br>Moscow, Russia,<br><a href="http://www.mirantis.ru/" target="_blank">www.mirantis.com</a><br><a href="http://www.mirantis.ru/" target="_blank">www.mirantis.ru</a><br><a href="mailto:vkuklin@mirantis.com" target="_blank">vkuklin@mirantis.com</a></div>
</div></div></div></div>
</blockquote></div><br></div>
<p></p>
-- <br>
You received this message because you are subscribed to the Google Groups "fuel-core-team" group.<br>
To unsubscribe from this group and stop receiving emails from it, send an email to <a href="mailto:fuel-core-team+unsubscribe@mirantis.com" target="_blank">fuel-core-team+unsubscribe@mirantis.com</a>.<br>
For more options, visit <a href="https://groups.google.com/a/mirantis.com/d/optout" target="_blank">https://groups.google.com/a/mirantis.com/d/optout</a>.<br>
</div></div></blockquote></div><br></div>
</div></div></blockquote></div><br><br clear="all"><br>-- <br><div dir="ltr">Yours Faithfully,<br>Vladimir Kuklin,<br>Fuel Library Tech Lead,<br>Mirantis, Inc.<br><a href="tel:%2B7%20%28495%29%20640-49-04" value="+74956404904" target="_blank">+7 (495) 640-49-04</a><br><a href="tel:%2B7%20%28926%29%20702-39-68" value="+79267023968" target="_blank">+7 (926) 702-39-68</a><br>Skype kuklinvv<br>45bk3, Vorontsovskaya Str.<br>Moscow, Russia,<br><a href="http://www.mirantis.ru/" target="_blank">www.mirantis.com</a><br><a href="http://www.mirantis.ru/" target="_blank">www.mirantis.ru</a><br><a href="mailto:vkuklin@mirantis.com" target="_blank">vkuklin@mirantis.com</a></div><span class="HOEnZb"><font color="#888888">
</font></span></div><span class="HOEnZb"><font color="#888888">
</font></span></div></div></div><span class="HOEnZb"><font color="#888888"><br><br clear="all"><div><br></div>-- <br><div dir="ltr">Mike Scherbakov<br>#mihgen<br><br></div>
</font></span></div>
<br>_______________________________________________<br>
OpenStack-dev mailing list<br>
<a href="mailto:OpenStack-dev@lists.openstack.org">OpenStack-dev@lists.openstack.org</a><br>
<a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev" target="_blank">http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev</a><br>
<br></blockquote></div><br><br clear="all"><br>-- <br><div dir="ltr"><div>Dmitry Borodaenko</div></div>
</div>