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

[Jan Cholasta]

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

-- 
Jan Cholasta