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

[Martin Kosek]

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...

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