Open Stack

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstack.org/pipermail/openstack-dev/attachments/20141009/4991a136/attachment.html>