Hi, I think it must be a mandatory request for fuel-library core reviewers.
-- Best regards, Sergii Golovatiuk, Skype #golserge IRC #holser On Thu, Oct 9, 2014 at 10:19 AM, Mike Scherbakov <mscherba...@mirantis.com> wrote: > This discussion should go in the public, as not every developer is here, > and other opinions are important. > > 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: > > - at least one Subject Matter Expert (SME)'s +1 > - Core developer should not merge if there are no other +1th or +2th > - Recommended to have two core reviews of complicated / suspicious > patches > - Must have at least two core reviews for patches which change > reference architecture > > Thanks, > > From: Vladimir Kuklin <vkuk...@mirantis.com> > Date: Mon, Oct 6, 2014 at 6:12 PM > Subject: Re: Contribution to fuel-library > To: Anastasia Urlapova <aurlap...@mirantis.com> > Cc: Aleksandr Didenko <adide...@mirantis.com>, Andrey Danin < > ada...@mirantis.com>, Partner Integrations Team <p...@mirantis.com>, Mike > Scherbakov <mscherba...@mirantis.com>, Sergii Golovatiuk < > sgolovat...@mirantis.com>, fuel-core-team <fuel-core-t...@mirantis.com> > > > Nastya, > > 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. > > On Mon, Oct 6, 2014 at 6:07 PM, Anastasia Urlapova <aurlap...@mirantis.com > > wrote: > >> Alex, >> could you update >> https://blueprints.launchpad.net/fuel/+spec/fuel-library-modular-testing >> according your ideas and suggestions. >> >> Thanks, >> Nastya. >> >> On Mon, Oct 6, 2014 at 3:53 PM, Aleksandr Didenko <adide...@mirantis.com> >> wrote: >> >>> I would also add rspec testing to the CI as well: >>> >>> a) code linting >>> b) syntax checking >>> c) [in future] rake spec testing >>> d) noop dry run test >>> e) [in future] deployment modules tests >>> f) system tests >>> g) [in future] integration tests >>> >>> On Mon, Oct 6, 2014 at 2:34 PM, Vladimir Kuklin <vkuk...@mirantis.com> >>> wrote: >>> >>>> 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: >>>> >>>> 1) Code itself should be reviewed manually >>>> 2) Unit tests should be written by the developer himself. Other tests >>>> should be written by QA and/or developers >>>> 3) All the other criteria should be checked by CI automatically - there >>>> MUST be no manual process: >>>> a) code linting >>>> b) syntax checking >>>> c) noop dry run test >>>> d) [in future] deployment modules tests >>>> e) system tests >>>> f) [in future] integration tests >>>> 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 >>>> >>>> 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. >>>> >>>> On Mon, Oct 6, 2014 at 1:28 PM, Andrey Danin <ada...@mirantis.com> >>>> wrote: >>>> >>>>> >>>>> +PI team >>>>> Please, keep us in this discussion. We are interested in it a lot. >>>>> >>>>> On Mon, Oct 6, 2014 at 12:31 AM, Aleksandr Didenko < >>>>> adide...@mirantis.com> wrote: >>>>> >>>>>> Hi, >>>>>> >>>>>> > 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. >>>>>> >>>>>> 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] >>>>>> >>>>>> 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. >>>>>> >>>>>> So the workflow for contributing into fuel-library should be >>>>>> something like this: >>>>>> >>>>>> 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: >>>>>> fuel-library/deployment/puppet/*MODULE*/Modulefile >>>>>> For example: >>>>>> fuel-library/deployment/puppet/ceilometer/Modulefile >>>>>> >>>>>> 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. >>>>>> >>>>>> 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] >>>>>> >>>>>> 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. >>>>>> >>>>>> [1] >>>>>> https://wiki.openstack.org/wiki/Puppet-openstack#Developer_documentation >>>>>> [2] http://docs.mirantis.com/fuel-dev/develop/module_structure.html >>>>>> >>>>>> Regards, >>>>>> Alex >>>>>> >>>>>> On Mon, Oct 6, 2014 at 8:17 AM, Mike Scherbakov < >>>>>> mscherba...@mirantis.com> wrote: >>>>>> >>>>>>> Hi Sergii, Alex, >>>>>>> this is an important topic. I would split it into 2: >>>>>>> >>>>>>> 1. Changes in the workflow of Puppet Modules contribution, after >>>>>>> we've merged upstream ones >>>>>>> 2. Revision of code review rules in fuel-library repository >>>>>>> >>>>>>> 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. >>>>>>> >>>>>>> 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: >>>>>>> >>>>>>> - SME should +1 (or +2 if he is core) >>>>>>> - Core can give his +2 and merge >>>>>>> >>>>>>> Sometimes we have only Core member immediately accepting a changeset. >>>>>>> >>>>>>> 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. >>>>>>> >>>>>>> [1] >>>>>>> https://wiki.openstack.org/wiki/NeutronDevelopment#Being_a_Good_Neutron_Developer_.28and_becoming_a_Core_Developer.29, >>>>>>> " >>>>>>> since two core developers are needed to approve any patch..." >>>>>>> [2] https://wiki.openstack.org/wiki/ReviewChecklist, Nova Review >>>>>>> Checklist section, #6 >>>>>>> >>>>>>> >>>>>>> On Fri, Oct 3, 2014 at 2:06 PM, Aleksandr Didenko < >>>>>>> adide...@mirantis.com> wrote: >>>>>>> >>>>>>>> +100500 to Sergii. >>>>>>>> >>>>>>>> > I would recommend to have at least +1 from Fuel Library Core for >>>>>>>> Community patches before we merge. >>>>>>>> >>>>>>>> Better for any patch in fuel-library, not just community ones :) >>>>>>>> >>>>>>>> Regards, >>>>>>>> Alex >>>>>>>> >>>>>>>> On Fri, Oct 3, 2014 at 11:25 AM, Sergii Golovatiuk < >>>>>>>> sgolovat...@mirantis.com> wrote: >>>>>>>> >>>>>>>>> Hi crew! >>>>>>>>> >>>>>>>>> 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 >>>>>>>>> >>>>>>>>> 1. This patch may correlate with puppet upstream manifest. Fuel >>>>>>>>> Library checks if it's already applied to upstream. >>>>>>>>> 2. Fuel Library Core Team tries to cover patches by tests >>>>>>>>> 3. Fuel Library Core Team checks the linting and code standards >>>>>>>>> 4. Fuel Library Core Team does all the best to backport patchsets >>>>>>>>> to puppet upstream manifests >>>>>>>>> >>>>>>>>> I would recommend to have at least +1 from Fuel Library Core for >>>>>>>>> Community patches before we merge. Thank you very much! >>>>>>>>> >>>>>>>>> -- >>>>>>>>> Best regards, >>>>>>>>> Sergii Golovatiuk, >>>>>>>>> Skype #golserge >>>>>>>>> IRC #holser >>>>>>>>> >>>>>>>>> -- >>>>>>>>> You received this message because you are subscribed to the Google >>>>>>>>> Groups "fuel-core-team" group. >>>>>>>>> To unsubscribe from this group and stop receiving emails from it, >>>>>>>>> send an email to fuel-core-team+unsubscr...@mirantis.com. >>>>>>>>> For more options, visit >>>>>>>>> https://groups.google.com/a/mirantis.com/d/optout. >>>>>>>>> >>>>>>>> >>>>>>>> -- >>>>>>>> You received this message because you are subscribed to the Google >>>>>>>> Groups "fuel-core-team" group. >>>>>>>> To unsubscribe from this group and stop receiving emails from it, >>>>>>>> send an email to fuel-core-team+unsubscr...@mirantis.com. >>>>>>>> For more options, visit >>>>>>>> https://groups.google.com/a/mirantis.com/d/optout. >>>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> -- >>>>>>> Mike Scherbakov >>>>>>> #mihgen >>>>>>> >>>>>>> >>>>>> -- >>>>>> You received this message because you are subscribed to the Google >>>>>> Groups "fuel-core-team" group. >>>>>> To unsubscribe from this group and stop receiving emails from it, >>>>>> send an email to fuel-core-team+unsubscr...@mirantis.com. >>>>>> For more options, visit >>>>>> https://groups.google.com/a/mirantis.com/d/optout. >>>>>> >>>>> >>>>> >>>>> >>>>> -- >>>>> Andrey Danin >>>>> ada...@mirantis.com >>>>> skype: gcon.monolake >>>>> >>>>> -- >>>>> You received this message because you are subscribed to the Google >>>>> Groups "fuel-core-team" group. >>>>> To unsubscribe from this group and stop receiving emails from it, send >>>>> an email to fuel-core-team+unsubscr...@mirantis.com. >>>>> For more options, visit >>>>> https://groups.google.com/a/mirantis.com/d/optout. >>>>> >>>> >>>> >>>> >>>> -- >>>> Yours Faithfully, >>>> Vladimir Kuklin, >>>> Fuel Library Tech Lead, >>>> Mirantis, Inc. >>>> +7 (495) 640-49-04 >>>> +7 (926) 702-39-68 >>>> Skype kuklinvv >>>> 45bk3, Vorontsovskaya Str. >>>> Moscow, Russia, >>>> www.mirantis.com <http://www.mirantis.ru/> >>>> www.mirantis.ru >>>> vkuk...@mirantis.com >>>> >>> >>> -- >>> You received this message because you are subscribed to the Google >>> Groups "fuel-core-team" group. >>> To unsubscribe from this group and stop receiving emails from it, send >>> an email to fuel-core-team+unsubscr...@mirantis.com. >>> For more options, visit >>> https://groups.google.com/a/mirantis.com/d/optout. >>> >> >> > > > -- > Yours Faithfully, > Vladimir Kuklin, > Fuel Library Tech Lead, > Mirantis, Inc. > +7 (495) 640-49-04 > +7 (926) 702-39-68 > Skype kuklinvv > 45bk3, Vorontsovskaya Str. > Moscow, Russia, > www.mirantis.com <http://www.mirantis.ru/> > www.mirantis.ru > vkuk...@mirantis.com > > > > -- > Mike Scherbakov > #mihgen > > > _______________________________________________ > OpenStack-dev mailing list > OpenStack-dev@lists.openstack.org > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev > >
_______________________________________________ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev