Refactor SetService --enable/disable option

This patch changes the following:

1. --enable/disable option should follow the rules
   in the doc below:
   http://docs.openstack.org/developer/python-openstackclient/command-options.html#boolean-options

2. "--disable-resion" is specified but not "--disable",
   an exception is raised instead of igoring
   "--disable-reason" option.

Change-Id: I92e9234111e661bfe7119a8e19389a87c874ab0c

Author: Tang Chen

doc/source/command-objects/compute-service.rst

@@ -63,16 +63,15 @@ Set service command
 .. _compute-service-set:
 .. option:: --enable
 
-    Enable service (default)
+    Enable service
 
 .. option:: --disable
 
     Disable service
 
 .. option:: --disable-reason <reason>
 
-    Reason for disabling the service (in quotes).  Note that when the service
-    is enabled, this option is ignored.
+    Reason for disabling the service (in quotes). Should be used with --disable option.
 
 .. describe:: <host>
 

openstackclient/compute/v2/service.py

@@ -16,6 +16,7 @@
 """Service action implementations"""
 
 from openstackclient.common import command
+from openstackclient.common import exceptions
 from openstackclient.common import utils
 from openstackclient.i18n import _
 
@@ -110,40 +111,47 @@ def get_parser(self, prog_name):
         enabled_group = parser.add_mutually_exclusive_group()
         enabled_group.add_argument(
             "--enable",
-            dest="enabled",
-            default=True,
             action="store_true",
-            help=_("Enable a service (default)")
+            help=_("Enable service")
         )
         enabled_group.add_argument(
             "--disable",
-            dest="enabled",
-            action="store_false",
-            help=_("Disable a service")
+            action="store_true",
+            help=_("Disable service")
         )
         parser.add_argument(
             "--disable-reason",
             default=None,
             metavar="<reason>",
-            help=_("Reason for disabling the service (in quotas).  Note that "
-                   "when the service is enabled, this option is ignored.")
+            help=_("Reason for disabling the service (in quotas). "
+                   "Should be used with --disable option.")
         )
         return parser
 
     def take_action(self, parsed_args):
         compute_client = self.app.client_manager.compute
         cs = compute_client.services
 
-        if not parsed_args.enabled:
+        if (parsed_args.enable or not parsed_args.disable) and \
+                parsed_args.disable_reason:
+            msg = _("Cannot specify option --disable-reason without "
+                    "--disable specified.")
+            raise exceptions.CommandError(msg)
+
+        enabled = None
+        if parsed_args.enable:
+            enabled = True
+        if parsed_args.disable:
+            enabled = False
+
+        if enabled is None:
+            return
+        elif enabled:
+            cs.enable(parsed_args.host, parsed_args.service)
+        else:
             if parsed_args.disable_reason:
                 cs.disable_log_reason(parsed_args.host,
                                       parsed_args.service,
                                       parsed_args.disable_reason)
             else:
                 cs.disable(parsed_args.host, parsed_args.service)
-        else:
-            if parsed_args.disable_reason:
-                msg = _("argument --disable-reason has been ignored")
-                self.log.info(msg)
-
-            cs.enable(parsed_args.host, parsed_args.service)

openstackclient/tests/compute/v2/test_service.py

@@ -13,8 +13,7 @@
 #   under the License.
 #
 
-import mock
-
+from openstackclient.common import exceptions
 from openstackclient.compute.v2 import service
 from openstackclient.tests.compute.v2 import fakes as compute_fakes
 
@@ -128,14 +127,31 @@ def setUp(self):
 
         self.cmd = service.SetService(self.app, None)
 
+    def test_set_nothing(self):
+        arglist = [
+            self.service.host,
+            self.service.binary,
+        ]
+        verifylist = [
+            ('host', self.service.host),
+            ('service', self.service.binary),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+        result = self.cmd.take_action(parsed_args)
+
+        self.service_mock.enable.assert_not_called()
+        self.service_mock.disable.assert_not_called()
+        self.service_mock.disable_log_reason.assert_not_called()
+        self.assertIsNone(result)
+
     def test_service_set_enable(self):
         arglist = [
             '--enable',
             self.service.host,
             self.service.binary,
         ]
         verifylist = [
-            ('enabled', True),
+            ('enable', True),
             ('host', self.service.host),
             ('service', self.service.binary),
         ]
@@ -156,7 +172,7 @@ def test_service_set_disable(self):
             self.service.binary,
         ]
         verifylist = [
-            ('enabled', False),
+            ('disable', True),
             ('host', self.service.host),
             ('service', self.service.binary),
         ]
@@ -179,7 +195,7 @@ def test_service_set_disable_with_reason(self):
             self.service.binary,
         ]
         verifylist = [
-            ('enabled', False),
+            ('disable', True),
             ('disable_reason', reason),
             ('host', self.service.host),
             ('service', self.service.binary),
@@ -203,24 +219,13 @@ def test_service_set_only_with_disable_reason(self):
             self.service.binary,
         ]
         verifylist = [
-            ('enabled', True),
             ('disable_reason', reason),
             ('host', self.service.host),
             ('service', self.service.binary),
         ]
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
-
-        with mock.patch.object(self.cmd.log, 'info') as mock_log:
-            result = self.cmd.take_action(parsed_args)
-
-            msg = "argument --disable-reason has been ignored"
-            mock_log.assert_called_once_with(msg)
-
-        self.service_mock.enable.assert_called_with(
-            self.service.host,
-            self.service.binary
-        )
-        self.assertIsNone(result)
+        self.assertRaises(exceptions.CommandError, self.cmd.take_action,
+                          parsed_args)
 
     def test_service_set_enable_with_disable_reason(self):
         reason = 'earthquake'
@@ -231,21 +236,11 @@ def test_service_set_enable_with_disable_reason(self):
             self.service.binary,
         ]
         verifylist = [
-            ('enabled', True),
+            ('enable', True),
             ('disable_reason', reason),
             ('host', self.service.host),
             ('service', self.service.binary),
         ]
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
-
-        with mock.patch.object(self.cmd.log, 'info') as mock_log:
-            result = self.cmd.take_action(parsed_args)
-
-            msg = "argument --disable-reason has been ignored"
-            mock_log.assert_called_once_with(msg)
-
-        self.service_mock.enable.assert_called_with(
-            self.service.host,
-            self.service.binary
-        )
-        self.assertIsNone(result)
+        self.assertRaises(exceptions.CommandError, self.cmd.take_action,
+                          parsed_args)

releasenotes/notes/service-set-option-61772a8940ad0778.yaml

@@ -0,0 +1,6 @@
+---
+upgrade:
+   - An exception is not raised by command ``service set`` when nothing
+     specified. Instead, the service is not enabled by default. And if
+     ``--disable-resion`` is specified but not ``--disable``, an
+     exception will be raised.