Re: [openstack-dev] [Fuel] Contribution to fuel-library: new requirements for reviews
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 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 > Date: Mon, Oct 6, 2014 at 6:12 PM > Subject: Re: Contribution to fuel-library > To: Anastasia Urlapova > Cc: Aleksandr Didenko , Andrey Danin < > ada...@mirantis.com>, Partner Integrations Team , Mike > Scherbakov , Sergii Golovatiuk < > sgolovat...@mirantis.com>, fuel-core-team > > > 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 > 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 >> 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 >>> 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 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 >>>
Re: [openstack-dev] [Fuel] Contribution to fuel-library: new requirements for reviews
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 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 > Date: Mon, Oct 6, 2014 at 6:12 PM > Subject: Re: Contribution to fuel-library > To: Anastasia Urlapova > Cc: Aleksandr Didenko , Andrey Danin < > ada...@mirantis.com>, Partner Integrations Team , Mike > Scherbakov , Sergii Golovatiuk < > sgolovat...@mirantis.com>, fuel-core-team > > > 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 > 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 >> 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 >>> 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 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
[openstack-dev] [Fuel] Contribution to fuel-library: new requirements for reviews
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 Date: Mon, Oct 6, 2014 at 6:12 PM Subject: Re: Contribution to fuel-library To: Anastasia Urlapova Cc: Aleksandr Didenko , Andrey Danin < ada...@mirantis.com>, Partner Integrations Team , Mike Scherbakov , Sergii Golovatiuk < sgolovat...@mirantis.com>, fuel-core-team 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 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 > 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 >> 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 >>> 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/puppe
