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

[Rob Crittenden]

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
> 

I'd recommend a min/max validator too. Min 1 because 0 makes no sense,
and max, I dunno, MAXINT if nothing else. Otherwise guaranteed to get
kicked back by QE at some point.

rob