Merge lp:~fwereade/pyjuju/check-latest-formulas into lp:pyjuju

[William Reade]

Use the /latest?charms=blah,blah API to detect latest revisions in RemoteCharmRepository; added analogous .latest to LocalFormulaRepository as well.

[William Reade (fwereade)]

Based on request for a cache dir in https://code.launchpad.net/~fwereade/juju/use-remote-formulas/+merge/77323, this branch now actually uses the cache. It assumes that a charm store implementation will guarantee that a charm-url-with-revision uniquely identifies a given charm, but I think that's a reasonable thing to demand ;-).

[William Reade (fwereade)]

Technically, I suppose, I'm demanding that a charm-url-without-revision plus a revision will uniquely identify a given charm, but if the two things turn out to be different I think we have bigger problems.

[Gustavo Niemeyer (niemeyer)]

[1]

+ def latest(self, charm_url):
+ d = self.find(charm_url)
+ d.addCallback(attrgetter("metadata.revision"))
+ return d

Eventually the method signature should be closer to the API in
the spec, since we don't want to do a query for every single
charm we have to find out the revision for.

I'm fine to move this forward as an initial implementation, though.

[2]

+ d.addCallback(attrgetter("metadata.revision"))

This seems clearer, avoids the import, and is shorter:

    d.addCallback(lambda c: c.metadata.revision)

[3]

+ url = "%s/%s?charms=%s" % (self.url_base, query, charm_url)

charm_url has to be url-escaped here.

[4]

+ def find(self, charm_url):
+ revision = yield self.latest(charm_url)
+ charm_url = charm_url.with_revision(revision)
+ charm = yield self._get_charm(charm_url)

Why is this hardcoding getting the latest formula at all times?

If charm_url has a revision, there's no reason not to respect it
and proceed exactly the same way, I think.

Also, the original version of this function worked with a single
roundtrip to the server. This one does three of them (one for
the revision, one for the charm, and another one for the sha256).
I think we should tweak the server API so we can get both the
revision and the sha256 at the same time. Will think a bit about
this and post back later today.

review: Needs Fixing

[William Reade (fwereade)]

Thanks :).

> [1]
>
> + def latest(self, charm_url):
> + d = self.find(charm_url)
> + d.addCallback(attrgetter("metadata.revision"))
> + return d
>
> Eventually the method signature should be closer to the API in
> the spec, since we don't want to do a query for every single
> charm we have to find out the revision for.
>
> I'm fine to move this forward as an initial implementation, though.

Agree, but am trying to avoid speculative generality in the plastic bits.

> [2]
>
> + d.addCallback(attrgetter("metadata.revision"))
>
> This seems clearer, avoids the import, and is shorter:
>
> d.addCallback(lambda c: c.metadata.revision)

Awww, ok.

> [3]
>
> + url = "%s/%s?charms=%s" % (self.url_base, query, charm_url)
>
> charm_url has to be url-escaped here.

* fwereade hangs head in shame

> [4]
>
> + def find(self, charm_url):
> + revision = yield self.latest(charm_url)
> + charm_url = charm_url.with_revision(revision)
> + charm = yield self._get_charm(charm_url)
>
> Why is this hardcoding getting the latest formula at all times?
>
> If charm_url has a revision, there's no reason not to respect it
> and proceed exactly the same way, I think.

Sounds good, I'll fix it.

> Also, the original version of this function worked with a single
> roundtrip to the server. This one does three of them (one for
> the revision, one for the charm, and another one for the sha256).
> I think we should tweak the server API so we can get both the
> revision and the sha256 at the same time. Will think a bit about
> this and post back later today.

A combined method in the server API would be great, we'd just need one roundtrip on cache hits and two on misses. I'll go ahead assuming one exists and make sure we agree what it should be called before next MP :).

[Gustavo Niemeyer (niemeyer)]

[4]

So, here is an idea to sort out the multiple-request problem: let's
replace the /latest entry by one called /charm-info that is called
the same way (/charm-info?charms=<urls>), but returns a JSON object
like this instead:

{charm_url: {"sha256": s, "revision": n}, ...}

How does that sound?

[William Reade (fwereade)]

> [4]
>
> So, here is an idea to sort out the multiple-request problem: let's
> replace the /latest entry by one called /charm-info that is called
> the same way (/charm-info?charms=<urls>), but returns a JSON object
> like this instead:
>
> {charm_url: {"sha256": s, "revision": n}, ...}
>
> How does that sound?

Perfect.

[lp:~fwereade/pyjuju/check-latest-formulas]

386. By William Reade on 2011-10-04

merge trunk

[Gustavo Niemeyer (niemeyer)]

This is looking great. LGTM, considering just a couple of minors:

[5]

