From 8fd2ac3ba1c48189d60d73043c4b6868de2633f2 Mon Sep 17 00:00:00 2001
From: Ruby Loo <rloo@yahoo-inc.com>
Date: Thu, 2 Apr 2015 18:17:33 +0000
Subject: [PATCH] Consistent and more valid strings for Booleans
The set of valid input strings for Boolean values was inconsistent among
the subcommands of the Ironic CLI. egs:
-for node-set-console-mode: 'true', 'false'
-for node-set-maintenance: 'on', 'off', 'true', 'false', 'True', 'False'
This change allows the same set of valid values for all the subcommands:
'0', '1', 'f', 'false', 'n', 'no', 'off', 'on', 't', 'true', 'y', 'yes'.
For invalid strings, we output the subcommand usage along with the
argument, invalid string, and valid string choices.
At the API level, if NodeManager.set_maintenance() is passed an invalid
state (maintenance mode) value, an InvalidAttribute exception is raised.
Change-Id: I541695388489cdc640265df8ee6a9999e1442870
Closes-Bug: #1415943
---
ironicclient/common/utils.py | 28 ++++++++++
ironicclient/shell.py | 6 ++-
ironicclient/tests/unit/test_utils.py | 19 +++++++
ironicclient/tests/unit/v1/test_node.py | 54 ++++++++++++++++---
ironicclient/tests/unit/v1/test_node_shell.py | 40 ++++++++++++--
ironicclient/v1/node.py | 48 ++++++++++++++---
ironicclient/v1/node_shell.py | 26 ++++-----
7 files changed, 188 insertions(+), 33 deletions(-)
diff --git a/ironicclient/common/utils.py b/ironicclient/common/utils.py
index 75378b019..4affab94c 100644
--- a/ironicclient/common/utils.py
+++ b/ironicclient/common/utils.py
@@ -26,6 +26,7 @@ import subprocess
import tempfile
from oslo_utils import importutils
+from oslo_utils import strutils
from ironicclient.common.i18n import _
from ironicclient import exc
@@ -249,3 +250,30 @@ def check_empty_arg(arg, arg_descriptor):
if not arg.strip():
raise exc.CommandError(_('%(arg)s cannot be empty or only have blank'
' spaces') % {'arg': arg_descriptor})
+
+
+def bool_argument_value(arg_name, bool_str, strict=True, default=False):
+ """Returns the Boolean represented by bool_str.
+
+ Returns the Boolean value for the argument named arg_name. The value is
+ represented by the string bool_str. If the string is an invalid Boolean
+ string: if strict is True, a CommandError exception is raised; otherwise
+ the default value is returned.
+
+ :param arg_name: The name of the argument
+ :param bool_str: The string representing a Boolean value
+ :param strict: Used if the string is invalid. If True, raises an exception.
+ If False, returns the default value.
+ :param default: The default value to return if the string is invalid
+ and not strict
+ :returns: the Boolean value represented by bool_str or the default value
+ if bool_str is invalid and strict is False
+ :raises CommandError: if bool_str is an invalid Boolean string
+
+ """
+ try:
+ val = strutils.bool_from_string(bool_str, strict, default)
+ except ValueError as e:
+ raise exc.CommandError(_("argument %(arg)s: %(err)s.")
+ % {'arg': arg_name, 'err': e})
+ return val
diff --git a/ironicclient/shell.py b/ironicclient/shell.py
index f972c26fe..92bd5179d 100644
--- a/ironicclient/shell.py
+++ b/ironicclient/shell.py
@@ -253,7 +253,8 @@ class IronicShell(object):
parser = self.get_base_parser()
self.subcommands = {}
- subparsers = parser.add_subparsers(metavar='<subcommand>')
+ subparsers = parser.add_subparsers(metavar='<subcommand>',
+ dest='subparser_name')
submodule = utils.import_versioned_module(version, 'shell')
submodule.enhance_parser(parser, subparsers, self.subcommands)
utils.define_commands_from_module(subparsers, self, self.subcommands)
@@ -536,6 +537,9 @@ class IronicShell(object):
args.func(client, args)
except exc.Unauthorized:
raise exc.CommandError(_("Invalid OpenStack Identity credentials"))
+ except exc.CommandError as e:
+ subcommand_parser = self.subcommands[args.subparser_name]
+ subcommand_parser.error(e)
@cliutils.arg('command', metavar='<subcommand>', nargs='?',
help='Display help for <subcommand>')
diff --git a/ironicclient/tests/unit/test_utils.py b/ironicclient/tests/unit/test_utils.py
index 4ce123cac..f395a87da 100644
--- a/ironicclient/tests/unit/test_utils.py
+++ b/ironicclient/tests/unit/test_utils.py
@@ -95,6 +95,25 @@ class UtilsTest(test_utils.BaseTestCase):
self.assertRaises(exc.CommandError,
utils.split_and_deserialize, 'foo:bar')
+ def test_bool_arg_value(self):
+ self.assertTrue(utils.bool_argument_value('arg', 'y', strict=True))
+ self.assertTrue(utils.bool_argument_value('arg', 'TrUe', strict=True))
+ self.assertTrue(utils.bool_argument_value('arg', '1', strict=True))
+ self.assertTrue(utils.bool_argument_value('arg', 1, strict=True))
+
+ self.assertFalse(utils.bool_argument_value('arg', '0', strict=True))
+ self.assertFalse(utils.bool_argument_value('arg', 'No', strict=True))
+
+ self.assertRaises(exc.CommandError,
+ utils.bool_argument_value, 'arg', 'ee', strict=True)
+
+ self.assertFalse(utils.bool_argument_value('arg', 'ee', strict=False))
+ self.assertTrue(utils.bool_argument_value('arg', 'ee', strict=False,
+ default=True))
+ # No check that default is a Boolean...
+ self.assertEqual('foo', utils.bool_argument_value('arg', 'ee',
+ strict=False, default='foo'))
+
class CommonParamsForListTest(test_utils.BaseTestCase):
def setUp(self):
diff --git a/ironicclient/tests/unit/v1/test_node.py b/ironicclient/tests/unit/v1/test_node.py
index e30fdfef4..8fd9bfaf7 100644
--- a/ironicclient/tests/unit/v1/test_node.py
+++ b/ironicclient/tests/unit/v1/test_node.py
@@ -66,6 +66,8 @@ CONSOLE_DATA_ENABLED = {'console_enabled': True,
'console_info': {'test-console': 'test-console-data'}}
CONSOLE_DATA_DISABLED = {'console_enabled': False, 'console_info': None}
+CONSOLE_ENABLE = 'true'
+
BOOT_DEVICE = {'boot_device': 'pxe', 'persistent': False}
SUPPORTED_BOOT_DEVICE = {'supported_boot_devices': ['pxe']}
@@ -232,7 +234,7 @@ fake_responses = {
CONSOLE_DATA_ENABLED,
),
'PUT': (
- {'enabled': 'true'},
+ {'enabled': CONSOLE_ENABLE},
None,
),
},
@@ -422,6 +424,15 @@ class NodeManagerTest(testtools.TestCase):
self.assertThat(nodes, HasLength(1))
self.assertEqual(NODE1['uuid'], getattr(nodes[0], 'uuid'))
+ def test_node_list_unassociated_string(self):
+ nodes = self.mgr.list(associated="False")
+ expect = [
+ ('GET', '/v1/nodes/?associated=False', {}, None),
+ ]
+ self.assertEqual(expect, self.api.calls)
+ self.assertThat(nodes, HasLength(1))
+ self.assertEqual(NODE1['uuid'], getattr(nodes[0], 'uuid'))
+
def test_node_list_maintenance(self):
nodes = self.mgr.list(maintenance=True)
expect = [
@@ -431,6 +442,15 @@ class NodeManagerTest(testtools.TestCase):
self.assertThat(nodes, HasLength(1))
self.assertEqual(NODE2['uuid'], getattr(nodes[0], 'uuid'))
+ def test_node_list_maintenance_string(self):
+ nodes = self.mgr.list(maintenance="True")
+ expect = [
+ ('GET', '/v1/nodes/?maintenance=True', {}, None),
+ ]
+ self.assertEqual(expect, self.api.calls)
+ self.assertThat(nodes, HasLength(1))
+ self.assertEqual(NODE2['uuid'], getattr(nodes[0], 'uuid'))
+
def test_node_list_no_maintenance(self):
nodes = self.mgr.list(maintenance=False)
expect = [
@@ -621,6 +641,20 @@ class NodeManagerTest(testtools.TestCase):
self.assertEqual(expect, self.api.calls)
self.assertEqual(None, maintenance)
+ def test_node_set_maintenance_bad(self):
+ self.assertRaises(exc.InvalidAttribute, self.mgr.set_maintenance,
+ NODE1['uuid'], 'bad')
+
+ def test_node_set_maintenance_bool(self):
+ maintenance = self.mgr.set_maintenance(NODE1['uuid'], True,
+ maint_reason='reason')
+ body = {'reason': 'reason'}
+ expect = [
+ ('PUT', '/v1/nodes/%s/maintenance' % NODE1['uuid'], {}, body),
+ ]
+ self.assertEqual(expect, self.api.calls)
+ self.assertEqual(None, maintenance)
+
def test_node_set_power_state(self):
power_state = self.mgr.set_power_state(NODE1['uuid'], "on")
body = {'target': 'power on'}
@@ -705,13 +739,17 @@ class NodeManagerTest(testtools.TestCase):
sorted(states.to_dict().keys()))
def test_node_set_console_mode(self):
- enabled = 'true'
- self.mgr.set_console_mode(NODE1['uuid'], enabled)
- body = {'enabled': enabled}
- expect = [
- ('PUT', '/v1/nodes/%s/states/console' % NODE1['uuid'], {}, body),
- ]
- self.assertEqual(expect, self.api.calls)
+ global ENABLE
+ for enabled in ['true', True, 'False', False]:
+ self.api.calls = []
+ ENABLE = enabled
+ self.mgr.set_console_mode(NODE1['uuid'], enabled)
+ body = {'enabled': enabled}
+ expect = [
+ ('PUT', '/v1/nodes/%s/states/console' % NODE1['uuid'], {},
+ body),
+ ]
+ self.assertEqual(expect, self.api.calls)
def test_node_get_console(self):
info = self.mgr.get_console(NODE1['uuid'])
diff --git a/ironicclient/tests/unit/v1/test_node_shell.py b/ironicclient/tests/unit/v1/test_node_shell.py
index f54cfe6ba..dbbb01e90 100644
--- a/ironicclient/tests/unit/v1/test_node_shell.py
+++ b/ironicclient/tests/unit/v1/test_node_shell.py
@@ -222,7 +222,7 @@ class NodeShellTest(utils.BaseTestCase):
n_shell.do_node_set_maintenance(client_mock, args)
client_mock.node.set_maintenance.assert_called_once_with(
- 'node_uuid', 'true', maint_reason='reason')
+ 'node_uuid', True, maint_reason='reason')
def test_do_node_set_maintenance_false(self):
client_mock = mock.MagicMock()
@@ -234,7 +234,19 @@ class NodeShellTest(utils.BaseTestCase):
n_shell.do_node_set_maintenance(client_mock, args)
client_mock.node.set_maintenance.assert_called_once_with(
- 'node_uuid', 'false', maint_reason=None)
+ 'node_uuid', False, maint_reason=None)
+
+ def test_do_node_set_maintenance_bad(self):
+ client_mock = mock.MagicMock()
+ args = mock.MagicMock()
+ args.node = 'node_uuid'
+ args.maintenance_mode = 'yuck'
+ # NOTE(jroll) None is the default. <3 mock.
+ args.reason = None
+
+ self.assertRaises(exceptions.CommandError,
+ n_shell.do_node_set_maintenance, client_mock, args)
+ self.assertFalse(client_mock.node.set_maintenance.called)
def test_do_node_set_maintenance_false_with_reason_fails(self):
client_mock = mock.MagicMock()
@@ -256,7 +268,7 @@ class NodeShellTest(utils.BaseTestCase):
n_shell.do_node_set_maintenance(client_mock, args)
client_mock.node.set_maintenance.assert_called_once_with(
- 'node_uuid', 'on', maint_reason='reason')
+ 'node_uuid', True, maint_reason='reason')
def test_do_node_set_maintenance_off(self):
client_mock = mock.MagicMock()
@@ -268,7 +280,7 @@ class NodeShellTest(utils.BaseTestCase):
n_shell.do_node_set_maintenance(client_mock, args)
client_mock.node.set_maintenance.assert_called_once_with(
- 'node_uuid', 'off', maint_reason=None)
+ 'node_uuid', False, maint_reason=None)
def test_do_node_set_maintenance_off_with_reason_fails(self):
client_mock = mock.MagicMock()
@@ -403,6 +415,26 @@ class NodeShellTest(utils.BaseTestCase):
client_mock.node.set_provision_state.assert_called_once_with(
'node_uuid', 'provide', configdrive=None)
+ def test_do_node_set_console_mode(self):
+ client_mock = mock.MagicMock()
+ args = mock.MagicMock()
+ args.node = 'node_uuid'
+ args.enabled = 'true'
+
+ n_shell.do_node_set_console_mode(client_mock, args)
+ client_mock.node.set_console_mode.assert_called_once_with(
+ 'node_uuid', True)
+
+ def test_do_node_set_console_mode_bad(self):
+ client_mock = mock.MagicMock()
+ args = mock.MagicMock()
+ args.node = 'node_uuid'
+ args.enabled = 'yuck'
+
+ self.assertRaises(exceptions.CommandError,
+ n_shell.do_node_set_console_mode, client_mock, args)
+ self.assertFalse(client_mock.node.set_console_mode.called)
+
def test_do_node_set_boot_device(self):
client_mock = mock.MagicMock()
args = mock.MagicMock()
diff --git a/ironicclient/v1/node.py b/ironicclient/v1/node.py
index f351dea60..5774d98b7 100644
--- a/ironicclient/v1/node.py
+++ b/ironicclient/v1/node.py
@@ -14,6 +14,8 @@
import os
+from oslo_utils import strutils
+
from ironicclient.common import base
from ironicclient.common.i18n import _
from ironicclient.common import utils
@@ -39,11 +41,15 @@ class NodeManager(base.Manager):
detail=False, sort_key=None, sort_dir=None):
"""Retrieve a list of nodes.
- :param associated: Optional, boolean whether to return a list of
- associated or unassociated nodes.
- :param maintenance: Optional, boolean value that indicates whether
- to get nodes in maintenance mode ("True"), or not
- in maintenance mode ("False").
+ :param associated: Optional. Either a Boolean or a string
+ representation of a Boolean that indicates whether
+ to return a list of associated (True or "True") or
+ unassociated (False or "False") nodes.
+ :param maintenance: Optional. Either a Boolean or a string
+ representation of a Boolean that indicates whether
+ to return nodes in maintenance mode (True or
+ "True"), or not in maintenance mode (False or
+ "False").
:param marker: Optional, the UUID of a node, eg the last
node from a previous result set. Return
the next result set.
@@ -196,11 +202,32 @@ class NodeManager(base.Manager):
_('Unknown HTTP method: %s') % http_method)
def set_maintenance(self, node_id, state, maint_reason=None):
+ """Set the maintenance mode for the node.
+
+ :param node_id: The UUID of the node.
+ :param state: the maintenance mode; either a Boolean or a string
+ representation of a Boolean (eg, 'true', 'on', 'false',
+ 'off'). True to put the node in maintenance mode; False
+ to take the node out of maintenance mode.
+ :param maint_reason: Optional string. Reason for putting node
+ into maintenance mode.
+ :raises: InvalidAttribute if state is an invalid string (that doesn't
+ represent a Boolean).
+
+ """
+ if isinstance(state, bool):
+ maintenance_mode = state
+ else:
+ try:
+ maintenance_mode = strutils.bool_from_string(state, True)
+ except ValueError as e:
+ raise exc.InvalidAttribute(_("Argument 'state': %(err)s") %
+ {'err': e})
path = "%s/maintenance" % node_id
- if state in ('true', 'on'):
+ if maintenance_mode:
reason = {'reason': maint_reason}
return self._update(self._path(path), reason, method='PUT')
- if state in ('false', 'off'):
+ else:
return self._delete(self._path(path))
def set_power_state(self, node_id, state):
@@ -241,6 +268,13 @@ class NodeManager(base.Manager):
return info.to_dict()
def set_console_mode(self, node_uuid, enabled):
+ """Set the console mode for the node.
+
+ :param node_uuid: The UUID of the node.
+ :param enabled: Either a Boolean or a string representation of a
+ Boolean. True to enable the console; False to disable.
+
+ """
path = "%s/states/console" % node_uuid
target = {'enabled': enabled}
return self._update(self._path(path), target, method='PUT')
diff --git a/ironicclient/v1/node_shell.py b/ironicclient/v1/node_shell.py
index dedefd7f2..622a63642 100644
--- a/ironicclient/v1/node_shell.py
+++ b/ironicclient/v1/node_shell.py
@@ -74,12 +74,10 @@ def do_node_show(cc, args):
@cliutils.arg(
'--maintenance',
metavar='<boolean>',
- choices=['true', 'True', 'false', 'False'],
help="List nodes in maintenance mode: 'true' or 'false'.")
@cliutils.arg(
'--associated',
metavar='<boolean>',
- choices=['true', 'True', 'false', 'False'],
help="List nodes by instance association: 'true' or 'false'.")
@cliutils.arg(
'--detail',
@@ -91,9 +89,11 @@ def do_node_list(cc, args):
"""List the nodes which are registered with the Ironic service."""
params = {}
if args.associated is not None:
- params['associated'] = args.associated
+ params['associated'] = utils.bool_argument_value("--associated",
+ args.associated)
if args.maintenance is not None:
- params['maintenance'] = args.maintenance
+ params['maintenance'] = utils.bool_argument_value("--maintenance",
+ args.maintenance)
params['detail'] = args.detail
if args.detail:
@@ -291,20 +291,21 @@ def do_node_port_list(cc, args):
@cliutils.arg(
'maintenance_mode',
metavar='<maintenance-mode>',
- choices=['true', 'True', 'false', 'False', 'on', 'off'],
help="'true' or 'false'; 'on' or 'off'.")
@cliutils.arg(
'--reason',
metavar='<reason>',
default=None,
- help=('Reason for setting maintenance mode to "true" or "on";'
- ' not valid when setting to "false" or "off".'))
+ help=("Reason for setting maintenance mode to 'true' or 'on';"
+ " not valid when setting to 'false' or 'off'."))
def do_node_set_maintenance(cc, args):
"""Enable or disable maintenance mode for a node."""
- if args.reason and args.maintenance_mode.lower() in ('false', 'off'):
+ maintenance_mode = utils.bool_argument_value("<maintenance-mode>",
+ args.maintenance_mode)
+ if args.reason and not maintenance_mode:
raise exceptions.CommandError(_('Cannot set "reason" when turning off '
'maintenance mode.'))
- cc.node.set_maintenance(args.node, args.maintenance_mode.lower(),
+ cc.node.set_maintenance(args.node, maintenance_mode,
maint_reason=args.reason)
@@ -370,12 +371,11 @@ def do_node_get_console(cc, args):
@cliutils.arg(
'enabled',
metavar='<enabled>',
- choices=['true', 'false'],
- help="Enable or disable console access for a node. Supported options are: "
- "'true' or 'false'.")
+ help="Enable or disable console access for a node: 'true' or 'false'.")
def do_node_set_console_mode(cc, args):
"""Enable or disable serial console access for a node."""
- cc.node.set_console_mode(args.node, args.enabled)
+ enable = utils.bool_argument_value("<enabled>", args.enabled)
+ cc.node.set_console_mode(args.node, enable)
@cliutils.arg('node', metavar='<node>', help="Name or UUID of the node.")