Merge lp:~niemeyer/pyjuju/no-regex-option into lp:pyjuju

[Gustavo Niemeyer]

This handles the two changes suggested in the mailing list:

- Drop regex type
- Rename 'str' to 'string'

For the second change, it also introduces backwards compatibility logic
so that we can continue to work with 'str' for the moment while we warn
authors to move out of it.

[lp:~niemeyer/pyjuju/no-regex-option]

337. By Gustavo Niemeyer on 2011-08-30

Fixed examples.

[William Reade (fwereade)]

[0]

+ _warned_str_is_obsolete = False

Unused?

Otherwise +1.

review: Approve

[Kapil Thangavelu (hazmat)]

This is modifying a global variable when a deprecated usage is found, but multiple formulas may be utilized in a single command (deploy with auto-resolve against the ppa, or --with syntax for example), and in that case deprecated usage by additional formulas would go un-logged. perhaps keying to config path in a class dict, or just yagni for now

review: Needs Information

[Gustavo Niemeyer (niemeyer)]

Note that the message doesn't really specify which formula is using the obsoleted format. My idea with that is that people would review all of their formulas once they get the warning, while at the same time we can avoid pestering too much poor unrelated users that can't do anything about it.

[Kapil Thangavelu (hazmat)]

hmmm.. i almost think its worth changing the constructor to include the formula name for this case. Giving warnings on potentially third-party formulas isn't particularly useful without identifying the formula in question imo. The primary role i'm concerned about isn't the formula-author but the ensemble user consuming formulas. All that said, at this point those roles are a perfect intersection.

review: Approve

[Gustavo Niemeyer (niemeyer)]

I agree with all of your points, and also that it feels a bit early in the game to be worrying too much about this. I hope all formulas get fixed ASAP so that we can obsolete the option completely.

=== modified file 'ensemble/formula/config.py'
--- ensemble/formula/config.py	2011-08-24 00:18:27 +0000
+++ ensemble/formula/config.py	2011-08-30 21:40:24 +0000
@@ -1,5 +1,5 @@
+import sys
 import os
-import re
 
 import yaml
 
@@ -7,29 +7,24 @@
                                  Constant, OneOf, Int, Float)
 from ensemble.formula.errors import ServiceConfigError
 
-# regex type requires validator
-REGEX_TYPE_SCHEMA = KeyDict({"type": Constant("regex"),
-                        "validator": String(),
-                        "default": OneOf(String(), Int(), Float()),
-                        "description": String(),
-                        },
-                       optional=["default", "description"])
-
-SIMPLE_TYPE_SCHEMA = KeyDict({"type": OneOf(Constant("str"),
-                                            Constant("int"),
-                                            Constant("float")),
-                              "default": OneOf(String(), Int(), Float()),
-                              "description": String(),
-                              },
-                             optional=["default", "description"])
+OPTION_SCHEMA = KeyDict({
+        "type": OneOf(Constant("string"),
+                      Constant("str"), # Obsolete
+                      Constant("int"),
+                      Constant("float")),
+        "default": OneOf(String(), Int(), Float()),
+        "description": String(),
+    },
+    optional=["default", "description"],
+    )
 
 # Schema used to validate ConfigOptions specifications
 CONFIG_SCHEMA = KeyDict({
-    "options": Dict(
-                     String(),
-                     OneOf(REGEX_TYPE_SCHEMA, SIMPLE_TYPE_SCHEMA))
+    "options": Dict(String(), OPTION_SCHEMA),
     })
 
+WARNED_STR_IS_OBSOLETE = False
+
 
 class ConfigOptions(object):
     """Represents the configuration options exposed by a formula.
@@ -90,6 +85,8 @@
         data = self.parse_serialization_data(raw_data, pathname)
         self._data = data
 
+    _warned_str_is_obsolete = False
+
     def parse_serialization_data(self, data, pathname=None):
         """Verify we have sensible option metadata.
 
