[Kimchi-devel] About the review process

[Aline Manera]

On 01/27/2014 06:25 AM, Shu Ming wrote:
> More and more patches coming into this mailing list and I found some
> comments to the patches were not addressed correctly. One scenario was
> that reviewer A had concern on the patch, but B and C gave the
> "reviewed-by" to th patch, then the maintainer merged the patch into the
> upstream without the concern addressed. The concern may be a very good
> point to improve the code. In order to eliminate the scenario, I think
> we can improve the code review process in this way:
>
> 1) To the reviewer, if you have concern on the patch, please give your
> opinion in a very clear way like "NACK" opposite to "Reviewed-by" in the
> first line of the reply.
>
> 2) To the submitter, please address all the concern and have the
> reviewer to give a clear opinion.
>
> 3) To the maintainer, please don't not merge the patch if the concern is
> not addressed.
>
> 4) To help the reviewer to understand your code, please post RFC for
> your features and have a widely discussion.
>
> One example, see the detail in the bottom,
> "Use new control modules in root.py" which was given one concern on the
> patch, but the submitter didn't reply to the concern. Then two others
> gave the "reviewed-by" to th patch that leads to the patch merged. The
> concern on the patch "not sure we can move these code to __all__ or
> other attribute of control/__init__.py just from kimchi.control.networks
> import *" was not very clear to the patch submitter and the patch
> submitter just ignored the comment.
>
> IMO, that is not a clean way to do the review. Here, we do miss the
> chance to improve the origin patch.  Finally, another patch "Improve
> controller" was post to address the concern which caused code quality
> improvement of the original patch delayed and work duplicated.
>
> See the example patch here and the reviewer's comment and concern inline.

Hi Ming,

Thanks for your note.
I agree with most of the points you exposed here.

I just want to make one thing clearer: will be rare the cases where 
everyone will agree with the patch implementation.
Because that we have some patches with "Reviewed-by" and "NACK".
In that cases, if the maintainer does not agree with the "NACK" the 
patch will be merged. Otherwise, the maintainer will block it.


> ---
> On 12/24/2013 02:41 AM, Aline Manera wrote:
>> --- a/src/kimchi/root.py
>> +++ b/src/kimchi/root.py
>> @@ -26,13 +26,23 @@ import json
>>
>>
>> from kimchi import auth
>> -from kimchi import controller
>> from kimchi import template
>> from kimchi.config import get_api_schema_file
>> from kimchi.control.utils import parse_request
>> -
>> -
>> -class Root(controller.Resource):
>> +from kimchi.control.base import Resource
>> +from kimchi.control.config import Config
>> +from kimchi.control.debugreports import DebugReports
>> +from kimchi.control.host import Host
>> +from kimchi.control.interfaces import Interfaces
>> +from kimchi.control.networks import Networks
>> +from kimchi.control.plugins import Plugins
>> +from kimchi.control.storagepools import StoragePools
>> +from kimchi.control.tasks import Tasks
>> +from kimchi.control.templates import Templates
>> +from kimchi.control.vms import VMs
> not sure we can move these code to __all__ or other attribute of
> control/__init__.py just
> from kimchi.control.networks import *
>
>
> +
> +
> +class Root(Resource):
> def __init__(self, model, dev_env):
> self._handled_error = ['error_page.400', 'error_page.404',
> 'error_page.405', 'error_page.406',
> @@ -45,17 +55,17 @@ class Root(controller.Resource):
> self._cp_config = dict([(key, self.error_development_handler)
> for key in self._handled_error])
>
> _______________________________________________
> Kimchi-devel mailing list
> Kimchi-devel at ovirt.org
> http://lists.ovirt.org/mailman/listinfo/kimchi-devel
>