Code review comment for lp:~johnsca/charm-helpers/services-callback-fu

Revision history for this message
Benjamin Saller (bcsaller) wrote :

LGTM with some trivials

Thanks for this.

https://codereview.appspot.com/98490043/diff/1/charmhelpers/core/host.py
File charmhelpers/core/host.py (right):

https://codereview.appspot.com/98490043/diff/1/charmhelpers/core/host.py#newcode67
charmhelpers/core/host.py:67: """Determine whether a system service is
available"""
'an OS service' or 'an init service' or 'an upstart service'

or something similar, service is very overloaded in this context

https://codereview.appspot.com/98490043/diff/1/charmhelpers/core/services.py
File charmhelpers/core/services.py (right):

https://codereview.appspot.com/98490043/diff/1/charmhelpers/core/services.py#newcode32
charmhelpers/core/services.py:32: restarting the service via Upstart).
Indicate that callback is called with service name,

We should also consider an 'incomplete' event, if a relation is broken
we might stop the service for example.

https://codereview.appspot.com/98490043/diff/1/charmhelpers/core/services.py#newcode35
charmhelpers/core/services.py:35: the service via Upstart).
stop is fine, for a stop hook, but incomplete would be for
relation-broken hooks. We can defer that till we actually intend to
support it though.

https://codereview.appspot.com/98490043/diff/1/charmhelpers/core/services.py#newcode94
charmhelpers/core/services.py:94: for service_name in service_names or
SERVICES.keys():
Handle single argument string calls,
if service_names and isinstance(service_names, str):
    service_names = [service_names]

Makes sense with the 'incomplete' mentioned above

https://codereview.appspot.com/98490043/diff/1/charmhelpers/core/services.py#newcode104
charmhelpers/core/services.py:104: Given the name of a registered
service, return a ServiceManager
This isn't still a ServiceManager as per the comment if I'm reading this
properly.

https://codereview.appspot.com/98490043/

« Back to merge proposal