@@ -108,11 +105,23 @@
             raise ServiceConfigError("Invalid options specification: %s%s" % (
                 error, pathname))
 
+        # XXX Drop this after everyone has migrated their config to 'string'.
+        global WARNED_STR_IS_OBSOLETE
+        if not WARNED_STR_IS_OBSOLETE:
+            for name, info in data["options"].iteritems():
+                for field, value in info.iteritems():
+                    if field == "type" and value == "str":
+                        sys.stderr.write(
+                            "WARNING: Formula is using obsolete 'str' type "
+                            "in config.yaml. Rename it to 'string'.\n")
+                        WARNED_STR_IS_OBSOLETE = True
+                        break
+
         return data["options"]
 
     def _validate_one(self, name, value):
         # see if there is a type associated with the option
-        kind = self._data[name].get("type", "str")
+        kind = self._data[name].get("type", "string")
         if kind not in validation_kinds:
             raise ServiceConfigError(
                 "Unknown service option type: %s" % kind)
@@ -179,15 +188,10 @@
     except ValueError:
         return value, False
 
-
-def validate_regex(value, options):
-    pattern = options["validator"]
-    return value, re.match(pattern, value)
-
 # maps service option types to callables
 validation_kinds = {
-    "str": validate_str,
+    "string": validate_str,
+    "str": validate_str, # Obsolete
     "int": validate_int,
     "float": validate_float,
-    "regex": validate_regex
     }
 
=== modified file 'ensemble/formula/tests/repository/dummy/config.yaml'
--- ensemble/formula/tests/repository/dummy/config.yaml	2011-08-24 00:18:27 +0000
+++ ensemble/formula/tests/repository/dummy/config.yaml	2011-08-30 21:40:24 +0000
@@ -1,5 +1,5 @@
 options:
-  title: {default: My Title, description: A descriptive title used for the service., type: str}
-  outlook: {description: No default outlook., type: str}
-  username: {default: admin001, description: The name of the initial account (given admin permissions)., type: regex, validator: '\w{8}'}
+  title: {default: My Title, description: A descriptive title used for the service., type: string}
+  outlook: {description: No default outlook., type: string}
+  username: {default: admin001, description: The name of the initial account (given admin permissions)., type: string}
   skill-level: {description: A number indicating skill., type: int}
 
=== modified file 'ensemble/formula/tests/repository/wordpress/config.yaml'
--- ensemble/formula/tests/repository/wordpress/config.yaml	2011-05-31 23:07:35 +0000
+++ ensemble/formula/tests/repository/wordpress/config.yaml	2011-08-30 21:40:24 +0000
@@ -1,3 +1,3 @@
 options:
-  blog-title: {default: My Title, description: A descriptive title used for the blog., type: str}
+  blog-title: {default: My Title, description: A descriptive title used for the blog., type: string}
 
 
=== modified file 'ensemble/formula/tests/test_config.py'
--- ensemble/formula/tests/test_config.py	2011-08-26 12:24:00 +0000
+++ ensemble/formula/tests/test_config.py	2011-08-30 21:40:24 +0000
@@ -1,16 +1,30 @@
 # -*- encoding: utf-8 -*-
+from StringIO import StringIO
+import sys
+
 import yaml
 
 from ensemble.lib.testing import TestCase
 from ensemble.formula.config import ConfigOptions
 from ensemble.formula.errors import ServiceConfigError
+from ensemble.formula import config
 
 sample_configuration = """
 options:
-  title: {default: My Title, description: A descriptive title used for the service., type: str}
-  outlook: {description: No default outlook., type: str}
-  username: {default: admin001, description: The name of the initial account (given admin permissions)., type: regex, validator: '\w{8}'}
-  skill-level: {description: A number indicating skill., type: int}
+  title:
+    default: My Title
+    description: A descriptive title used for the service.
+    type: string
+  outlook:
+    description: No default outlook.
+    type: string
+  username:
+    default: admin001
+    description: The name of the initial account (given admin permissions).
+    type: string
+  skill-level:
+    description: A number indicating skill.
+    type: int
 """
 
 sample_yaml_data = yaml.load(sample_configuration)
