Open Stack

Documented in the Wiki:
https://wiki.openstack.org/wiki/Fuel/Code_Review_Rules#Merging_commits

On Thu, Oct 9, 2014 at 1:19 AM, Mike Scherbakov <mscherbakov at 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 <vkuklin at mirantis.com>
> Date: Mon, Oct 6, 2014 at 6:12 PM
> Subject: Re: Contribution to fuel-library
> To: Anastasia Urlapova <aurlapova at mirantis.com>
> Cc: Aleksandr Didenko <adidenko at mirantis.com>, Andrey Danin <
> adanin at mirantis.com>, Partner Integrations Team <pi at mirantis.com>, Mike
> Scherbakov <mscherbakov at mirantis.com>, Sergii Golovatiuk <
> sgolovatiuk at mirantis.com>, fuel-core-team <fuel-core-team at 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 <aurlapova at 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 <adidenko at 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 <vkuklin at 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 <adanin at 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 <
>>>>> adidenko at 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 <
>>>>>> mscherbakov at 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 <
>>>>>>> adidenko at 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 <
>>>>>>>> sgolovatiuk at 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+unsubscribe at 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+unsubscribe at 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+unsubscribe at mirantis.com.
>>>>>> For more options, visit
>>>>>> https://groups.google.com/a/mirantis.com/d/optout.
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Andrey Danin
>>>>> adanin at 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+unsubscribe at 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
>>>> vkuklin at 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+unsubscribe at 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
> vkuklin at mirantis.com
>
>
>
> --
> Mike Scherbakov
> #mihgen
>
>
> _______________________________________________
> OpenStack-dev mailing list
> OpenStack-dev at lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
>


-- 
Dmitry Borodaenko
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstack.org/pipermail/openstack-dev/attachments/20141009/569042ed/attachment.html>