On 07/30/2014 02:51 PM, Jan Cholasta wrote:
Dne 30.7.2014 v 14:47 Rob Crittenden napsal(a):
Jan Cholasta wrote:
Dne 29.7.2014 v 16:33 Rob Crittenden napsal(a):
Rob Crittenden wrote:
Jan Cholasta wrote:
Dne 28.7.2014 v 21:39 Rob Crittenden napsal(a):
This is oh-so close. AFAICT it generally does what it should, I
think it
is ready for a wider audience. Just a few more things:

306: A while True loop is used for something which AFAICT can only
ever
execute once. I'd think something like this is more readable:

for ca_nick, ca_flags in db.list_certs():
       if db.has_nickname(ca_cert):
           try:
               db.delete_cert(ca_nick)
           except ipautil.CalledProcessError:
               syslog.syslog(
                   syslog.LOG_ERR,
                   "Failed to remove certificate %s" % ca_nick)

Actually the while loop is necessary, because certutil -D (and in
turn
CertDB.delete_cert) deletes just a single cert with the nickname, but
there may be more versions of it and we need to delete them all.

Aha, excellent point. This would be a great comment in the code!


+1 on the additional syslogs. It will help figure out what's going
on if
things go sideways.

Otherwise things seem to be working. I think that fixing the
above is
enough for a +57 with the promise of unit tests to back up some of
these
new functions.

I'm working on that.


rob
rob


I have made a slight adjustment to patch 246 because of
<https://fedorahosted.org/freeipa/ticket/4039>, see
<http://www.redhat.com/archives/freeipa-devel/2014-July/msg00369.html>.


Updated rebased patches attached.

(once again, the correct order to apply them is 241-253, 262-294,
303-305, 295-299, 306-307)


ACK on 246.

IMHO this is ready to be pushed if you can add the comment on 306.

Added. Updated patch attached.


A slight rebase on 251 is needed for freeipa.spec.in.

Oh, or maybe not since you sent the whole shebang. I just did an
interdiff of the current and old 246.

Yep, I remember fixing this particular merge conflict.

ACK

rob


Thank you for the review.

(please push in this order: 241-253, 262-294, 303-305, 295-299, 306-307)


Pushed to:
master: 044c5c833a83a541f97785279acfe8e113035b3d
ipa-4-1: 044c5c833a83a541f97785279acfe8e113035b3d


--
PetrĀ³

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

Reply via email to