Use strict=True instead of `is_valid_boolstr`

Oslo's `bool_from_string` learned the `strict` keyword which allows
callers to detect invalid boolean values, so we can use that instead
of having a new Nova-specific function.

Change-Id: I61bfa4029897c7304bd54d6cdae9f9a9bc4c1f78

7 files changed, 128 insertions, 48 deletions

diff --git a/nova/api/openstack/compute/contrib/volumes.py b/nova/api/openstack/compute/contrib/volumes.py
index 7410543d5..3ede9e4a4 100644
--- a/nova/api/openstack/compute/contrib/volumes.py
+++ b/nova/api/openstack/compute/contrib/volumes.py
@@ -27,7 +27,6 @@ from nova import exception
 from nova.openstack.common import log as logging
 from nova.openstack.common import strutils
 from nova.openstack.common import uuidutils
-from nova import utils
 from nova import volume
 
 LOG = logging.getLogger(__name__)
@@ -617,27 +616,26 @@ class SnapshotController(wsgi.Controller):
         snapshot = body['snapshot']
         volume_id = snapshot['volume_id']
 
-        force = snapshot.get('force', False)
         LOG.audit(_("Create snapshot from volume %s"), volume_id,
-                context=context)
+                  context=context)
 
-        if not utils.is_valid_boolstr(force):
+        force = snapshot.get('force', False)
+        try:
+            force = strutils.bool_from_string(force, strict=True)
+        except ValueError:
             msg = _("Invalid value '%s' for force.") % force
             raise exception.InvalidParameterValue(err=msg)
 
-        if strutils.bool_from_string(force):
-            new_snapshot = self.volume_api.create_snapshot_force(context,
-                                        volume_id,
-                                        snapshot.get('display_name'),
-                                        snapshot.get('display_description'))
+        if force:
+            create_func = self.volume_api.create_snapshot_force
         else:
-            new_snapshot = self.volume_api.create_snapshot(context,
-                                        volume_id,
-                                        snapshot.get('display_name'),
-                                        snapshot.get('display_description'))
+            create_func = self.volume_api.create_snapshot
 
-        retval = _translate_snapshot_detail_view(context, new_snapshot)
+        new_snapshot = create_func(context, volume_id,
+                                   snapshot.get('display_name'),
+                                   snapshot.get('display_description'))
 
+        retval = _translate_snapshot_detail_view(context, new_snapshot)
         return {'snapshot': retval}
 
 
diff --git a/nova/compute/api.py b/nova/compute/api.py
index f47121c36..56100d962 100644
--- a/nova/compute/api.py
+++ b/nova/compute/api.py
@@ -476,6 +476,26 @@ class API(base.Base):
                 instance['uuid'], updates)
         return instance
 
