On 16.12.2015 12:24, Martin Kosek wrote:
> On 12/16/2015 12:01 PM, Petr Spacek wrote:
>> On 16.12.2015 11:15, Martin Kosek wrote:
>>> On 12/16/2015 10:02 AM, Petr Spacek wrote:
>>>> On 16.12.2015 09:53, Jan Cholasta wrote:
>>>>> On 16.12.2015 09:45, Petr Spacek wrote:
>>>>>> On 11.12.2015 15:50, Jan Cholasta wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 10.12.2015 18:04, Petr Spacek wrote:
>>>>>>>> On 9.12.2015 15:30, Petr Spacek wrote:
>>>>>>>>> Hello,
>>>>>>>>>
>>>>>>>>> this patch automates some of sanity checks proposed by Petr Vobornik.
>>>>>>>>>
>>>>>>>>> 'make review' should be used in root of clean Git tree which has 
>>>>>>>>> patches
>>>>>>>>> under
>>>>>>>>> review applied.
>>>>>>>>>
>>>>>>>>> Magic in review.sh attempts to detect nearest remote branch which can 
>>>>>>>>> be
>>>>>>>>> used
>>>>>>>>> as diff base for review. Please see review.sh for further details.
>>>>>>>>
>>>>>>>> And here is the patch! :-)
>>>>>>>
>>>>>>> Nice, but I would rather see this in ipatool
>>>>>>> (<https://github.com/freeipa/freeipa-tools>). Or is there any obvious 
>>>>>>> benefit
>>>>>>> in having this in freeipa itself that I'm missing?
>>>>>>
>>>>>> For me the obvious benefit is:
>>>>>> git clone
>>>>>> git am
>>>>>> make review
>>>>>>
>>>>>> Done.
>>>>>>
>>>>>> No need to find & learn other tool, no risk of using wrong version of 
>>>>>> the tool
>>>>>> to wrong version of source tree etc.
>>>>>
>>>>> AFAIK all IPA developers are supposed to use ipatool, and it already has a
>>>>
>>>> Good to know. How does a newcomer learn about it? Honestly I never used
>>>> ipatool (or not even cloned it).
>>>
>>> ipatool targets rather upstream members with write access, so they are 
>>> hardly
>>> newcomers.
>>
>> I though that we want to make it easy to contribute, so why are you talking
>> about core developers?
> 
> I was mostly refering to ipatool's "push" command that I mostly used, but it's
> true there is also "start-review" or "am" commands that could be useful to
> others too.
> 
>> Shouldn't we make it easy to self-review own patches for everyone? Including
>> random people who want to submit few patches and go away? (Think how we can
>> apply usability principles to development.)
> 
> We should, I hope my reply did not suggest otherwise.

Hmm, so I finally tried ipatool and it blew up:

$ ../tool/ipatool --help
Configuration is specified in the file given by --config.
Run ipatool sample-config to see what needs to go in it.

$ ../tool/ipatool sample-config
Traceback (most recent call last):
  File "../tool/ipatool", line 701, in <module>
    Context(docopt.docopt(__doc__)).run()
  File "../tool/ipatool", line 233, in __init__
    with open(os.path.expanduser(options['--config'])) as conf_file:
FileNotFoundError: [Errno 2] No such file or directory:
'/home/pspacek/.ipa/toolconf.yaml'

... after a while I found out that it is too heavy Python magic inside (for me
as non-Pythonist) so I'm giving up.

Moreover it AFAIK does not work on tree with applied patches (which is normal
situation before you send the patched to list for review), so I'm not even
sure how I could integrate the review scripts into it.

For now I'm going to live with my review.sh scripts in following Git repo:
https://github.com/pspacek/ipareview.git

Pull requests or rewrites for ipatool are welcome.
You can take this as a Christmas present :-)

-- 
Petr^2 Spacek

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to