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 _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
