[Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.
Rob Crittenden
rcritten at redhat.com
Tue Aug 19 15:44:54 UTC 2014
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.
>>
>> 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).
>>
>> 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?
rob
More information about the Freeipa-devel
mailing list