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

[Rob Crittenden]

Tomas Babej wrote:
> On 10/19/2012 03:16 PM, Martin Kosek wrote:
>> On 10/19/2012 02:49 PM, Tomas Babej wrote:
>>> 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
>> NACK. Server installation crashed with the first step:
>>
>> # ipa-server-install
>>
>> Configuring NTP daemon (ntpd)
>>    [1/4]: stopping ntpd
>>    [2/4]: writing configuration
>>    [3/4]: configuring ntpd to start on boot
>>    [4/4]: starting ntpd
>> Unexpected error - see /var/log/ipaserver-install.log for details:
>> TypeError: expected a character buffer object
>>
>>
>> ipaserver-install.log:
>>
>> 2012-10-19T12:59:37Z INFO   File
>> "/usr/lib/python2.7/site-packages/ipaserver/install/installutils.py",
>> line
>> 614, in run_script
>>      return_value = main_function()
>>
>>    File "/sbin/ipa-server-install", line 904, in main
>>      ntp.create_instance()
>>
>>    File
>> "/usr/lib/python2.7/site-packages/ipaserver/install/ntpinstance.py",
>> line 158, in create_instance
>>      self.start_creation()
>>
>>    File
>> "/usr/lib/python2.7/site-packages/ipaserver/install/service.py", line
>> 358, in start_creation
>>      self.print_msg(end_message)
>>
>>    File
>> "/usr/lib/python2.7/site-packages/ipaserver/install/service.py", line
>> 295, in print_msg
>>      print_msg(message, self.output_fd)
>>
>>    File
>> "/usr/lib/python2.7/site-packages/ipaserver/install/service.py", line
>> 60, in print_msg
>>      output_fd.write(message)
>>
>> 2012-10-19T12:59:37Z INFO The ipa-server-install command failed,
>> exception:
>> TypeError: expected a       character buffer object
>>
>>
>> Martin
> I also encountered this beforehand. Due to the unfortunate rebase, I
> retested
> the whole patch once again, to make sure there are no such errors that I
> considered fixed still lurking.
> This should work now.
>
> Tomas
>

ACK, pushed to master and ipa-3-0

rob