+ try:
+ data = yield getPage(url)
+ returnValue(json.loads(data)[str(charm_url)])
+ except (Error, KeyError, ValueError, TypeError):
+ raise CharmNotFound(self.url_base, charm_url)

Hmmm.. do we really want TypeError and ValueError in this list?
My gut feeling when looking at this is that they would be bugs,
but maybe you have something else in mind.

[6]

+ assert charm.metadata.revision == revision, "bad charm revision"
+ assert charm.get_sha256() == info["sha256"], "bad bundle hash"

The first seems like a good fit for an assertion, but the bottom one
is a plausible runtime error that deserves a proper exception and
a nice error message to help the user.

review: Approve

[William Reade (fwereade)]

[5]

They're intended to guard against bugs in the charm store, really, but I take your point. Hmm. It would certainly be clearer if I assumed that CS will either give me what I asked for, or Error out; can't guard against everything, after all, and the above is indeed potentially masking local bugs. Yep, I'm convinced.

[6]

Sounds good. I'll make sure I delete the bad cache entry as well, so there's some chance of a retry actually working.

[lp:~fwereade/pyjuju/check-latest-formulas]

387. By William Reade on 2011-10-05

address review points

388. By William Reade on 2011-10-05

merge trunk

=== modified file 'juju/charm/repository.py'
--- juju/charm/repository.py	2011-10-03 20:04:45 +0000
+++ juju/charm/repository.py	2011-10-05 10:00:46 +0000
@@ -1,9 +1,11 @@
+import json
 import os
 import tempfile
+import urllib
 import yaml
 
 from twisted.internet.defer import fail, inlineCallbacks, returnValue, succeed
-from twisted.web.client import downloadPage
+from twisted.web.client import downloadPage, getPage
 from twisted.web.error import Error
 
 from juju.charm.provider import get_charm_from_path
@@ -21,6 +23,11 @@
         pass
 
 
+def _cache_key(charm_url):
+    charm_url.assert_revision()
+    return under.quote("%s.charm" % charm_url)
+
+
 class LocalCharmRepository(object):
     """Charm repository in a local directory."""
 
@@ -50,11 +57,12 @@
         recent one (greatest revision) will be returned.
         """
         assert charm_url.collection.schema == "local", "schema mismatch"
-        assert charm_url.revision is None, "find-by-revision not supported yet"
 
         latest = None
         for charm in self._collection(charm_url.collection):
             if charm.metadata.name == charm_url.name:
+                if charm.metadata.revision == charm_url.revision:
+                    return succeed(charm)
                 if (latest is None or
                     latest.metadata.revision < charm.metadata.revision):
                     latest = charm
@@ -64,6 +72,11 @@
 
         return succeed(latest)
 
+    def latest(self, charm_url):
+        d = self.find(charm_url.with_revision(None))
+        d.addCallback(lambda c: c.metadata.revision)
+        return d
+
 
 class RemoteCharmRepository(object):
 
@@ -75,42 +88,60 @@
             self.cache_path = cache_path
 
     @inlineCallbacks
-    def _download(self, url):
+    def _get_info(self, charm_url):
+        url = "%s/charm-info?charms=%s" % (
+            self.url_base, urllib.quote(str(charm_url)))
+        try:
+            data = yield getPage(url)
+            returnValue(json.loads(data)[str(charm_url)])
+        except Error:
+            raise CharmNotFound(self.url_base, charm_url)
+
+    @inlineCallbacks
+    def _download(self, charm_url, cache_path):
+        url = "%s/charm/%s" % (self.url_base, urllib.quote(charm_url.path))
         downloads = os.path.join(self.cache_path, "downloads")
         _makedirs(downloads)
         f = tempfile.NamedTemporaryFile(
-            prefix=under.quote(url), suffix=".charm", dir=downloads,
+            prefix=_cache_key(charm_url), suffix=".part", dir=downloads,
             delete=False)
         f.close()
-        completed_path = f.name
-        downloading_path = completed_path + ".part"
-        yield downloadPage(url, downloading_path)
-        os.rename(downloading_path, completed_path)
-        returnValue(completed_path)
-
-    def _cache(self, charm_url_base, temp_charm_path):
-        temp_charm = get_charm_from_path(temp_charm_path)
-        revision = temp_charm.metadata.revision
-        charm_key = under.quote(
-            "%s.charm" % (charm_url_base.with_revision(revision)))
-        charm_path = os.path.join(self.cache_path, charm_key)
-        _makedirs(self.cache_path)
-        os.rename(temp_charm_path, charm_path)
-        return get_charm_from_path(charm_path)
-
-    @inlineCallbacks
-    def find(self, charm_url):
-        assert charm_url.revision is None, "find-by-revision not supported yet"
-        _, path = str(charm_url).split(":", 1)
-        url = "%s/charm/%s" % (self.url_base, path)
+        downloading_path = f.name
         try:
-            temp_charm_path = yield self._download(url)
+            yield downloadPage(url, downloading_path)
         except Error:
             raise CharmNotFound(self.url_base, charm_url)
-        returnValue(self._cache(charm_url, temp_charm_path))
-
-
-@inlineCallbacks
+        os.rename(downloading_path, cache_path)
+
+    @inlineCallbacks
+    def find(self, charm_url):
+        info = yield self._get_info(charm_url)
+        revision = info["revision"]
+        if charm_url.revision is None:
+            charm_url = charm_url.with_revision(revision)
+        else:
+            assert revision == charm_url.revision, "bad url revision"
+
+        cache_path = os.path.join(self.cache_path, _cache_key(charm_url))
+        cached = os.path.exists(cache_path)
+        if not cached:
+            yield self._download(charm_url, cache_path)
+        charm = get_charm_from_path(cache_path)
+
+        assert charm.metadata.revision == revision, "bad charm revision"
+        if charm.get_sha256() != info["sha256"]:
+            os.remove(cache_path)
+            name = "%s (%s)" % (
+                charm_url, "cached" if cached else "downloaded")
+            raise CharmError(name, "SHA256 mismatch")
+        returnValue(charm)
+
+    @inlineCallbacks
+    def latest(self, charm_url):
+        info = yield self._get_info(charm_url.with_revision(None))
+        returnValue(info["revision"])
+
+
 def resolve(vague_name, repository_path, default_series):
     """Get a Charm and associated identifying information
 
