[Freeipa-devel] [PATCH 0018] Make service naming in ipa-server-install consistent

[Tomas Babej]

On 10/19/2012 01:44 PM, Martin Kosek wrote:
> On 10/19/2012 01:26 PM, Tomas Babej wrote:
>> On 10/18/2012 11:27 AM, Martin Kosek wrote:
>>> On 10/11/2012 05:11 PM, Tomas Babej wrote:
>>>> On 10/11/2012 12:32 PM, Martin Kosek wrote:
>>>>> On 10/11/2012 12:26 PM, Tomas Babej wrote:
>>>>>> Hi,
>>>>>>
>>>>>> This patch forces more consistency into ipa-server-install output. All
>>>>>> descriptions of services that are not instances of
>>>>>> SimpleServiceInstance are now in the following format:
>>>>>>
>>>>>> <Description> (<Service Name>)
>>>>>>
>>>>>> Furthermore, start_creation method has been modified to support
>>>>>> custom start and end messages.
>>>>>>
>>>>>> Sample output produced by this patch attached.
>>>>>>
>>>>>> https://fedorahosted.org/freeipa/ticket/3059
>>>>>>
>>>>>> Tomas
>>>>>>
>>>>> Just based on reading the patch, this does not look right:
>>>>>
>>>>> -        self.start_creation("Configuring certificate server", 210)
>>>>> +        self.start_creation("Configuring directory server for the CA",
>>>>> +            end_message="Done configuring directory server for the CA",
>>>>> +            show_service_name=True, runtime=210)
>>>>>
>>>>> Martin
>>>>>
>>>> Thanks, glitch fixed.
>>>>
>>>> Tomas
>>> Ok, I managed to get the patch a proper review. I like the result, in the
>>> console, but I still not entirely satisfied with the patch, looks
>>> over-engineered to me + there is a lot of duplication with "Configuring
>>> %(service)s" and "Done configuring %(service)s" messages.
>>>
>>> These messages could be easily generated only based on name of a service
>>> (self.service_name, we got that) and a new "friendly service name" or
>>> description.
>>>
>>> If we add description this way:
>>>
>>> class KrbInstance(service.Service):
>>>       def __init__(self, fstore=None):
>>>           service.Service.__init__(self, "krb5kdc", description="Kerberos KDC")
>>> ...
>>>
>>> the start_creation could be very simple:
>>> ...
>>> self.start_creation(runtime=30)
>>> ...
>>>
>>> All messages could be simply generated by the Service class just based on
>>> self.service_name and self.description with having the same output as you do
>>> now.
>>>
>>> Martin
>> I simplified the approach as you suggested. However, I kept optional
>> start_message and end_message parameters in case we would want
>> to specify the messages instead of using the pre-generated ones.
>> This is used in baseupdate.py and upgradeinstance.py
>>
>> More info in the documentation of start_creation() function.
>>
>> Tomas
> NACK. Tried running ./make-lint with your patch, got thousands of errors...
More like one error thousand times :) I somehow botched up rebasing
the patch, I swear that I corrected that indentation issue beforehand.
Nevermind, thanks for checking. ./make-lint runs clean now :)
>
> But the approach looks OK, I am just thinking that default value for
> show_service_name of start_creation() should be True as it is the most widely
> used value - you would not have to specify it everytime you call
> start_creation() in *instance.py files.
>
> Martin
Default values switched.

Tomas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0018-4-Make-service-naming-in-ipa-server-install-consistent.patch
Type: text/x-patch
Size: 15554 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20121019/265299aa/attachment.bin>