Merge lp:~clint-fewbar/pyjuju/namespace-from-env into lp:pyjuju

[Clint Byrum]

Default Charm namespace should be overridable by environment variables.

If JUJU_DEFAULT_NS is specified, juju will infer that namespace.
Otherwise, 'cs:' will still be the default.

https://codereview.appspot.com/5695056/

[Benjamin Saller (bcsaller)]

LGTM, it wasn't clear from the mailing list if you agreed about gustavo's suggesstion of calling the env var JUJU_DEFAULT_NS or not, also (and I'm not 100% clear how to reference this now that we've split the docs off) you should add a note to the existing docs about this option.

review: Approve

[Kapil Thangavelu (hazmat)]

From the list discussion this should set namespace and series.

[lp:~clint-fewbar/pyjuju/namespace-from-env]

469. By Clint Byrum on 2012-03-07

working with new proposed ability to override series and user as well

470. By Clint Byrum on 2012-03-07

handle only user being inferred from environment

[Benjamin Saller (bcsaller)]

Thanks for sticking with this. I've included some feedback.

https://codereview.appspot.com/5695056/diff/3001/juju/charm/tests/test_url.py
File juju/charm/tests/test_url.py (left):

https://codereview.appspot.com/5695056/diff/3001/juju/charm/tests/test_url.py#oldcode126
juju/charm/tests/test_url.py:126:
All the tests assume the env is set to a proper value, test the case
where it is not.

https://codereview.appspot.com/5695056/diff/3001/juju/charm/url.py
File juju/charm/url.py (right):

