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