@@ -125,7 +156,7 @@
     :param str default_series: the Ubuntu series to insert when `charm_name` is
         inadequately specified.
 
-    :return: a tuple of a :class:`CharmURL` and a
+    :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.
@@ -135,5 +166,4 @@
         repo = LocalCharmRepository(repository_path)
     elif url.collection.schema == "cs":
         repo = RemoteCharmRepository("https://store.juju.ubuntu.com")
-    charm = yield repo.find(url)
-    returnValue((url, charm))
+    return repo, url
 
=== modified file 'juju/charm/tests/test_repository.py'
--- juju/charm/tests/test_repository.py	2011-09-30 10:32:01 +0000
+++ juju/charm/tests/test_repository.py	2011-10-05 10:00:46 +0000
@@ -1,3 +1,4 @@
+import json
 import os
 import inspect
 import shutil
@@ -12,6 +13,7 @@
 from juju.charm.repository import (
     LocalCharmRepository, RemoteCharmRepository, resolve)
 from juju.charm.url import CharmURL
+from juju.errors import CharmError
 from juju.lib import under
 
 from juju.charm import tests
@@ -61,25 +63,37 @@
         return CharmURL.parse("local:series/" + name)
 
     @inlineCallbacks
-    def test_find_charm_by_name_which_is_unbundled(self):
-        charm = yield self.repository1.find(self.charm_url("sample"))
-        self.assertTrue(charm)
-        self.assertTrue(charm.metadata)
-
-    def test_find_charm_ignores_unknown_files(self):
+    def assert_not_there(self, name):
+        url = self.charm_url(name)
+        msg = "Charm 'local:series/%s' not found in repository %s" % (
+            name, self.unbundled_repo_path)
+        err = yield self.assertFailure(
+            self.repository1.find(url), CharmNotFound)
+        self.assertEquals(str(err), msg)
+        err = yield self.assertFailure(
+            self.repository1.latest(url), CharmNotFound)
+        self.assertEquals(str(err), msg)
+
+    def test_find_inappropriate_url(self):
+        url = CharmURL.parse("cs:foo/bar")
+        err = self.assertRaises(AssertionError, self.repository1.find, url)
+        self.assertEquals(str(err), "schema mismatch")
+
+    def test_completely_missing(self):
+        return self.assert_not_there("zebra")
+
+    def test_unkown_files_ignored(self):
         self.makeFile(
             "Foobar",
             path=os.path.join(self.repository1.path, "series", "zebra"))
-        return self.assertFailure(
-            self.repository1.find(self.charm_url("zebra")), CharmNotFound)
+        return self.assert_not_there("zebra")
 
-    def test_find_charm_ignores_unknown_directories(self):
+    def test_unknown_directories_ignored(self):
         self.makeDir(
             path=os.path.join(self.repository1.path, "series", "zebra"))
-        return self.assertFailure(
-            self.repository1.find(self.charm_url("zebra")), CharmNotFound)
+        return self.assert_not_there("zebra")
 
-    def test_find_charm_ignores_broken_charms(self):
+    def test_broken_charms_ignored(self):
         charm_path = self.makeDir(
             path=os.path.join(self.repository1.path, "series", "zebra"))
         fh = open(os.path.join(charm_path, "metadata.yaml"), "w+")
