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

[Martin Babinsky]

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

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