On 01/13/2014 05:36 PM, Rob Crittenden wrote: > Ana Krivokapic wrote: >> On 11/15/2013 05:03 PM, Tomas Babej wrote: >>> On 11/07/2013 05:25 PM, Ana Krivokapic wrote: >>>> Hello, >>>> >>>> This patch addresses tickethttps://fedorahosted.org/freeipa/ticket/3790. >>>> >>>> >>>> >>>> _______________________________________________ >>>> Freeipa-devel mailing list >>>> [email protected] >>>> https://www.redhat.com/mailman/listinfo/freeipa-devel >>> >>> Looking good.. >>> >>> I have two questions: >>> >>> >>> 1.) Nitpick: I'd suggest we rename the save_state(service) and >>> restore_state(service) to more descriptive >>> save_service_state/restore_service_state? >>> >> >> Well, if the argument you are passing to these function does not have a >> name which suggests it is a service (which it should have anyway), you >> can do: `save_state(service=x)`. So I don't think >> `save_service_state(x)` is more readable. >> >>> 2.) There are other places in ipa-client-install where we save and >>> restore the state of the service. Having abstracted that into a >>> function, should we use this at other places as well? >>> >>> >> >> I looked at other instances in ipa-client-install where service states >> are saved and restored. It seems that each of these cases includes some >> custom logic which does not make it possible to use the two functions >> I've added. > > It looks fine to me as-is. I couldn't find any other services to include in > this which is indeed a shame as this significantly simplifies things. > > ACK > > rob
Thanks for the review. Pushed to master. Martin _______________________________________________ Freeipa-devel mailing list [email protected] https://www.redhat.com/mailman/listinfo/freeipa-devel
