[Freeipa-devel] [PATCHES] 295-299 Allow changing chaining of the IPA CA certificate

[Petr Viktorin]

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³