@@ -79,8 +93,7 @@
         self.config.parse(sample_configuration)
         data = self.config.validate(sample_input)
 
-        # This should include an overridden value, a default and a new
-        # value.
+        # This should include an overridden value, a default and a new value.
         self.assertEqual(data,
                          {"username": "admin001",
                           "outlook": "Peachy",
@@ -88,7 +101,8 @@
 
         # now try to set a value outside the expected
         sample_input["bad"] = "value"
-        error = self.assertRaises(ServiceConfigError, self.config.validate, sample_input)
+        error = self.assertRaises(ServiceConfigError,
+                                  self.config.validate, sample_input)
         self.assertEqual(error.message,
                          "bad is not a valid configuration option.")
 
@@ -97,28 +111,6 @@
         config = ConfigOptions()
         self.assertRaises(ServiceConfigError, config.validate, sample_input)
 
-    def test_validate_regex(self):
-        self.config.parse(sample_configuration)
-
-        error = self.assertRaises(ServiceConfigError,
-                                  self.config.validate, dict(username="1234"))
-        self.assertEquals(str(error), "Invalid value for username: 1234")
-
-        data = self.config.validate(dict(username="12345678"))
-        self.assertEqual(data, {"username": "12345678", "title": "My Title"})
-
-    def test_validate_regex_wo_validator(self):
-        """Assure that validator is required for type=regex."""
-        local_configuration = sample_configuration + \
-          """  fails: {description: missing required., type:  regex}"""
-        error = self.assertRaises(ServiceConfigError,
-                          self.config.parse,
-                          local_configuration)
-        self.assertEquals(
-            str(error),
-            "Invalid options specification: options.fails.validator: "
-            "required value not found")
-
     def test_validate_types(self):
         self.config.parse(sample_configuration)
 
@@ -131,3 +123,30 @@
         self.assertEqual(data, {"skill-level": 9001,
                                 "title": "My Title",
                                 "username": "admin001"})
+
+    def test_validate_with_obsolete_str(self):
+        """
+        Test the handling for the obsolete 'str' option type (it's
+        'string' now). Remove support for it after a while, and take
+        this test with it.
+        """
+        config = yaml.load(sample_configuration)
+        config["options"]["title"]["type"] = "str"
+        obsolete_config = yaml.dump(config)
+
+        sio = StringIO()
+        self.patch(sys, "stderr", sio)
+
+        self.config.parse(obsolete_config)
+        data = self.config.validate({"title": "Helpful Title"})
+        self.assertEqual(data["title"], "Helpful Title")
+        self.assertIn("obsolete 'str'", sio.getvalue())
+
+        # Trying it again, it should not warn since we don't want
+        # to pester the formula author.
+        sio.truncate(0)
+        self.config.parse(obsolete_config)
+        data = self.config.validate({"title": "Helpful Title"})
+        self.assertEqual(data["title"], "Helpful Title")
+        self.assertEqual(sio.getvalue(), "")
+
 
=== modified file 'examples/php/config.yaml'
--- examples/php/config.yaml	2011-07-01 11:40:20 +0000
+++ examples/php/config.yaml	2011-08-30 21:40:24 +0000
@@ -1,5 +1,5 @@
 options:
   application_file:
     description: An application file to push.
-    type: str
+    type: string
 
 
=== modified file 'examples/wordpress/config.yaml'
--- examples/wordpress/config.yaml	2011-06-13 20:33:35 +0000
+++ examples/wordpress/config.yaml	2011-08-30 21:40:24 +0000
@@ -3,5 +3,5 @@
     description: |
       The URL of a wordpress plugin which will be installed and made available in the Admin UI.
       http://downloads.wordpress.org/plugin/buddypress.1.2.8.zip is a valid example.
-    type: str
+    type: string