Merge lp:~jaypipes/glance/bug756886 into lp:~glance-coresec/glance/cactus-trunk

[Jay Pipes]

Change parsing of headers to accept 'True', 'on', 1 for boolean truth values.

[Rick Harris (rconradharris)]

I'm not so sure I agree with the intent of this patch. The code doesn't match the docs; but, in this case, I think it might be the docs that are in the wrong.

I'm not sure why we would want to loosen our constraints around what's accepted by Glance's boolean fields. I know other projects do this, and it's arisen as a sort of anti-pattern to accept "1", "true" or "on" in a lot of config files. To me, though, a single "True" or "False" value is much less ambiguous and thus easier to communicate to users. ("Use true or false or it ain't gonna work!")

This might cause some issues when configs are more naturally expressed as "on" and "off". But the trade off of a single "Right Way" I think is worth the minor mismatch. Thoughts?

In terms of the code, looks good, but `isinstance` is preferred to `type()` these days (I believe).

[Jay Pipes (jaypipes)]

I could go either way, really. Not a big deal at all. Justin, what do you think?

[Devin Carlen (devcamcar)]

Tend to agree - let's just standardize around true/false, case insensitive and keep it simple.

[Jay Pipes (jaypipes)]

okey dokey, I'll fix up and re-propose.

[justinsb (justin-fathomdb)]

I don't really mind, as long as the docs match :-) I think most of the issues I filed with glance could be doc issues. It's OK to have a strict API as long as the rules are clearly documented.

Case insensitive 'true' is an improvement though, as is abstracting this logic into a function, and so LGTM, whatever values we converge on.

review: Approve

[Jay Pipes (jaypipes)]

OK, sounds good. I'm changing the docs and modifying the code to just do case-insensitive checks on 'true' and 'false'. Cheers!

[Jay Pipes (jaypipes)]

OK, guys, please re-review. Thanks!

[justinsb (justin-fathomdb)]

LGTM. Thanks for fixing the docs as well!

review: Approve

[Rick Harris (rconradharris)]

Awesome, thanks Jay.

review: Approve

[Thierry Carrez (ttx)]

lgtm

review: Approve

=== modified file 'doc/source/glanceapi.rst'
--- doc/source/glanceapi.rst	2011-03-08 16:40:35 +0000
+++ doc/source/glanceapi.rst	2011-04-12 21:48:27 +0000
@@ -308,8 +308,7 @@
 
   This header is optional.
 
-  When present, Glance converts the value of the header to a boolean value,
-  so "on, 1, true" are all true values. When true, the image is marked as
+  When Glance finds the string "true" (case-insensitive), the image is marked as
   a public image, meaning that any user may view its metadata and may read
   the disk image from Glance.
 

=== modified file 'glance/utils.py'
--- glance/utils.py	2011-03-09 13:38:25 +0000
+++ glance/utils.py	2011-04-12 21:48:27 +0000
@@ -91,12 +91,25 @@
     if 'size' in result:
         result['size'] = int(result['size'])
     if 'is_public' in result:
-        result['is_public'] = (result['is_public'] == 'True')
+        result['is_public'] = bool_from_header_value(result['is_public'])
     if 'deleted' in result:
-        result['deleted'] = (result['deleted'] == 'True')
+        result['deleted'] = bool_from_header_value(result['deleted'])
     return result
 
 
+def bool_from_header_value(value):
+    """
+    Returns True if value is a boolean True or the
+    string 'true', case-insensitive, False otherwise
+    """
+    if isinstance(value, bool):
+        return value
+    elif isinstance(value, (basestring, unicode)):
+        if str(value).lower() == 'true':
+            return True
+    return False
+
+
 def has_body(req):
     """
     Returns whether a Webob.Request object will possess an entity body.

=== modified file 'tests/unit/test_utils.py'
--- tests/unit/test_utils.py	2011-03-17 21:43:26 +0000
+++ tests/unit/test_utils.py	2011-04-12 21:48:27 +0000
@@ -65,3 +65,44 @@
         result = utils.get_image_meta_from_headers(response)
         for k, v in fixture.iteritems():
             self.assertEqual(v, result[k])
+
+    def test_boolean_header_values(self):
+        """
+        Tests that boolean headers like is_public can be set
+        to True if any of ('True', 'On', 1) is provided, case-insensitive
+        """
+        fixtures = [{'is_public': True},
+                    {'is_public': 'True'},
+                    {'is_public': 'true'}]
+
+        expected = {'is_public': True}
+
+        class FakeResponse():
+            pass
+
+        for fixture in fixtures:
+            headers = utils.image_meta_to_http_headers(fixture)
+
+            response = FakeResponse()
+            response.headers = headers
+            result = utils.get_image_meta_from_headers(response)
+            for k, v in expected.items():
+                self.assertEqual(v, result[k])
+
+        # Ensure False for other values...
+        fixtures = [{'is_public': False},
+                    {'is_public': 'Off'},
+                    {'is_public': 'on'},
+                    {'is_public': '1'},
+                    {'is_public': 'False'}]
+
+        expected = {'is_public': False}
+
+        for fixture in fixtures:
+            headers = utils.image_meta_to_http_headers(fixture)
+
+            response = FakeResponse()
+            response.headers = headers
+            result = utils.get_image_meta_from_headers(response)
+            for k, v in expected.items():
+                self.assertEqual(v, result[k])