On 01/13/2015 04:03 PM, Rob Crittenden wrote:
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

Yes I was also thinking about that. The code kinda works even with negative values (makes at least one pass of TGT kinit), but I guess that's really not how it should behave. I will update the patch accordingly.

There is also a question of semantics: should this option mean "Perform N additional attempts to get TGT if the first one fails" (this is the current implementation and N=0 then means "no additional attempts except the first one"), or should the meaning be "Perform N TGT attempts in total" (then N=0 would mean "do not do any TGT kinit calls" and thus makes no sense)?

I hope I made the distinction clear enough.

Martin^3

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to