@@ -90,45 +104,27 @@
 revision: 0
 summary: hola""")
         fh.close()
-
-        return self.assertFailure(
-            self.repository1.find(self.charm_url("zebra")), CharmNotFound)
-
-    @inlineCallbacks
-    def test_find_charm_by_name_which_is_bundled(self):
-        charm = yield self.repository2.find(self.charm_url("sample"))
-        self.assertTrue(charm)
-        self.assertTrue(charm.metadata)
-
-    @inlineCallbacks
-    def test_find_charm_by_name_fails_with_bad_series(self):
-        error = yield self.assertFailure(
-            self.repository1.find(CharmURL.parse("local:missing/charm")),
-            CharmNotFound)
-        self.assertEquals(
-            str(error),
-            "Charm 'local:missing/charm' not found in repository %s"
-            % self.unbundled_repo_path)
-
-    def test_find_charm_by_name_fails(self):
-        d = self.assertFailure(
-            self.repository1.find(self.charm_url("missing")), CharmNotFound)
-
-        def verify(error):
-            self.assertEquals(
-                str(error),
-                "Charm 'local:series/missing' not found in repository %s"
-                % self.unbundled_repo_path)
-        d.addCallback(verify)
-        return d
-
-    @inlineCallbacks
-    def test_find_charm_with_multiple_versions_returns_latest(self):
-        # Copy the repository out of the codebase, so that we can hack it.
-        charm = yield self.repository1.find(self.charm_url("sample"))
-        self.assertEquals(charm.metadata.revision, 2)
-
-        # Invert the "latest" logic, to ensure it's not just a coincidence.
+        return self.assert_not_there("zebra")
+
+    def assert_there(self, name, repo, revision, latest_revision=None):
+        url = self.charm_url(name)
+        charm = yield repo.find(url)
+        self.assertEquals(charm.metadata.revision, revision)
+        latest = yield repo.latest(url)
+        self.assertEquals(latest, latest_revision or revision)
+
+    def test_success_unbundled(self):
+        return self.assert_there("sample", self.repository1, 2)
+        return self.assert_there("sample-2", self.repository1, 2)
+
+    def test_success_bundled(self):
+        return self.assert_there("sample", self.repository2, 2)
+        return self.assert_there("sample-2", self.repository2, 2)
+
+    @inlineCallbacks
+    def test_no_revision_gets_latest(self):
+        yield self.assert_there("sample", self.repository1, 2)
+
         file = open(os.path.join(
             self.repository1.path, "series/old/metadata.yaml"), "rw+")
         data = yaml.load(file.read())
@@ -137,18 +133,8 @@
         file.write(yaml.dump(data))
         file.close()
 
-        charm = yield self.repository1.find(self.charm_url("sample"))
-        self.assertEquals(charm.metadata.revision, 3)
-
-    def test_find_inappropriate_url(self):
-        url = CharmURL.parse("cs:foo/bar")
-        err = self.assertRaises(AssertionError, self.repository1.find, url)
-        self.assertEquals(str(err), "schema mismatch")
-
-    def test_find_with_revision(self):
-        url = CharmURL.parse("local:foo/bar-1")
-        err = self.assertRaises(AssertionError, self.repository1.find, url)
-        self.assertEquals(str(err), "find-by-revision not supported yet")
+        yield self.assert_there("sample", self.repository1, 3)
+        yield self.assert_there("sample-2", self.repository1, 2, 3)
 
 
 class RemoteRepositoryTest(RepositoryTestBase):
@@ -157,142 +143,270 @@
         super(RemoteRepositoryTest, self).setUp()
         self.cache_path = os.path.join(
             tempfile.mkdtemp(), "notexistyet")
+        self.download_path = os.path.join(self.cache_path, "downloads")
 
         def delete():
             if os.path.exists(self.cache_path):
                 shutil.rmtree(self.cache_path)
         self.addCleanup(delete)
 
+        self.charm = CharmDirectory(
+            os.path.join(self.unbundled_repo_path, "series", "dummy"))
+        with open(self.charm.as_bundle().path, "rb") as f:
+            self.bundle_data = f.read()
+        self.sha256 = self.charm.as_bundle().get_sha256()
+        self.getPage = self.mocker.replace("twisted.web.client.getPage")
+        self.downloadPage = self.mocker.replace(
+            "twisted.web.client.downloadPage")
+
     def repo(self, url_base):
         return RemoteCharmRepository(url_base, self.cache_path)
 
-    @inlineCallbacks
-    def assert_find(self, dns_name, url_str, expect_url):
-        src_charm = CharmDirectory(
-            os.path.join(self.unbundled_repo_path, "series", "dummy"))
-        with open(src_charm.as_bundle().path, "rb") as f:
-            bundle_data = f.read()
-
-        download_dir = os.path.join(self.cache_path, "downloads")
-
-        downloadPage = self.mocker.replace("twisted.web.client.downloadPage")
-        downloadPage(expect_url, ANY)
-
-        def download(_, temp_path):
-            self.assertTrue(temp_path.startswith(download_dir))
-            with open(temp_path, "wb") as f:
-                f.write(bundle_data)
+    def cache_location(self, url_str, revision):
+        charm_url = CharmURL.parse(url_str)
+        cache_key = under.quote(
+            "%s.charm" % (charm_url.with_revision(revision)))
+        return os.path.join(self.cache_path, cache_key)
+
+    def charm_info(self, url_str, revision):
+        return json.dumps({
+            url_str: {"revision": revision, "sha256": self.sha256}})
+
+    def mock_charm_info(self, url, result):
+        self.getPage(url)
+        self.mocker.result(result)
+
+    def mock_download(self, url, error=None):
+        self.downloadPage(url, ANY)
+        if error:
+            return self.mocker.result(fail(error))
+
+        def download(_, path):
+            self.assertTrue(path.startswith(self.download_path))
+            with open(path, "wb") as f:
+                f.write(self.bundle_data)
             return succeed(None)
         self.mocker.call(download)
-        self.mocker.replay()
-
-        repo = self.repo(dns_name)
-        charm = yield repo.find(CharmURL.parse(url_str))
-        self.assertEquals(charm.get_sha256(), src_charm.get_sha256())
-        self.assertEquals(os.listdir(download_dir), [])
-        expect_name = "%s-%s.charm" % (
-            under.quote(url_str), charm.metadata.revision)
-        self.assertEquals(
-            charm.path, os.path.join(self.cache_path, expect_name))
-
-    def test_find_plain(self):
-        return self.assert_find(
-            "https://somewhe.re", "cs:series/name",
-            "https://somewhe.re/charm/series/name")
-
-    def test_find_user(self):
-        return self.assert_find(
-            "https://somewhereel.se", "cs:~user/series/name",
-            "https://somewhereel.se/charm/~user/series/name")
-
-    def test_cant_find(self):
-        downloadPage = self.mocker.replace("twisted.web.client.downloadPage")
-        downloadPage("https://anoth.er/charm/series/name", ANY)
-        self.mocker.result(fail(Error("500")))
-        self.mocker.replay()
-
-        repo = RemoteCharmRepository("https://anoth.er")
-        d = self.assertFailure(
-            repo.find(CharmURL.parse("cs:series/name")),
-            CharmNotFound)
-
-        def verify(error):
-            self.assertEquals(
-                str(error),
-                "Charm 'cs:series/name' not found in repository "
-                "https://anoth.er")
-        d.addCallback(verify)
-        return d
-
-    def test_revision(self):
-        repo = RemoteCharmRepository("whatev.er")
-        d = self.assertFailure(
-            repo.find(CharmURL.parse("cs:series/name-1")),
-            AssertionError)
-
-        def verify(error):
-            self.assertEquals(str(error), "find-by-revision not supported yet")
-        d.addCallback(verify)
-        return d
+
+    @inlineCallbacks
+    def assert_find_uncached(self, dns_name, url_str, info_url, find_url):
+        self.mock_charm_info(info_url, succeed(self.charm_info(url_str, 1)))
+        self.mock_download(find_url)
+        self.mocker.replay()
+
+        repo = self.repo(dns_name)
+        charm = yield repo.find(CharmURL.parse(url_str))
+        self.assertEquals(charm.get_sha256(), self.sha256)
+        self.assertEquals(charm.path, self.cache_location(url_str, 1))
+        self.assertEquals(os.listdir(self.download_path), [])
+
+    @inlineCallbacks
+    def assert_find_cached(self, dns_name, url_str, info_url):
+        os.makedirs(self.cache_path)
+        cache_location = self.cache_location(url_str, 1)
+        shutil.copy(self.charm.as_bundle().path, cache_location)
+
+        self.mock_charm_info(info_url, succeed(self.charm_info(url_str, 1)))
+        self.mocker.replay()
+
+        repo = self.repo(dns_name)
+        charm = yield repo.find(CharmURL.parse(url_str))
+        self.assertEquals(charm.get_sha256(), self.sha256)
+        self.assertEquals(charm.path, cache_location)
+
+    def assert_find_error(self, dns_name, url_str, err_type, message):
+        self.mocker.replay()
+        repo = self.repo(dns_name)
+        d = self.assertFailure(repo.find(CharmURL.parse(url_str)), err_type)
+
+        def verify(error):
+            self.assertEquals(str(error), message)
+        d.addCallback(verify)
+        return d
+
+    @inlineCallbacks
+    def assert_latest(self, dns_name, url_str, revision):
+        self.mocker.replay()
+        repo = self.repo(dns_name)
+        result = yield repo.latest(CharmURL.parse(url_str))
+        self.assertEquals(result, revision)
+
+    def assert_latest_error(self, dns_name, url_str, err_type, message):
+        self.mocker.replay()
+        repo = self.repo(dns_name)
+        d = self.assertFailure(repo.latest(CharmURL.parse(url_str)), err_type)
+
+        def verify(error):
+            self.assertEquals(str(error), message)
+        d.addCallback(verify)
+        return d
+
+    def test_find_plain_uncached(self):
+        return self.assert_find_uncached(
+            "https://somewhe.re", "cs:series/name",
+            "https://somewhe.re/charm-info?charms=cs%3Aseries/name",
+            "https://somewhe.re/charm/series/name-1")
+
+    def test_find_revision_uncached(self):
+        return self.assert_find_uncached(
+            "https://somewhe.re", "cs:series/name-1",
+            "https://somewhe.re/charm-info?charms=cs%3Aseries/name-1",
+            "https://somewhe.re/charm/series/name-1")
+
+    def test_find_user_uncached(self):
+        return self.assert_find_uncached(
+            "https://somewhereel.se", "cs:~user/srs/name",
+            "https://somewhereel.se/charm-info?charms=cs%3A%7Euser/srs/name",
+            "https://somewhereel.se/charm/%7Euser/srs/name-1")
+
+    def test_find_plain_cached(self):
+        return self.assert_find_cached(
+            "https://somewhe.re", "cs:series/name",
+            "https://somewhe.re/charm-info?charms=cs%3Aseries/name")
+
+    def test_find_revision_cached(self):
+        return self.assert_find_cached(
+            "https://somewhe.re", "cs:series/name-1",
+            "https://somewhe.re/charm-info?charms=cs%3Aseries/name-1")
+
+    def test_find_user_cached(self):
+        return self.assert_find_cached(
+            "https://somewhereel.se", "cs:~user/srs/name",
+            "https://somewhereel.se/charm-info?charms=cs%3A%7Euser/srs/name")
+
+    def test_find_info_error(self):
+        self.mock_charm_info(
+            "https://anoth.er/charm-info?charms=cs%3Aseries/name",
+            fail(Error("500")))
+        return self.assert_find_error(
+            "https://anoth.er", "cs:series/name", CharmNotFound,
+            "Charm 'cs:series/name' not found in repository https://anoth.er")
+
+    def test_find_info_bad_revision(self):
+        self.mock_charm_info(
+            "https://anoth.er/charm-info?charms=cs%3Aseries/name-99",
+            succeed(self.charm_info("cs:series/name-99", 1)))
+        return self.assert_find_error(
+            "https://anoth.er", "cs:series/name-99", AssertionError,
+            "bad url revision")
+
+    def test_find_download_error(self):
+        self.mock_charm_info(
+            "https://anoth.er/charm-info?charms=cs%3Aseries/name",
+            succeed(json.dumps({"cs:series/name": {"revision": 123}})))
+        self.mock_download(
+            "https://anoth.er/charm/series/name-123", Error("999"))
+        return self.assert_find_error(
+            "https://anoth.er", "cs:series/name", CharmNotFound,
+            "Charm 'cs:series/name-123' not found in repository "
+            "https://anoth.er")
+
+    def test_find_charm_revision_mismatch(self):
+        self.mock_charm_info(
+            "https://anoth.er/charm-info?charms=cs%3Aseries/name",
+            succeed(json.dumps({"cs:series/name": {"revision": 99}})))
+        self.mock_download("https://anoth.er/charm/series/name-99")
+        return self.assert_find_error(
+            "https://anoth.er", "cs:series/name", AssertionError,
+            "bad charm revision")
+
+    @inlineCallbacks
+    def test_find_downloaded_hash_mismatch(self):
+        cache_location = self.cache_location("cs:series/name-1", 1)
+        self.mock_charm_info(
+            "https://anoth.er/charm-info?charms=cs%3Aseries/name",
+            succeed(json.dumps(
+                {"cs:series/name": {"revision": 1, "sha256": "NO YUO"}})))
+        self.mock_download("https://anoth.er/charm/series/name-1")
+        yield self.assert_find_error(
+            "https://anoth.er", "cs:series/name", CharmError,
+            "Error processing 'cs:series/name-1 (downloaded)': SHA256 "
+            "mismatch")
+        self.assertFalse(os.path.exists(cache_location))
+
+    @inlineCallbacks
+    def test_find_cached_hash_mismatch(self):
+        os.makedirs(self.cache_path)
+        cache_location = self.cache_location("cs:series/name-1", 1)
+        shutil.copy(self.charm.as_bundle().path, cache_location)
+
+        self.mock_charm_info(
+            "https://anoth.er/charm-info?charms=cs%3Aseries/name",
+            succeed(json.dumps(
+                {"cs:series/name": {"revision": 1, "sha256": "NO YUO"}})))
+        yield self.assert_find_error(
+            "https://anoth.er", "cs:series/name", CharmError,
+            "Error processing 'cs:series/name-1 (cached)': SHA256 mismatch")
+        self.assertFalse(os.path.exists(cache_location))
+
+    def test_latest_plain(self):
+        self.mock_charm_info(
+            "https://somewhe.re/charm-info?charms=cs%3Afoo/bar",
+            succeed(self.charm_info("cs:foo/bar", 99)))
+        return self.assert_latest("https://somewhe.re", "cs:foo/bar", 99)
+
+    def test_latest_user(self):
+        self.mock_charm_info(
+            "https://somewhereel.se/charm-info?charms=cs%3A%7Efee/foo/bar",
+            succeed(self.charm_info("cs:~fee/foo/bar", 123)))
+        return self.assert_latest(
+            "https://somewhereel.se", "cs:~fee/foo/bar", 123)
+
+    def test_latest_revision(self):
+        self.mock_charm_info(
+            "https://somewhereel.se/charm-info?charms=cs%3A%7Efee/foo/bar",
+            succeed(self.charm_info("cs:~fee/foo/bar", 123)))
+        return self.assert_latest(
+            "https://somewhereel.se", "cs:~fee/foo/bar-99", 123)
+
+    def test_latest_error(self):
+        self.mock_charm_info(
+            "https://andanoth.er/charm-info?charms=cs%3A%7Eblib/blab/blob",
+            fail(Error("404")))
+        return self.assert_latest_error(
+            "https://andanoth.er", "cs:~blib/blab/blob", CharmNotFound,
+            "Charm 'cs:~blib/blab/blob' not found in repository "
+            "https://andanoth.er")
 
 
 class ResolveTest(RepositoryTestBase):
 
-    @inlineCallbacks
-    def test_resolve_local_with_collection(self):
-        url, charm = yield resolve(
-            "local:series/sample", self.unbundled_repo_path, "series")
-        self.assertEquals(str(url), "local:series/sample")
-        self.assertEquals(charm.metadata.revision, 2)
-
-    @inlineCallbacks
-    def test_resolve_local_no_collection(self):
-        url, charm = yield resolve(
-            "local:sample", self.unbundled_repo_path, "series")
-        self.assertEquals(str(url), "local:series/sample")
-        self.assertEquals(charm.metadata.revision, 2)
-
-    @inlineCallbacks
-    def test_resolve_local_alternative_collection(self):
-        url, charm = yield resolve(
-            "local:series/sample", self.unbundled_repo_path, "otherseries")
-        self.assertEquals(str(url), "local:series/sample")
-        self.assertEquals(charm.metadata.revision, 2)
-
-    def assert_not_found(self, name, default_series):
-        downloadPage = self.mocker.replace("twisted.web.client.downloadPage")
-        downloadPage("https://store.juju.ubuntu.com/charm/series/whatever", ANY)
-        self.mocker.result(fail(Error("404")))
-        self.mocker.replay()
-
-        d = self.assertFailure(
-            resolve(name, None, default_series), CharmNotFound)
-
-        def verify(error):
-            self.assertEquals(
-                str(error),
-                "Charm 'cs:series/whatever' not found in repository "
-                "https://store.juju.ubuntu.com")
-        d.addCallback(verify)
-        return d
-
-    def test_resolve_minimal(self):
-        return self.assert_not_found("whatever", "series")
-
-    def test_resolve_no_root(self):
-        return self.assert_not_found("series/whatever", "series")
-
-    def test_resolve_with_root(self):
-        return self.assert_not_found("cs:series/whatever", "series")
-
-    def test_resolve_nonsense_root(self):
-        d = self.assertFailure(
-            resolve("blah:whatever", None, "series"), CharmURLError)
-
-        def verify(error):
-            self.assertEquals(
-                str(error),
-                "Bad charm URL 'blah:series/whatever': invalid schema (URL "
-                "inferred from 'blah:whatever')")
-        d.addCallback(verify)
-        return d
+    def assert_resolve_local(self, vague, default, expect):
+        repo, url = resolve(vague, "/some/path", default)
+        self.assertEquals(str(url), expect)
+        self.assertTrue(isinstance(repo, LocalCharmRepository))
+        self.assertEquals(repo.path, "/some/path")
+
+    def test_resolve_local(self):
+        self.assert_resolve_local(
+            "local:series/sample", "default", "local:series/sample")
+        self.assert_resolve_local(
+            "local:sample", "default", "local:default/sample")
+
+    def assert_resolve_remote(self, vague, default, expect):
+        repo, url = resolve(vague, None, default)
+        self.assertEquals(str(url), expect)
+        self.assertTrue(isinstance(repo, RemoteCharmRepository))
+        self.assertEquals(repo.url_base, "https://store.juju.ubuntu.com")
+
+    def test_resolve_remote(self):
+        self.assert_resolve_remote(
+            "sample", "default", "cs:default/sample")
+        self.assert_resolve_remote(
+            "series/sample", "default", "cs:series/sample")
+        self.assert_resolve_remote(
+            "cs:sample", "default", "cs:default/sample")
+        self.assert_resolve_remote(
+            "cs:series/sample", "default", "cs:series/sample")
+        self.assert_resolve_remote(
+            "cs:~user/sample", "default", "cs:~user/default/sample")
+        self.assert_resolve_remote(
+            "cs:~user/series/sample", "default", "cs:~user/series/sample")
+
+    def test_resolve_nonsense(self):
+        error = self.assertRaises(
+            CharmURLError, resolve, "blah:whatever", None, "series")
+        self.assertEquals(
+            str(error),
+            "Bad charm URL 'blah:series/whatever': invalid schema (URL "
+            "inferred from 'blah:whatever')")
 
=== modified file 'juju/charm/tests/test_url.py'
--- juju/charm/tests/test_url.py	2011-09-29 03:07:26 +0000
+++ juju/charm/tests/test_url.py	2011-10-05 10:00:46 +0000
@@ -29,6 +29,7 @@
         url = CharmURL.parse(string)
         self.assert_url(url, schema, user, series, name, rev)
         self.assertEquals(str(url), string)
+        self.assertEquals(url.path, string.split(":", 1)[1])
 
     def test_parse(self):
         self.assert_parse(
 
=== modified file 'juju/charm/url.py'
--- juju/charm/url.py	2011-09-29 03:07:26 +0000
+++ juju/charm/url.py	2011-10-05 10:00:46 +0000
@@ -56,6 +56,10 @@
             return "%s/%s" % (self.collection, self.name)
         return "%s/%s-%s" % (self.collection, self.name, self.revision)
 
+    @property
+    def path(self):
+        return str(self).split(":", 1)[1]
+
     def with_revision(self, revision):
         other = copy.deepcopy(self)
         other.revision = revision
 
=== modified file 'juju/control/deploy.py'
--- juju/control/deploy.py	2011-09-30 09:48:25 +0000
+++ juju/control/deploy.py	2011-10-05 10:00:46 +0000
@@ -81,12 +81,11 @@
            service_name, log, config_file=None):
     """Deploy a charm within an environment.
 