https://codereview.appspot.com/5695056/diff/3001/juju/charm/url.py#newcode131
juju/charm/url.py:131: default_schema, rest =
os.environ.get("JUJU_DEFAULT_NS","cs:").split(":",1)
I think this module is too low level to be interacting with the
environment like this. The juju/control layer should consume environment
information and can supply information. Those (deploy and upgrade-charm)
use the repository.resolve function which can take additional arguments
for this (and handle errors in a place where we can inform the user they
set the variable incorrectly. Resolve can take the parsed output of the
environment and pass it to infer. juju/control can even use a util
method from here to help parse the argument but that parsing has to be
more solid that what is here now.

Currently a bad value, missing a ":" for example, will result in a
ValueError when trying to unpack on the left-hand-side. Need to be more
defensive than that.

https://codereview.appspot.com/5695056/

[lp:~clint-fewbar/pyjuju/namespace-from-env]

471. By Clint Byrum on 2012-03-09

move JUJU_DEFAULT_NS to deploy command

472. By Clint Byrum on 2012-03-09

merging with trunk

473. By Clint Byrum on 2012-03-09

documenting usage of JUJU_DEFAULT_NS in deploy.

474. By Clint Byrum on 2012-03-09

cleanup, remove improper env setting leftover rom refactoring

475. By Clint Byrum on 2012-03-09

get more defensive about bad namespaces

[Kapil Thangavelu (hazmat)]

This code would be much simpler, and easier to reason about its
correctness, if it did all its parsing up front and just used the
variables instead of conditionally redefining them as it went with
incremental parsing of 'rest'.

https://codereview.appspot.com/5695056/diff/8002/juju/charm/url.py
File juju/charm/url.py (right):

https://codereview.appspot.com/5695056/diff/8002/juju/charm/url.py#newcode155
juju/charm/url.py:155: default_infer_series, rest = rest.split('/',1)
untested

https://codereview.appspot.com/5695056/diff/8002/juju/charm/url.py#newcode182
juju/charm/url.py:182: rest = "%s/%s" % (default_infer_series, rest)
untested

https://codereview.appspot.com/5695056/

[Kapil Thangavelu (hazmat)]

Excerpts from Kapil Thangavelu's message of 2012-03-15 18:07:25 UTC:
> This code would be much simpler, and easier to reason about its
> correctness, if it did all its parsing up front and just used the
> variables instead of conditionally redefining them as it went with
> incremental parsing of 'rest'.
>
>
> https://codereview.appspot.com/5695056/diff/8002/juju/charm/url.py
> File juju/charm/url.py (right):
>
> https://codereview.appspot.com/5695056/diff/8002/juju/charm/url.py#newcode155
> juju/charm/url.py:155: default_infer_series, rest = rest.split('/',1)
> untested
>
> https://codereview.appspot.com/5695056/diff/8002/juju/charm/url.py#newcode182
> juju/charm/url.py:182: rest = "%s/%s" % (default_infer_series, rest)
> untested
>
> https://codereview.appspot.com/5695056/
>

diff to pep8 fixes
http://paste.ubuntu.com/885281/

[Kapil Thangavelu (hazmat)]

i've agreed with clint to take this over the completion and landing of this branch.

Unmerged revisions

475. By Clint Byrum on 2012-03-09

get more defensive about bad namespaces

474. By Clint Byrum on 2012-03-09

cleanup, remove improper env setting leftover rom refactoring

473. By Clint Byrum on 2012-03-09

documenting usage of JUJU_DEFAULT_NS in deploy.

472. By Clint Byrum on 2012-03-09

merging with trunk

471. By Clint Byrum on 2012-03-09

move JUJU_DEFAULT_NS to deploy command

470. By Clint Byrum on 2012-03-07

handle only user being inferred from environment

469. By Clint Byrum on 2012-03-07

working with new proposed ability to override series and user as well

468. By Clint Byrum on 2012-02-24

Pull default namespace from environment.

=== modified file 'juju/charm/repository.py'
--- juju/charm/repository.py	2012-02-16 19:19:42 +0000
+++ juju/charm/repository.py	2012-03-09 22:39:18 +0000
@@ -167,7 +167,7 @@
         returnValue(info["revision"])
 
 
-def resolve(vague_name, repository_path, default_series):
+def resolve(vague_name, repository_path, default_series, default_namespace=None):
     """Get a Charm and associated identifying information
 
     :param str vague_name: a lazily specified charm name, suitable for use with
@@ -181,12 +181,15 @@
     :param str default_series: the Ubuntu series to insert when `charm_name` is
         inadequately specified.
 
+    :param str default_namespace: partial charm url that is passed to
+        :meth:`CharmURL.infer`
+
     :return: a tuple of a :class:`juju.charm.url.CharmURL` and a
         :class:`juju.charm.base.CharmBase` subclass, which together contain
         both the charm's data and all information necessary to specify its
         source.
     """
-    url = CharmURL.infer(vague_name, default_series)
+    url = CharmURL.infer(vague_name, default_series, default_namespace)
     if url.collection.schema == "local":
         repo = LocalCharmRepository(repository_path)
     elif url.collection.schema == "cs":

=== modified file 'juju/charm/tests/test_url.py'
--- juju/charm/tests/test_url.py	2011-10-06 21:04:13 +0000
+++ juju/charm/tests/test_url.py	2012-03-09 22:39:18 +0000
@@ -98,8 +98,8 @@
         url3.assert_revision()
         self.assertEquals(str(url3), "cs:foo/bar-999")
 
-    def assert_infer(self, string, schema, user, series, name, rev):
-        url = CharmURL.infer(string, "default")
+    def assert_infer(self, string, schema, user, series, name, rev, ns=None):
+        url = CharmURL.infer(string, "default", ns)
         self.assert_url(url, schema, user, series, name, rev)
 
     def test_infer(self):
@@ -124,6 +124,77 @@
         self.assert_infer(
             "local:name-0", "local", None, "default", "name", 0)
 
+    def test_infer_from_default_namespace(self):
+        self.assert_infer(
+            "name", "local", None, "default", "name", None, 'local')
+        self.assert_infer(
+            "name-0", "local", None, "default", "name", 0, 'local')
+        self.assert_infer(
+            "series/name", "local", None, "series", "name", None, 'local')
+        self.assert_infer(
+            "series/name-0", "local", None, "series", "name", 0, 'local')
+        self.assert_infer(
+            "name", "local", None, "default", "name", None, 'local:')
+        self.assert_infer(
+            "name-0", "local", None, "default", "name", 0, 'local:')
+        self.assert_infer(
+            "series/name", "local", None, "series", "name", None, 'local:')
+        self.assert_infer(
+            "series/name-0", "local", None, "series", "name", 0, 'local:')
+
+    def test_infer_series_from_default_namespace(self):
+        self.assert_infer(
+            "name", "cs", None, "envseries", "name", None, 'cs:envseries')
+        self.assert_infer(
+            "name-0", "cs", None, "envseries", "name", 0, 'cs:envseries')
+        self.assert_infer(
+            "series/name", "cs", None, "series", "name", None, 'cs:envseries')
+        self.assert_infer(
+            "series/name-0", "cs", None, "series", "name", 0, 'cs:envseries')
+
+    def test_infer_user_from_default_namespace(self):
+        self.assert_infer(
+            "name", "cs", "foouser", "default", "name", None, 'cs:~foouser')
+        self.assert_infer(
+            "name-0", "cs", "foouser", "default", "name", 0, 'cs:~foouser')
+        self.assert_infer(
+            "series/name", "cs", "foouser", "series", "name", None, 'cs:~foouser')
+        self.assert_infer(
+            "series/name-0", "cs", "foouser", "series", "name", 0, 'cs:~foouser')
+
+    def test_infer_user_and_series_from_environ(self):
+        self.assert_infer(
+            "name", "cs", "foouser", "fooseries", "name", None, 'cs:~foouser/fooseries')
+        self.assert_infer(
+            "name-0", "cs", "foouser", "fooseries", "name", 0, 'cs:~foouser/fooseries')
+        self.assert_infer(
+            "series/name", "cs", "foouser", "series", "name", None, 'cs:~foouser/fooseries')
+        self.assert_infer(
+            "series/name-0", "cs", "foouser", "series", "name", 0, 'cs:~foouser/fooseries')
+
+    def test_infer_bad_namespace(self):
+        # Invalid schema "zyzzyx/"
+        self.assertRaises(
+            CharmURLError, CharmURL.infer, "name","fooseries", 'zyzzyx/:/~foo///')
+        self.assertRaises(
+            CharmURLError, CharmURL.infer, "series/name","fooseries", 'zyzzyx/:/~foo///')
+        self.assertRaises(
+            CharmURLError, CharmURL.infer, "~user/series/name","fooseries", 'zyzzyx/:/~foo///')
+        """ cs:name, does not actually need to be inferred from the 
+            default namespace, but the namespace will still trigger problems. """
+        self.assertRaises(
+            CharmURLError, CharmURL.infer, "cs:name","fooseries", 'zyzzyx/:/~foo///')
+        self.assertRaises(
+            CharmURLError, CharmURL.infer, "local:name","fooseries", 'zyzzyx/:/~foo///')
+
+        # Just a user is too ambiguous
+        self.assertRaises(
+            CharmURLError, CharmURL.infer, "name","fooseries", '~someuser')
+        self.assertRaises(
+            CharmURLError, CharmURL.infer, "series/name","fooseries", '~someuser')
+        self.assertRaises(
+            CharmURLError, CharmURL.infer, "~user/series/name","fooseries", '~someuser')
+
     def test_cannot_infer(self):
         err = self.assertRaises(
             CharmURLError, CharmURL.infer, "name", "invalid!series")

=== modified file 'juju/charm/url.py'
--- juju/charm/url.py	2011-10-06 21:04:13 +0000
+++ juju/charm/url.py	2012-03-09 22:39:18 +0000
@@ -1,5 +1,6 @@
 import copy
 import re
+import os
 
 from juju.charm.errors import CharmURLError
 
@@ -7,6 +8,7 @@
 _USER_RE = re.compile("^[a-z0-9][a-zA-Z0-9+.-]+$")
 _SERIES_RE = re.compile("^[a-z]+([a-z-]+[a-z])?$")
 _NAME_RE = re.compile("^[a-z][a-z0-9]*(-[a-z0-9]*[a-z][a-z0-9]*)*$")
+_NAMESPACE_RE = re.compile("^[a-zA-Z]+:\~?[a-zA-Z\-]*/?")
 
 
 class CharmCollection(object):
@@ -114,7 +116,7 @@
         return cls(CharmCollection(schema, user, series), name, revision)
 
     @classmethod
-    def infer(cls, vague_name, default_series):
+    def infer(cls, vague_name, default_series, default_namespace=None):
         """Turn a potentially fuzzy alias into a CharmURL."""
         try:
             # it might already be a valid URL string
@@ -127,19 +129,59 @@
             raise CharmURLError(
                 vague_name, "a URL with a user must specify a schema")
 
+        if default_namespace is None:
+            default_namespace = 'cs:'
+        else:
+            # schema is the most basic unit, so assume that if no :
+            if ':' not in default_namespace:
+                default_namespace = "%s:" % (default_namespace)
+
+        if not _NAMESPACE_RE .match(default_namespace):
+            raise CharmURLError(default_namespace, "invalid default namespace")
+
+        default_schema, rest = default_namespace.split(":",1)
+
+        # determine what has been defaulted in JUJU_DEFAULT_NS
+        if len(rest) and rest[0] == '~':
+            if '/' in rest:
+                default_infer_user, rest = rest[1:].split('/',1)
+            else:
+                default_infer_user = rest[1:]
+                rest=''
+        else:
+            default_infer_user = None
+
+        if '/' in rest:
+            default_infer_series, rest = rest.split('/',1)
+        elif len(rest):
+            default_infer_series = rest
+        else:
+            default_infer_series = default_series
+
+        # determine what is and is not set in vague_name
         if ":" in vague_name:
             schema, rest = vague_name.split(":", 1)
         else:
-            schema, rest = "cs", vague_name
+            schema = default_schema
+            rest = vague_name
+
+        if rest[0] != '~':
+            if default_infer_user is None:
+                if '/' not in rest:
+                    rest = "%s/%s" % (default_infer_series, rest)
+            else:
+                if '/' in rest:
+                    rest = "~%s/%s" % (default_infer_user, rest)
+                else:
+                    rest = "~%s/%s/%s" % (default_infer_user, default_infer_series, rest)
+        else:
+            if '/' in rest:
+                infer_user, rest = rest.split('~')[1].split('/',1)
+                rest = "~%s/%s/%s" % (infer_user, default_infer_series, rest)
+            else:
+                rest = "%s/%s" % (default_infer_series, rest)
 
         url_string = "%s:%s" % (schema, rest)
-        parts = rest.split("/")
-        if len(parts) == 1:
-            url_string = "%s:%s/%s" % (schema, default_series, rest)
-        elif len(parts) == 2:
-            if parts[0].startswith("~"):
-                url_string = "%s:%s/%s/%s" % (
-                    schema, parts[0], default_series, parts[1])
 
         try:
             return cls.parse(url_string)

=== modified file 'juju/control/deploy.py'
--- juju/control/deploy.py	2012-03-09 20:20:20 +0000
+++ juju/control/deploy.py	2012-03-09 22:39:18 +0000
@@ -112,9 +112,14 @@
     a service from the charm, and get it set to be launched
     on a new machine. If --repository is not specified, it
     will be taken from the environment variable JUJU_REPOSITORY.
+    If `charm` has a non-specific URL it will be resolved
+    using the default namespace, which is "cs:". That default can
+    be overridden with the environment variable JUJU_DEFAULT_NS.
     """
+    default_namespace = os.environ.get('JUJU_DEFAULT_NS')
     repo, charm_url = resolve(
-        charm_name, repository_path, environment.default_series)
+        charm_name, repository_path, environment.default_series,
+        default_namespace)
 
     log.info("Searching for charm")
     charm = yield repo.find(charm_url)

=== modified file 'juju/control/tests/test_deploy.py'
--- juju/control/tests/test_deploy.py	2012-03-09 20:13:36 +0000
+++ juju/control/tests/test_deploy.py	2012-03-09 22:39:18 +0000
@@ -210,10 +210,19 @@
 
     @inlineCallbacks
     def test_deploy(self):
+        yield self._test_deploy('local:sample')
+
+    @inlineCallbacks
+    def test_deploy_namespace_from_environ(self):
+        self.change_environment(JUJU_DEFAULT_NS='local')
+        yield self._test_deploy('sample')
+
+    @inlineCallbacks
+    def _test_deploy(self, charmname):
         """Create service, and service unit on machine from charm"""
         environment = self.config.get("firstenv")
         yield deploy.deploy(
-            self.config, environment, self.unbundled_repo_path, "local:sample",
+            self.config, environment, self.unbundled_repo_path, charmname,
             "myblog", logging.getLogger("deploy"), ["cpu=123"])
         topology = yield self.get_topology()
 
@@ -299,7 +308,7 @@
         repo.type
         self.mocker.result("store")
         resolve = self.mocker.replace("juju.control.deploy.resolve")
-        resolve("cs:sample", None, "series")
+        resolve("cs:sample", None, "series", None)
         self.mocker.result((repo, CharmURL.infer("cs:sample", "series")))
         repo.find(MATCH(lambda x: isinstance(x, CharmURL)))
         self.mocker.result(CharmDirectory(self.sample_dir1))