From ac1d86c34333780e30b9393d155ae84a769ac222 Mon Sep 17 00:00:00 2001 From: Tang Chen Date: Wed, 25 May 2016 11:12:57 +0800 Subject: [PATCH] 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 --- .../command-objects/compute-service.rst | 5 +- openstackclient/compute/v2/service.py | 38 ++++++++----- .../tests/compute/v2/test_service.py | 57 +++++++++---------- .../service-set-option-61772a8940ad0778.yaml | 6 ++ 4 files changed, 57 insertions(+), 49 deletions(-) create mode 100644 releasenotes/notes/service-set-option-61772a8940ad0778.yaml diff --git a/doc/source/command-objects/compute-service.rst b/doc/source/command-objects/compute-service.rst index bda64c811d..71225bc37a 100644 --- a/doc/source/command-objects/compute-service.rst +++ b/doc/source/command-objects/compute-service.rst @@ -63,7 +63,7 @@ Set service command .. _compute-service-set: .. option:: --enable - Enable service (default) + Enable service .. option:: --disable @@ -71,8 +71,7 @@ Set service command .. option:: --disable-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:: diff --git a/openstackclient/compute/v2/service.py b/openstackclient/compute/v2/service.py index 6093b8ce17..e7966a8a1a 100644 --- a/openstackclient/compute/v2/service.py +++ b/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,23 +111,20 @@ class SetService(command.Command): 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="", - 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 @@ -134,16 +132,26 @@ class SetService(command.Command): 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) diff --git a/openstackclient/tests/compute/v2/test_service.py b/openstackclient/tests/compute/v2/test_service.py index 7a5a840f05..cf53497878 100644 --- a/openstackclient/tests/compute/v2/test_service.py +++ b/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,6 +127,23 @@ class TestServiceSet(TestService): 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', @@ -135,7 +151,7 @@ class TestServiceSet(TestService): self.service.binary, ] verifylist = [ - ('enabled', True), + ('enable', True), ('host', self.service.host), ('service', self.service.binary), ] @@ -156,7 +172,7 @@ class TestServiceSet(TestService): self.service.binary, ] verifylist = [ - ('enabled', False), + ('disable', True), ('host', self.service.host), ('service', self.service.binary), ] @@ -179,7 +195,7 @@ class TestServiceSet(TestService): self.service.binary, ] verifylist = [ - ('enabled', False), + ('disable', True), ('disable_reason', reason), ('host', self.service.host), ('service', self.service.binary), @@ -203,24 +219,13 @@ class TestServiceSet(TestService): 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 @@ class TestServiceSet(TestService): 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) diff --git a/releasenotes/notes/service-set-option-61772a8940ad0778.yaml b/releasenotes/notes/service-set-option-61772a8940ad0778.yaml new file mode 100644 index 0000000000..02c7fcba2e --- /dev/null +++ b/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.