[Freeipa-devel] [PATCH 0001] ipa-client-install: attempt to get host TGT several times before aborting client installation

[Martin Babinsky]

On 01/14/2015 04:53 PM, Martin Babinsky wrote:
> On 01/13/2015 04:48 PM, Martin Babinsky wrote:
>> On 01/13/2015 09:46 AM, Jan Cholasta wrote:
>>> Dne 13.1.2015 v 09:22 Martin Kosek napsal(a):
>>>> On 01/12/2015 05:45 PM, Martin Babinsky wrote:
>>>>> related to ticket https://fedorahosted.org/freeipa/ticket/4808
>>>>>
>>>>> Patch attached.
>>>>>
>>>>> Martin^3
>>>>
>>>> I think the --tgt-kinit-attempts approach is good one. Couple comments
>>>> I have
>>>> when reading the patch:
>>>>
>>>> 1) Function
>>>> +def get_host_tgt(options, keytab, host, realm, env):
>>>> should be made more general purpose and instead of whole "options", it
>>>> should
>>>> rather accept just "kinit_attemps". It will then enable future
>>>> generations to
>>>> reuse the function for something else. Just a generally good practice,
>>>> nothing
>>>> critical.
>>>>
>>>> 2) I think
>>>> +        if returncode == 0:
>>>> +            root_logger.info("Attempt %d succeeded." % n_attempts)
>>>> +            break
>>>>
>>>> can be just DEBUG level. People do not need to know we will try
>>>> multiple attempts.
>>>>
>>>> 3) It may be even better to print
>>>> "Attempt %d/%d failed." instead of just number. But this is up to you.
>>>>
>>>> 4) I see several C-isms in the code, as a programming practice, let us
>>>> remove
>>>> them :-) In Python, the OK/notOK status is generally passed via
>>>> exceptions, not
>>>> return codes unless you really need them for anything meaningful.
>>>>
>>>> So, you can omit "raiseonerr=False" and have the handling code in an
>>>> Except
>>>> clause. When max number of attempts is breached, you then just raise
>>>> the
>>>> exception further (use bare "raise", to re-raise to keep the original
>>>> stack).
>>>
>>> +1
>>>
>>> Additionally:
>>>
>>> 5) I would prefer if the option was named --kinit-attempts instead of
>>> --tgt-kinit-attempts (the "tgt" seems redundant).
>>>
>>> 6) Please do not use backslashes for line wrapping, unless it is
>>> absolutely necessary. Instead, enclose the expression in parens for
>>> implicit continuation:
>>>
>>> +                           help=("number of attempts to obtain host
>>> TGT"
>>> +                                 "if the first one fails (defaults to
>>> %default)."))
>>>
>>> 7) Please follow PEP8 in new code:
>>>
>>> ipa-client/ipa-install/ipa-client-install:151:80: E501 line too long (93
>>>  > 79 characters)
>>> ipa-client/ipa-install/ipa-client-install:1100:1: E302 expected 2 blank
>>> lines, found 1
>>> ipa-client/ipa-install/ipa-client-install:1107:29: E126 continuation
>>> line over-indented for hanging indent
>>> ipa-client/ipa-install/ipa-client-install:1107:41: E231 missing
>>> whitespace after ','
>>> ipa-client/ipa-install/ipa-client-install:1108:29: E128 continuation
>>> line under-indented for visual indent
>>> ipa-client/ipa-install/ipa-client-install:1116:51: E225 missing
>>> whitespace around operator
>>> ipa-client/ipa-install/ipa-client-install:2453:80: E501 line too long
>>> (88 > 79 characters)
>>> ipa-client/ipa-install/ipa-client-install:2454:80: E501 line too long
>>> (89 > 79 characters)
>>> ipa-client/ipa-install/ipa-client-install:2531:80: E501 line too long
>>> (83 > 79 characters)
>>> ipa-client/ipa-install/ipa-client-install:2532:80: E501 line too long
>>> (81 > 79 characters)
>>>
>>> Honza
>>>
>> Thank you for your comments. Attaching the updated patch (I have sent
>> the message much earlier, but only to Jan because I messed up the reply
>> addresses in Thunderbird. Sorry for that).
>>
>> Martin
>>
>>
>> _______________________________________________
>> Freeipa-devel mailing list
>> Freeipa-devel at redhat.com
>> https://www.redhat.com/mailman/listinfo/freeipa-devel
>>
>
> Attaching updated patch.
>
> https://fedorahosted.org/freeipa/ticket/4808
>
> Martin^3
>
>
>
> _______________________________________________
> Freeipa-devel mailing list
> Freeipa-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/freeipa-devel
>

Forgot to update 'ipa-client-install' man page. The updated patch fixes 
this. Thanks to Rob for pointing it out.

Martin^3
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbabinsk-0001-4-ipa-client-install-added-new-option.patch
Type: text/x-patch
Size: 6010 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150114/66d75a5a/attachment.bin>