Merge lp:~stub/charm-helpers/bug-1214793-service-wrappers into lp:charm-helpers

[Stuart Bishop]

Make wrappers around the service() call, such as service_start() and service_stop(), return the underlying result.

[Jacek Nykis (jacekn)]

Looks fine, all tests check out.

review: Approve

=== modified file 'charmhelpers/core/host.py'
--- charmhelpers/core/host.py	2013-08-07 23:14:14 +0000
+++ charmhelpers/core/host.py	2013-08-21 10:12:28 +0000
@@ -19,20 +19,22 @@
 
 
 def service_start(service_name):
-    service('start', service_name)
+    return service('start', service_name)
 
 
 def service_stop(service_name):
-    service('stop', service_name)
+    return service('stop', service_name)
 
 
 def service_restart(service_name):
-    service('restart', service_name)
+    return service('restart', service_name)
 
 
 def service_reload(service_name, restart_on_failure=False):
-    if not service('reload', service_name) and restart_on_failure:
-        service('restart', service_name)
+    service_result = service('reload', service_name)
+    if not service_result and restart_on_failure:
+        service_result = service('restart', service_name)
+    return service_result
 
 
 def service(action, service_name):

=== modified file 'tests/core/test_host.py'
--- tests/core/test_host.py	2013-08-07 23:14:14 +0000
+++ tests/core/test_host.py	2013-08-21 10:12:28 +0000
@@ -82,21 +82,24 @@
     @patch.object(host, 'service')
     def test_starts_a_service(self, service):
         service_name = 'foo-service'
-        host.service_start(service_name)
+        service.side_effect = [True]
+        self.assertTrue(host.service_start(service_name))
 
         service.assert_called_with('start', service_name)
 
     @patch.object(host, 'service')
     def test_stops_a_service(self, service):
         service_name = 'foo-service'
-        host.service_stop(service_name)
+        service.side_effect = [True]
+        self.assertTrue(host.service_stop(service_name))
 
         service.assert_called_with('stop', service_name)
 
     @patch.object(host, 'service')
     def test_restarts_a_service(self, service):
         service_name = 'foo-service'
-        host.service_restart(service_name)
+        service.side_effect = [True]
+        self.assertTrue(host.service_restart(service_name))
 
         service.assert_called_with('restart', service_name)
 
@@ -104,7 +107,7 @@
     def test_reloads_a_service(self, service):
         service_name = 'foo-service'
         service.side_effect = [True]
-        host.service_reload(service_name)
+        self.assertTrue(host.service_reload(service_name))
 
         service.assert_called_with('reload', service_name)
 
@@ -112,7 +115,8 @@
     def test_failed_reload_restarts_a_service(self, service):
         service_name = 'foo-service'
         service.side_effect = [False, True]
-        host.service_reload(service_name, restart_on_failure=True)
+        self.assertTrue(
+            host.service_reload(service_name, restart_on_failure=True))
 
         service.assert_has_calls([
             call('reload', service_name),
@@ -123,9 +127,54 @@
     def test_failed_reload_without_restart(self, service):
         service_name = 'foo-service'
         service.side_effect = [False]
-        host.service_reload(service_name)
-
-        service.assert_called_with('reload', service_name)
+        self.assertFalse(host.service_reload(service_name))
+
+        service.assert_called_with('reload', service_name)
+
+
+    @patch.object(host, 'service')
+    def test_start_a_service_fails(self, service):
+        service_name = 'foo-service'
+        service.side_effect = [False]
+        self.assertFalse(host.service_start(service_name))
+
+        service.assert_called_with('start', service_name)
+
+    @patch.object(host, 'service')
+    def test_stop_a_service_fails(self, service):
+        service_name = 'foo-service'
+        service.side_effect = [False]
+        self.assertFalse(host.service_stop(service_name))
+
+        service.assert_called_with('stop', service_name)
+
+    @patch.object(host, 'service')
+    def test_restart_a_service_fails(self, service):
+        service_name = 'foo-service'
+        service.side_effect = [False]
+        self.assertFalse(host.service_restart(service_name))
+
+        service.assert_called_with('restart', service_name)
+
+    @patch.object(host, 'service')
+    def test_reload_a_service_fails(self, service):
+        service_name = 'foo-service'
+        service.side_effect = [False]
+        self.assertFalse(host.service_reload(service_name))
+
+        service.assert_called_with('reload', service_name)
+
+    @patch.object(host, 'service')
+    def test_failed_reload_restarts_a_service_fails(self, service):
+        service_name = 'foo-service'
+        service.side_effect = [False, False]
+        self.assertFalse(
+            host.service_reload(service_name, restart_on_failure=True))
+
+        service.assert_has_calls([
+            call('reload', service_name),
+            call('restart', service_name)
+        ])
 
     @patch('subprocess.check_output')
     def test_service_running_on_stopped_service(self, check_output):