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

7 files changed, 188 insertions, 33 deletions

diff --git a/ironicclient/common/utils.py b/ironicclient/common/utils.py
index 75378b0..4affab9 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 f972c26..92bd517 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 4ce123c..f395a87 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 e30fdfe..8fd9bfa 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 f54cfe6..dbbb01e 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 f351dea..5774d98 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 dedefd7..622a636 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.")