Log hint when --enable present with --disable-reason

--enable and --disable-reason should be mutually exclusive in "compute
service set" command, but now when they are present at the same time,
--disable-reason would be ignored silently. Fix these and add some hints
about --disable-reason argument is ignored in this situation.

Change-Id: I43254b6bc40fcae4fd0dc3457f26fad84c267072
Closes-Bug: #1556801

2 files changed, 71 insertions, 11 deletions

diff --git a/openstackclient/compute/v2/service.py b/openstackclient/compute/v2/service.py
index 68037c94..2b51af3d 100644
--- a/openstackclient/compute/v2/service.py
+++ b/openstackclient/compute/v2/service.py
@@ -17,6 +17,7 @@
 
 from openstackclient.common import command
 from openstackclient.common import utils
+from openstackclient.i18n import _  # noqa
 
 
 class DeleteService(command.Command):
@@ -117,8 +118,8 @@ class SetService(command.Command):
             "--disable-reason",
             default=None,
             metavar="<reason>",
-            help="Reason for disabling the service (in quotas)"
-        )
+            help="Reason for disabling the service (in quotas).  Note that "
+                 "when the service is enabled, this option is ignored.")
         return parser
 
     def take_action(self, parsed_args):
@@ -133,4 +134,8 @@ class SetService(command.Command):
             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)
diff --git a/openstackclient/tests/compute/v2/test_service.py b/openstackclient/tests/compute/v2/test_service.py
index 2feaf156..db097204 100644
--- a/openstackclient/tests/compute/v2/test_service.py
+++ b/openstackclient/tests/compute/v2/test_service.py
@@ -14,6 +14,7 @@
 #
 
 import copy
+import mock
 
 from openstackclient.compute.v2 import service
 from openstackclient.tests.compute.v2 import fakes as compute_fakes
@@ -138,14 +139,14 @@ class TestServiceSet(TestService):
 
     def test_service_set_enable(self):
         arglist = [
+            '--enable',
             compute_fakes.service_host,
             compute_fakes.service_binary,
-            '--enable',
         ]
         verifylist = [
+            ('enabled', True),
             ('host', compute_fakes.service_host),
             ('service', compute_fakes.service_binary),
-            ('enabled', True),
         ]
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
 
@@ -159,14 +160,14 @@ class TestServiceSet(TestService):
 
     def test_service_set_disable(self):
         arglist = [
+            '--disable',
             compute_fakes.service_host,
             compute_fakes.service_binary,
-            '--disable',
         ]
         verifylist = [
+            ('enabled', False),
             ('host', compute_fakes.service_host),
             ('service', compute_fakes.service_binary),
-            ('enabled', False),
         ]
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
 
@@ -181,17 +182,16 @@ class TestServiceSet(TestService):
     def test_service_set_disable_with_reason(self):
         reason = 'earthquake'
         arglist = [
+            '--disable',
+            '--disable-reason', reason,
             compute_fakes.service_host,
             compute_fakes.service_binary,
-            '--disable',
-            '--disable-reason',
-            reason
         ]
         verifylist = [
+            ('enabled', False),
+            ('disable_reason', reason),
             ('host', compute_fakes.service_host),
             ('service', compute_fakes.service_binary),
-            ('enabled', False),
-            ('disable_reason', reason)
         ]
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
 
@@ -203,3 +203,58 @@ class TestServiceSet(TestService):
             reason
         )
         self.assertIsNone(result)
+
+    def test_service_set_only_with_disable_reason(self):
+        reason = 'earthquake'
+        arglist = [
+            '--disable-reason', reason,
+            compute_fakes.service_host,
+            compute_fakes.service_binary,
+        ]
+        verifylist = [
+            ('enabled', True),
+            ('disable_reason', reason),
+            ('host', compute_fakes.service_host),
+            ('service', compute_fakes.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(
+            compute_fakes.service_host,
+            compute_fakes.service_binary
+        )
+        self.assertIsNone(result)
+
+    def test_service_set_enable_with_disable_reason(self):
+        reason = 'earthquake'
+        arglist = [
+            '--enable',
+            '--disable-reason', reason,
+            compute_fakes.service_host,
+            compute_fakes.service_binary,
+        ]
+        verifylist = [
+            ('enabled', True),
+            ('disable_reason', reason),
+            ('host', compute_fakes.service_host),
+            ('service', compute_fakes.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(
+            compute_fakes.service_host,
+            compute_fakes.service_binary
+        )
+        self.assertIsNone(result)