[Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.

[David Kupka]

On 08/19/2014 05:44 PM, Rob Crittenden wrote:
> David Kupka wrote:
>> On 08/19/2014 09:58 AM, Martin Kosek wrote:
>>> On 08/19/2014 09:05 AM, David Kupka wrote:
>>>> FreeIPA will use certmonger D-Bus API as discussed in this thread
>>>> https://www.redhat.com/archives/freeipa-devel/2014-July/msg00304.html
>>>>
>>>> This change should prevent hard-to-reproduce bugs like
>>>> https://fedorahosted.org/freeipa/ticket/4280
>>>
>>> Thanks for this effort, the updated certmonger module looks much
>>> better! This
>>> will help us get rid of the non-standard communication with certmonger.
>>>
>>> Just couple initial comments from me by reading the code:
>>>
>>> 1) Testing needs fixed version of certmonger, right? This needs to be
>>> spelled
>>> out right with the patch.
>> Yes, certmonger 0.75.13 and above should be fine according ticket
>> https://fedorahosted.org/certmonger/ticket/36. Added to patch description.
>
> You should update the spec to set the minimum version as well.
Sure, thanks.
>
>>>
>>> 2) Description text in patches is cheap, do not be afraid to use it and
>>> describe what you did and why. Link to the ticket is missing in the
>>> description
>>> as well:
>> Ok, increased verbosity a bit :-)
>>>
>>>> Subject: [PATCH] Use certmonger D-Bus API instead of messing with its
>>>> files.
>>>>
>>>> ---
>>>
>>> 3) get_request_id API:
>>>
>>>>            criteria = (
>>>> -            ('cert_storage_location', dogtag_constants.ALIAS_DIR,
>>>> -             certmonger.NPATH),
>>>> -            ('cert_nickname', nickname, None),
>>>> +            ('cert_storage_location', dogtag_constants.ALIAS_DIR),
>>>> +            ('cert_nickname', nickname),
>>>>            )
>>>>            request_id = certmonger.get_request_id(criteria)
>>>
>>> Do we want to continue using the "criteria" object or should we rather
>>> switch
>>> to normal function options? I.e. rather using
>>>
>>> request_id = certmonger.get_request_id(cert_nickname=nickname,
>>> cert_storage_location=dogtag_constants.ALIAS_DIR)
>>>
>>> ? It would look more consistent with other calls. I am just asking,
>>> not insisting.
>> I've no preference here. It seems to be a very small change. Has anyone
>> a reason to do it one way and not the other?
>
> I think I used this criteria thing to avoid having a bazillion optional
> parameters and for future-proofing. I think at this point the list is
> probably pretty stable, so I'd base it on whether you care about having
> a whole ton of optional parameters or not (it has the advantage of
> self-documenting itself).
>
The list is probably stable but also really excessive. I don't think it 
would help to have more than dozen optional parameters. So I prefer to 
leave as-is and change it in future if it is wanted.
>>>
>>> 3) Starting function:
>>>
>>>> +    try:
>>>> +        ipautil.run([paths.SYSTEMCTL, 'start', 'certmonger'],
>>>> skip_output=True)
>>>> +    except Exception, e:
>>>> +        root_logger.error('Failed to start certmonger: %s' % e)
>>>> +        raise e
>>>
>>> I see 2 issues related to this code:
>>> a) Do not call SYSTEMCTL directly. To be platform independent, rather use
>>> services.knownservices.messagebus.start() that is overridable by
>>> someone else
>>> porting to non-systemd platforms.
>> Is there anything that can't be done using ipalib/ipapython/ipaplatform?
>
> It can't make coffee (yet).
>
>>> b) In this case, do not use "raise e", but just "raise" to keep the
>>> exception
>>> stack trace intact for better debugging.
>> Every day there's something new to learn about python or FreeIPA.
>>>
>>> Both a) and b) should be fixed in other occasions and places.
>> I found only one occurence of a) issue. Is there some hidden or are you
>> talking about the whole FreeIPA project?
>>>
>>> 4) Feel free to add yourself to Authors section of this module. You
>>> refactored
>>> it greatly to earn it :-)
>> Done.
>
> You already import dbus, why also separately import DBusException?
>
Removed, thanks for noticing.
> rob
>

-- 
David Kupka
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-dkupka-0008-3-Use-certmonger-D-Bus-API-instead-of-messing-with-its.patch
Type: text/x-patch
Size: 35352 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140825/5576ea23/attachment.bin>