-
     This will publish the charm to the environment, creating
     a service from the charm, and get it set to be launched
     on a new machine.
     """
-    charm_url, charm = yield resolve(
+    repo, charm_url = resolve(
         charm_name, repository_path, environment.default_series)
 
     # Validate config options prior to deployment attempt
@@ -95,6 +94,9 @@
     if config_file:
         service_options = parse_config_options(config_file, service_name)
 
+    charm = yield repo.find(charm_url)
+    charm_id = str(charm_url.with_revision(charm.metadata.revision))
+
     provider = environment.get_machine_provider()
     placement_policy = provider.get_placement_policy()
     client = yield provider.connect()
@@ -108,7 +110,6 @@
 
         # Publish the charm to juju
         publisher = CharmPublisher(client, storage)
-        charm_id = str(charm_url.with_revision(charm.metadata.revision))
         yield publisher.add_charm(charm_id, charm)
         result = yield publisher.publish()
 
 
=== modified file 'juju/control/upgrade_charm.py'
--- juju/control/upgrade_charm.py	2011-09-29 03:07:26 +0000
+++ juju/control/upgrade_charm.py	2011-10-05 10:00:46 +0000
@@ -64,11 +64,12 @@
 
     old_charm_url = CharmURL.parse(old_charm_id)
     old_charm_url.assert_revision()
-    charm_url, charm = yield resolve(
+    repo, charm_url = resolve(
         str(old_charm_url.with_revision(None)),
         repository_path,
         environment.default_series)
-    new_charm_url = charm_url.with_revision(charm.metadata.revision)
+    new_charm_url = charm_url.with_revision(
+        (yield repo.latest(charm_url)))
     new_charm_id = str(new_charm_url)
 
     # Verify its newer than what's deployed
@@ -86,6 +87,7 @@
         # Publish the new charm
         storage = provider.get_file_storage()
         publisher = CharmPublisher(client, storage)
+        charm = yield repo.find(charm_url)
         yield publisher.add_charm(new_charm_id, charm)
         result = yield publisher.publish()
         charm_state = result[0]