+    def _check_config_drive(self, context, config_drive):
+        bool_like = True
+        try:
+            strutils.bool_from_string(config_drive, strict=True)
+        except ValueError:
+            bool_like = False
+
+        if config_drive is None:
+            return None, None
+        elif bool_like and config_drive not in (0, 1, '0', '1'):
+            # NOTE(sirp): '0' and '1' could be a bool value or an ID.  Since
+            # there are many other ways to specify bools (e.g. 't', 'f'), it's
+            # better to treat as an ID.
+            return None, config_drive
+        else:
+            cd_image_service, config_drive_id = \
+                glance.get_remote_image_service(context, config_drive)
+            cd_image_service.show(context, config_drive_id)
+            return config_drive_id, None
+
     def _validate_and_provision_instance(self, context, instance_type,
                                          image_href, kernel_id, ramdisk_id,
                                          min_count, max_count,
@@ -557,17 +577,8 @@ class API(base.Base):
             kernel_id, ramdisk_id = self._handle_kernel_and_ramdisk(
                     context, kernel_id, ramdisk_id, image)
 
-            # Handle config_drive
-            config_drive_id = None
-            if config_drive and not utils.is_valid_boolstr(config_drive):
-                # config_drive is volume id
-                config_drive_id = config_drive
-                config_drive = None
-
-                # Ensure config_drive image exists
-                cd_image_service, config_drive_id = \
-                    glance.get_remote_image_service(context, config_drive_id)
-                cd_image_service.show(context, config_drive_id)
+            config_drive_id, config_drive = self._check_config_drive(
+                context, config_drive)
 
             if key_data is None and key_name:
                 key_pair = self.db.key_pair_get(context, context.user_id,
diff --git a/nova/compute/flavors.py b/nova/compute/flavors.py
index 2958769e1..58dcd3fa5 100644
--- a/nova/compute/flavors.py
+++ b/nova/compute/flavors.py
@@ -123,10 +123,11 @@ def create(name, memory, vcpus, root_gb, ephemeral_gb=0, flavorid=None,
     kwargs['flavorid'] = unicode(flavorid)
 
     # ensure is_public attribute is boolean
-    if not utils.is_valid_boolstr(is_public):
-        msg = _("is_public must be a boolean")
-        raise exception.InvalidInput(reason=msg)
-    kwargs['is_public'] = strutils.bool_from_string(is_public)
+    try:
+        kwargs['is_public'] = strutils.bool_from_string(
+            is_public, strict=True)
+    except ValueError:
+        raise exception.InvalidInput(reason=_("is_public must be a boolean"))
 
     try:
         return db.instance_type_create(context.get_admin_context(), kwargs)
diff --git a/nova/tests/api/openstack/compute/contrib/test_volumes.py b/nova/tests/api/openstack/compute/contrib/test_volumes.py
index d1d7210f0..617b60311 100644
--- a/nova/tests/api/openstack/compute/contrib/test_volumes.py
+++ b/nova/tests/api/openstack/compute/contrib/test_volumes.py
@@ -81,6 +81,16 @@ def fake_detach_volume(self, context, instance, volume):
     return()
 
 
+def fake_create_snapshot(self, context, volume, name, description):
+    return({'id': 123,
+            'volume_id': 'fakeVolId',
+            'status': 'available',
+            'volume_size': 123,
+            'created_at': '2013-01-01 00:00:01',
+            'display_name': 'myVolumeName',
+            'display_description': 'myVolumeDescription'})
+
+
 def fake_get_instance_bdms(self, context, instance):
     return([{'id': 1,
              'instance_uuid': instance['uuid'],
@@ -668,3 +678,29 @@ class UnprocessableSnapshotTestCase(CommonUnprocessableEntityTestCase,
     resource = 'os-snapshots'
     entity_name = 'snapshot'
     controller_cls = volumes.SnapshotController
+
+
+class CreateSnapshotTestCase(test.TestCase):
+    def setUp(self):
+        super(CreateSnapshotTestCase, self).setUp()
+        self.controller = volumes.SnapshotController()
+        self.stubs.Set(cinder.API, 'get', fake_get_volume)
+        self.stubs.Set(cinder.API, 'create_snapshot_force',
+                       fake_create_snapshot)
+        self.stubs.Set(cinder.API, 'create_snapshot', fake_create_snapshot)
+        self.req = fakes.HTTPRequest.blank('/v2/fake/os-snapshots')
+        self.req.method = 'POST'
+        self.body = {'snapshot': {'volume_id': 1}}
+
+    def test_force_true(self):
+        self.body['snapshot']['force'] = 'True'
+        self.controller.create(self.req, body=self.body)
+
+    def test_force_false(self):
+        self.body['snapshot']['force'] = 'f'
+        self.controller.create(self.req, body=self.body)
+
+    def test_force_invalid(self):
+        self.body['snapshot']['force'] = 'foo'
+        self.assertRaises(exception.InvalidParameterValue,
+                          self.controller.create, self.req, body=self.body)
diff --git a/nova/tests/compute/test_compute.py b/nova/tests/compute/test_compute.py
index 094b348a2..22dd7bd2f 100644
--- a/nova/tests/compute/test_compute.py
+++ b/nova/tests/compute/test_compute.py
@@ -8851,3 +8851,56 @@ class GetAndCheckImageMetadataTest(test.TestCase):
         self.assertRaises(exception.ImageTooLarge,
                 self.compute._get_and_check_image_metadata,
                 self.context, instance)
+
+
+class CheckConfigDriveTestCase(test.TestCase):
+    # NOTE(sirp): `TestCase` is far too heavyweight for this test, this should
+    # probably derive from a `test.FastTestCase` that omits DB and env
+    # handling
+    def setUp(self):
+        super(CheckConfigDriveTestCase, self).setUp()
+        self.compute_api = compute.API()
+        self.context = context.RequestContext(
+                'fake_user_id', 'fake_project_id')
+
+        self.called = called = {'show': False}
+
+        def fake_get_remote_image_service(context, image_id):
+            class FakeGlance(object):
+                def show(self, context, image_id):
+                    called['show'] = True
+
+            return FakeGlance(), image_id
+
+        self.stubs.Set(glance, 'get_remote_image_service',
+                       fake_get_remote_image_service)
+
+    def tearDown(self):
+        self.stubs.UnsetAll()
+        super(CheckConfigDriveTestCase, self).tearDown()
+
+    def assertCheck(self, expected, config_drive):
+        self.assertEqual(expected,
+                         self.compute_api._check_config_drive(
+                             self.context, config_drive))
+
+    def test_value_is_none(self):
+        self.assertFalse(self.called['show'])
+        self.assertCheck((None, None), None)
+        self.assertFalse(self.called['show'])
+
+    def test_value_is_bool_like_string(self):
+        self.assertCheck((None, 'True'), 'True')
+        self.assertCheck((None, 'yes'), 'yes')
+
+    def test_bool_string_or_id(self):
+        # NOTE(sirp): '0' and '1' could be a bool value or an ID.  Since there
+        # are many other ways to specify bools (e.g. 't', 'f'), it's better to
+        # treat as an ID.
+        self.assertCheck((0, None), 0)
+        self.assertCheck((1, None), 1)
+        self.assertCheck(('0', None), '0')
+        self.assertCheck(('1', None), '1')
+
+    def test_value_is_image_id(self):
+        self.assertCheck((2, None), 2)
diff --git a/nova/tests/test_utils.py b/nova/tests/test_utils.py
index 39db03f3e..a2abedda8 100644
--- a/nova/tests/test_utils.py
+++ b/nova/tests/test_utils.py
@@ -360,19 +360,6 @@ class GenericUtilsTestCase(test.TestCase):
         h2 = hashlib.sha1(data).hexdigest()
         self.assertEquals(h1, h2)
 
-    def test_is_valid_boolstr(self):
-        self.assertTrue(utils.is_valid_boolstr('true'))
-        self.assertTrue(utils.is_valid_boolstr('false'))
-        self.assertTrue(utils.is_valid_boolstr('yes'))
-        self.assertTrue(utils.is_valid_boolstr('no'))
-        self.assertTrue(utils.is_valid_boolstr('y'))
-        self.assertTrue(utils.is_valid_boolstr('n'))
-        self.assertTrue(utils.is_valid_boolstr('1'))
-        self.assertTrue(utils.is_valid_boolstr('0'))
-
-        self.assertFalse(utils.is_valid_boolstr('maybe'))
-        self.assertFalse(utils.is_valid_boolstr('only on tuesdays'))
-
     def test_is_valid_ipv4(self):
         self.assertTrue(utils.is_valid_ipv4('127.0.0.1'))
         self.assertFalse(utils.is_valid_ipv4('::1'))
diff --git a/nova/utils.py b/nova/utils.py
index 3020781c8..3e9d8c597 100644
--- a/nova/utils.py
+++ b/nova/utils.py
@@ -605,12 +605,6 @@ def is_int_like(val):
         return False
 
 
-def is_valid_boolstr(val):
-    """Check if the provided string is a valid bool string or not."""
-    boolstrs = ('true', 'false', 'yes', 'no', 'y', 'n', '1', '0')
-    return str(val).lower() in boolstrs
-
-
 def is_valid_ipv4(address):
     """Verify that address represents a valid IPv4 address."""
     try: