Merge lp:~fwereade/pyjuju/isolate-formula-revisions into lp:pyjuju

[William Reade]

Notable points:

* CharmBundle.set_revision doesn't work: the use cases for revision-bumping all seem to me to be targeted at the source-code form of a charm (ie a CharmDirectory).
* charms with revision still in metadata.yaml are log.warning()ed
* charms with revision in both places are log.warning()ed in the same way, and the standalone revision file (if valid) is treated as the single point of truth
* charms with no revision at all can't be initialised
* charm metadata prefers to get revision information by calling a function passed in at construction time, to accommodate actual Charms (whose revision could, in theory, change during the lifetime of a Charm object; the result should always be up to date), but will fall back to the value in _data if no getter is present (or sane)
* MetaData is responsible for the warning, which only triggers if we're going through .parse(); when we're storing charm state in ZK, we still need the revision (which is therefore included in get_serialization_data()'s result), and so we need to treat revision-in-yaml as normal and expected when we're calling parse_serialization_data().
* CharmDirectory will, upon initialization, create the appropriate revision file inside the charm's directory, so all the average charm author should have to do is delete one line from metadata.yaml; if they fail to do this, the apparent revision will be frozen even when they change metadata.yaml... BUT, as soon as local auto-updating lands (tomorrow morning(?)), this will be resolved, and they should be free from ever worrying about manually tracking revisions again

...phew. It's not that much code in the end, but I think it warrants fairly close and pedantic examination ;).

[Gustavo Niemeyer (niemeyer)]

This is looking like a good start, but there is one important
conceptual issue we should probably address while we have time.

[1]

+ metadata = MetaData(get_revision_content=self._get_revision_content)
+ metadata.parse(zf.read("metadata.yaml"))
+ if metadata.revision is None:
+ raise CharmError(path, "has no revision")

Hmmm.. this is feeling a bit like a hack. Right now the concepts within
the charm are well separated in the API as well.. we have the config
with the contents of config.yaml and the metadata with the
contents of metadata.yaml. We're changing the location of the
revision information so that we separate logically the bits that are
human-editable from the bits that are automatically changed. If this
is a good idea, we should do this across the board, and take the
revision out of the metadata as well within the code base.

So, let's remove metadata.revision, and introduce something like a
metadata.obsolete_revision which is only accessed from the charm
implementation itself. Then, inside the charm implementation let's
introduce a method called get_revision() which either picks the
revision from the revision file or from the metadata.obsolete_revision
if the former is missing. Every other place that looks at
metadata.revision today should be changed to look at
charm.get_revision() instead, or to something contextually equivalent.
The storage and API in juju/state/charm.py will also have to change
accordingly, for instance.

This is certainly what we would have done if we started with the
revision separated in the first place, for instance.

Perhaps this will turn out to be too painful, but if we cannot do this
on time, I'd rather not change the revision location, otherwise we'll
be introducing a half-baked feature that looks like one thing in some
places and like something else in others, and forever have that hackish
taste when dealing with it.

[2]

+ def set_revision(self, revision):
+ with open(os.path.join(self.path, "revision"), "w") as f:
+ f.write(str(revision))

That's nice, and it shows the above point. set_revision is in the
charm, not in the metadata. That's where we need get_revision to
be as well.

[3]

+ try:
+ return int(self._get_revision_content())
+ except (ValueError, TypeError):
+ pass

If the file exists, and its content is invalid, we need to raise a
real error about the problem than pretending it doesn't exist.

[4]

+ if "revision" in self._data:
+ suffix = (" from %r" % path) if path else ""
+ log.warning(
+ "metadata file schema has changed: revision should be "
+ "removed" + suffix)

I suggest only logging the warning if the file path is known, as I
believe this will mean the user has a better chance of being able to
fix the problem rather than being just a consumer. Is that the case?

If so, I suggest something like this:

    if "revision" in self._data and path:
        log.warning(
            "%s: metadata.yaml: revision field is obsolete. "
            "Move it to the 'revision' file." % path)

review: Needs Fixing

[lp:~fwereade/pyjuju/isolate-formula-revisions]

382. By William Reade on 2011-10-05

merge trunk

[William Reade (fwereade)]

[1,2]

> Hmmm.. this is feeling a bit like a hack.

Guilty as charged, but I'd say it's quite well bounded.

> The storage and API in juju/state/charm.py will also have to change
> accordingly, for instance.

That's the core of the problem: that I thought I'd done enough damage in the last week or so, and that it would be important *not* to break charm-state storage and thus have to bump topology.VERSION. I'll go ahead with the nicer but more disruptive approach you propose in a stacked branch.

[3,4]

Good points; I'll fix them in this branch.

[lp:~fwereade/pyjuju/isolate-formula-revisions]

383. By William Reade on 2011-10-05

easy review points

[William Reade (fwereade)]

> That's the core of the problem: that I thought I'd done enough damage in the
> last week or so, and that it would be important *not* to break charm-state
> storage and thus have to bump topology.VERSION. I'll go ahead with the nicer
> but more disruptive approach you propose in a stacked branch.

But... just possibly... we don't actually need .revision on metadata *at all*, because CharmState takes if from a CharmURL. Stacking it anyway, just to be safe.

[lp:~fwereade/pyjuju/isolate-formula-revisions]

384. By William Reade on 2011-10-05

merge trunk

[William Reade (fwereade)]

Merged back from lp:~fwereade/juju/really-isolate-formula-revisions

MetaData no longer has .revision; just .obsolete_revision. Charms have .get_revision() to compensate, which uses metadata.obsolete_revision only when a real revision file is not present.

[lp:~fwereade/pyjuju/isolate-formula-revisions]

385. By William Reade on 2011-10-05

merge back from temporary really-isolate-formula-revision branch

[Gustavo Niemeyer (niemeyer)]

This looks fantastic. Just a few trivials, and let's try to get a +1 from Kapil.

[5]

+ revision = self.get_revision()
+ if revision is None:
+ raise CharmError(path, "has no revision")

Hmmm.. both implementations are doing this, so maybe the common
get_revision function itself could take care of it if neither
options have a valid revision.

[6]

+ revision = self.get_revision()
+ if revision is None:
+ raise CharmError(path, "has no revision")
+ if self._get_revision_file_content() is None:
+ self.set_revision(revision)

_get_revision_file_content is being called necessarily and at
least twice every time. Plus when the revision is actually
wanted outside of the implementation.

Maybe we should cache in a private attibute instead?

[7]

+ def set_revision(self, revision):
+ with open(os.path.join(self.path, "revision"), "w") as f:
+ f.write(str(revision))

A "\n" at the end would be nice to deal with it from the shell.

review: Approve

[lp:~fwereade/pyjuju/isolate-formula-revisions]

386. By William Reade on 2011-10-05

addressed review points

[Kapil Thangavelu (hazmat)]

Looks good to me

review: Approve

[lp:~fwereade/pyjuju/isolate-formula-revisions]

387. By William Reade on 2011-10-05

strip file contents before int()ing (explicit > implicit)

388. By William Reade on 2011-10-05

just read charm revisions once, inline in __init__

=== modified file 'examples/oneiric/mysql/metadata.yaml'
--- examples/oneiric/mysql/metadata.yaml	2011-09-15 19:24:47 +0000
+++ examples/oneiric/mysql/metadata.yaml	2011-10-05 18:13:26 +0000
@@ -1,5 +1,4 @@
 name: mysql
-revision: 11
 summary: "MySQL relational database provider"
 description: |
     Installs and configures the MySQL package (mysqldb), then runs it.

=== added file 'examples/oneiric/mysql/revision'
--- examples/oneiric/mysql/revision	1970-01-01 00:00:00 +0000
+++ examples/oneiric/mysql/revision	2011-10-05 18:13:26 +0000
@@ -0,0 +1,1 @@
+11

=== modified file 'examples/oneiric/php/metadata.yaml'
--- examples/oneiric/php/metadata.yaml	2011-09-15 19:24:47 +0000
+++ examples/oneiric/php/metadata.yaml	2011-10-05 18:13:26 +0000
@@ -1,5 +1,4 @@
 name: php
-revision: 5
 summary: "php container"
 description: |
   PHP environment for your code.

=== added file 'examples/oneiric/php/revision'
--- examples/oneiric/php/revision	1970-01-01 00:00:00 +0000
+++ examples/oneiric/php/revision	2011-10-05 18:13:26 +0000
@@ -0,0 +1,1 @@
+5

=== modified file 'examples/oneiric/wordpress/metadata.yaml'
--- examples/oneiric/wordpress/metadata.yaml	2011-09-15 19:24:47 +0000
+++ examples/oneiric/wordpress/metadata.yaml	2011-10-05 18:13:26 +0000
@@ -1,5 +1,4 @@
 name: wordpress
-revision: 31
 summary: "WordPress blog"
 description: |
     Installs WordPress package (wordpress). Upon the database provider

=== added file 'examples/oneiric/wordpress/revision'
--- examples/oneiric/wordpress/revision	1970-01-01 00:00:00 +0000
+++ examples/oneiric/wordpress/revision	2011-10-05 18:13:26 +0000
@@ -0,0 +1,1 @@
+31

=== modified file 'juju/agents/tests/test_machine.py'
--- juju/agents/tests/test_machine.py	2011-09-28 09:48:30 +0000
+++ juju/agents/tests/test_machine.py	2011-10-05 18:13:26 +0000
@@ -170,7 +170,7 @@
         self.assertTrue(os.path.exists(charm_path))
         bundle = CharmBundle(charm_path)
         self.assertEquals(
-            bundle.metadata.revision, self.charm.metadata.revision)
+            bundle.get_revision(), self.charm.get_revision())
         self.assertEquals(bundle.get_sha256(), checksum)
         self.assertIn(
             "Downloading charm %s" % charm_id, self.output.getvalue())

=== modified file 'juju/agents/tests/test_unit.py'
--- juju/agents/tests/test_unit.py	2011-10-04 21:20:30 +0000
+++ juju/agents/tests/test_unit.py	2011-10-05 18:13:26 +0000
@@ -598,7 +598,7 @@
             os.path.join(self.agent.unit_directory, "charm"))
 
         self.assertEqual(
-            self.charm.metadata.revision + 1, new_charm.metadata.revision)
+            self.charm.get_revision() + 1, new_charm.get_revision())
 
     @inlineCallbacks
     def test_agent_upgrade_bad_unit_state(self):
@@ -694,7 +694,7 @@
             os.path.join(self.agent.unit_directory, "charm"))
 
         self.assertEqual(
-            self.charm.metadata.revision + 1, new_charm.metadata.revision)
+            self.charm.get_revision() + 1, new_charm.get_revision())
 
         # Verify upgrade flag is cleared.
         self.assertFalse((yield self.states["unit"].get_upgrade_flag()))

=== modified file 'juju/charm/base.py'
--- juju/charm/base.py	2011-09-15 18:50:23 +0000
+++ juju/charm/base.py	2011-10-05 18:13:26 +0000
@@ -1,3 +1,20 @@
+from juju.errors import CharmError
+
+
+def get_revision(file_content, metadata, path):
+    if file_content is None:
+        result = metadata.obsolete_revision
+        if result is None:
+            raise CharmError(path, "has no revision")
+    else:
+        message = "invalid charm revision %r" % file_content
+        try:
+            result = int(file_content.strip())
+        except (ValueError, TypeError):
+            raise CharmError(path, message)
+        if result < 0:
+            raise CharmError(path, message)
+    return result
 
 
 class CharmBase(object):
@@ -10,6 +27,20 @@
         raise NotImplementedError("%s.%s not supported" %
                                   (self.__class__.__name__, attr))
 
+    def get_revision(self):
+        """Get the revision, preferably from the revision file.
+
+        Will fall back to metadata if not available.
+        """
+        self._unsupported("get_revision()")
+
+    def set_revision(self, revision):
+        """Update the revision file, if possible.
+
+        Some subclasses may not be able to do this.
+        """
+        self._unsupported("set_revision()")
+
     def as_bundle(self):
         """Transform this charm into a charm bundle, if possible.
 

=== modified file 'juju/charm/bundle.py'
--- juju/charm/bundle.py	2011-09-15 18:50:23 +0000
+++ juju/charm/bundle.py	2011-10-05 18:13:26 +0000
@@ -4,7 +4,7 @@
 
 from zipfile import ZipFile, BadZipfile
 
-from juju.charm.base import CharmBase
+from juju.charm.base import CharmBase, get_revision
 from juju.charm.config import ConfigOptions
 from juju.charm.metadata import MetaData
 from juju.errors import CharmError
@@ -15,28 +15,31 @@
     """ZIP-archive that contains charm directory content."""
 
     def __init__(self, path):
+        self.path = isinstance(path, file) and path.name or path
         try:
             zf = ZipFile(path, 'r')
         except BadZipfile, exc:
             raise CharmError(path, "must be a zip file (%s)" % exc)
 
-        metadata = MetaData()
         if "metadata.yaml" not in zf.namelist():
-            raise CharmError(path,
-                               "archive does not contain required file "
-                               "\"metadata.yaml\"")
-
-        content = zf.read("metadata.yaml")
-        metadata.parse(content)
-        self.metadata = metadata
-
-        config = ConfigOptions()
+            raise CharmError(
+                path, "charm does not contain required file 'metadata.yaml'")
+        self.metadata = MetaData()
+        self.metadata.parse(zf.read("metadata.yaml"))
+
+        try:
+            revision_content = zf.read("revision")
+        except KeyError:
+            revision_content = None
+        self._revision = get_revision(
+            revision_content, self.metadata, self.path)
+
+        self.config = ConfigOptions()
         if "config.yaml" in zf.namelist():
-            content = zf.read("config.yaml")
-            config.parse(content)
-        self.config = config
+            self.config.parse(zf.read("config.yaml"))
 
-        self.path = isinstance(path, file) and path.name or path
+    def get_revision(self):
+        return self._revision
 
     def compute_sha256(self):
         """Return the SHA256 digest for this charm bundle.
@@ -49,7 +52,7 @@
         """Extract the bundle to directory path and return a
         CharmDirectory handle"""
         from .directory import CharmDirectory
-        zf = ZipFile(self.path, 'r')
+        zf = ZipFile(self.path, "r")
         for info in zf.infolist():
             mode = info.external_attr >> 16
             extract_path = zf.extract(info, directory_path)

=== modified file 'juju/charm/directory.py'
--- juju/charm/directory.py	2011-09-15 19:24:47 +0000
+++ juju/charm/directory.py	2011-10-05 18:13:26 +0000
@@ -2,10 +2,11 @@
 import zipfile
 import tempfile
 
+from juju.charm.base import CharmBase, get_revision
+from juju.charm.bundle import CharmBundle
 from juju.charm.config import ConfigOptions
 from juju.charm.metadata import MetaData
-from juju.charm.bundle import CharmBundle
-from juju.charm.base import CharmBase
+from juju.errors import CharmError
 
 
 class CharmDirectory(CharmBase):
@@ -22,11 +23,30 @@
     def __init__(self, path):
         self.path = path
         self.metadata = MetaData(os.path.join(path, "metadata.yaml"))
+
+        revision_content = None
+        revision_path = os.path.join(self.path, "revision")
+        if os.path.exists(revision_path):
+            with open(revision_path) as f:
+                revision_content = f.read()
+        self._revision = get_revision(
+            revision_content, self.metadata, self.path)
+        if revision_content is None:
+            self.set_revision(self._revision)
+
         self.config = ConfigOptions()
         self.config.load(os.path.join(path, "config.yaml"))
         self._temp_bundle = None
         self._temp_bundle_file = None
 
+    def get_revision(self):
+        return self._revision
+
+    def set_revision(self, revision):
+        self._revision = revision
+        with open(os.path.join(self.path, "revision"), "w") as f:
+            f.write(str(revision) + "\n")
+
     def make_archive(self, path):
         """Create archive of directory and write to ``path``.
 
@@ -60,8 +80,7 @@
 
     def as_bundle(self):
         if self._temp_bundle is None:
-            prefix = "%s-%d.charm." % \
-                     (self.metadata.name, self.metadata.revision)
+            prefix = "%s-%d.charm." % (self.metadata.name, self.get_revision())
             temp_file = tempfile.NamedTemporaryFile(prefix=prefix)
             self.make_archive(temp_file.name)
             self._temp_bundle = CharmBundle(temp_file.name)

=== modified file 'juju/charm/metadata.py'
--- juju/charm/metadata.py	2011-09-28 23:04:39 +0000
+++ juju/charm/metadata.py	2011-10-05 18:13:26 +0000
@@ -1,15 +1,17 @@
+import logging
 import os
 
 import yaml
 
 from juju.errors import JujuError, FileNotFound
-from juju.charm.errors import CharmURLError
-from juju.charm.url import CharmURL
 from juju.lib.schema import (
     SchemaError, Bool, Constant, Dict, Int,
     KeyDict, OneOf, UnicodeOrString)
 
 
+log = logging.getLogger("juju.charm")
+
+
 class MetaDataError(JujuError):
     """Raised when an error in the info file of a charm is found."""
 
@@ -88,7 +90,7 @@
     "peers": Dict(UTF8_SCHEMA, InterfaceExpander(limit=1)),
     "provides": Dict(UTF8_SCHEMA, InterfaceExpander(limit=None)),
     "requires": Dict(UTF8_SCHEMA, InterfaceExpander(limit=1)),
-    }, optional=set(["provides", "requires", "peers"]))
+    }, optional=set(["provides", "requires", "peers", "revision"]))
 
 
 class MetaData(object):
@@ -110,12 +112,13 @@
         return self._data.get("name")
 
     @property
-    def revision(self):
+    def obsolete_revision(self):
         """The charm revision.
 
         The charm revision acts as a version, but unlike e.g. package
         versions, the charm revision is a monotonically increasing
-        integer.
+        integer. This should not be stored in metadata any more, but remains
+        for backward compatibility's sake.
         """
         return self._data.get("revision")
 
@@ -174,7 +177,11 @@
 
         @raise MetaDataError: When errors are found in the info data.
         """
-        return self.parse_serialization_data(yaml.load(content), path)
+        self.parse_serialization_data(yaml.load(content), path)
+        if "revision" in self._data and path:
+            log.warning(
+                "%s: revision field is obsolete. Move it to the 'revision' "
+                "file." % path)
 
     def parse_serialization_data(self, serialization_data, path=None):
         """Parse the unprocessed serialization data and load in this instance.

=== modified file 'juju/charm/repository.py'
--- juju/charm/repository.py	2011-10-05 09:51:16 +0000
+++ juju/charm/repository.py	2011-10-05 18:13:26 +0000
@@ -61,20 +61,20 @@
         latest = None
         for charm in self._collection(charm_url.collection):
             if charm.metadata.name == charm_url.name:
-                if charm.metadata.revision == charm_url.revision:
+                if charm.get_revision() == charm_url.revision:
                     return succeed(charm)
                 if (latest is None or
-                    latest.metadata.revision < charm.metadata.revision):
+                    latest.get_revision() < charm.get_revision()):
                     latest = charm
 
-        if latest is None:
+        if latest is None or charm_url.revision is not None:
             return fail(CharmNotFound(self.path, charm_url))
 
         return succeed(latest)
 
     def latest(self, charm_url):
         d = self.find(charm_url.with_revision(None))
-        d.addCallback(lambda c: c.metadata.revision)
+        d.addCallback(lambda c: c.get_revision())
         return d
 
 
@@ -128,7 +128,7 @@
             yield self._download(charm_url, cache_path)
         charm = get_charm_from_path(cache_path)
 
-        assert charm.metadata.revision == revision, "bad charm revision"
+        assert charm.get_revision() == revision, "bad charm revision"
         if charm.get_sha256() != info["sha256"]:
             os.remove(cache_path)
             name = "%s (%s)" % (

=== modified file 'juju/charm/tests/__init__.py'
--- juju/charm/tests/__init__.py	2011-09-28 00:36:30 +0000
+++ juju/charm/tests/__init__.py	2011-10-05 18:13:26 +0000
@@ -1,3 +1,3 @@
 def local_charm_id(charm):
     return "local:series/%s-%s" % (
-        charm.metadata.name, charm.metadata.revision)
+        charm.metadata.name, charm.get_revision())

=== modified file 'juju/charm/tests/repository/series/dummy/metadata.yaml'
--- juju/charm/tests/repository/series/dummy/metadata.yaml	2011-09-15 19:24:47 +0000
+++ juju/charm/tests/repository/series/dummy/metadata.yaml	2011-10-05 18:13:26 +0000
@@ -1,5 +1,4 @@
 name: dummy
-revision: 1
 summary: "That's a dummy charm."
 description: |
     This is a longer description which

=== added file 'juju/charm/tests/repository/series/dummy/revision'
--- juju/charm/tests/repository/series/dummy/revision	1970-01-01 00:00:00 +0000
+++ juju/charm/tests/repository/series/dummy/revision	2011-10-05 18:13:26 +0000
@@ -0,0 +1,1 @@
+1
\ No newline at end of file

=== modified file 'juju/charm/tests/repository/series/mysql-alternative/metadata.yaml'
--- juju/charm/tests/repository/series/mysql-alternative/metadata.yaml	2011-09-28 08:55:57 +0000
+++ juju/charm/tests/repository/series/mysql-alternative/metadata.yaml	2011-10-05 18:13:26 +0000
@@ -1,5 +1,4 @@
 name: mysql-alternative
-revision: 1
 summary: "Database engine"
 description: "A pretty popular database"
 provides:

=== added file 'juju/charm/tests/repository/series/mysql-alternative/revision'
--- juju/charm/tests/repository/series/mysql-alternative/revision	1970-01-01 00:00:00 +0000
+++ juju/charm/tests/repository/series/mysql-alternative/revision	2011-10-05 18:13:26 +0000
@@ -0,0 +1,1 @@
+1
\ No newline at end of file

=== modified file 'juju/charm/tests/repository/series/mysql/metadata.yaml'
--- juju/charm/tests/repository/series/mysql/metadata.yaml	2011-09-15 19:24:47 +0000
+++ juju/charm/tests/repository/series/mysql/metadata.yaml	2011-10-05 18:13:26 +0000
@@ -1,5 +1,4 @@
 name: mysql
-revision: 1
 summary: "Database engine"
 description: "A pretty popular database"
 provides:

=== added file 'juju/charm/tests/repository/series/mysql/revision'
--- juju/charm/tests/repository/series/mysql/revision	1970-01-01 00:00:00 +0000
+++ juju/charm/tests/repository/series/mysql/revision	2011-10-05 18:13:26 +0000
@@ -0,0 +1,1 @@
+1
\ No newline at end of file

=== modified file 'juju/charm/tests/repository/series/new/metadata.yaml'
--- juju/charm/tests/repository/series/new/metadata.yaml	2011-09-15 19:24:47 +0000
+++ juju/charm/tests/repository/series/new/metadata.yaml	2011-10-05 18:13:26 +0000
@@ -1,5 +1,4 @@
 name: sample
-revision: 2
 summary: "That's a sample charm."
 description: |
     This is a longer description which

=== added file 'juju/charm/tests/repository/series/new/revision'
--- juju/charm/tests/repository/series/new/revision	1970-01-01 00:00:00 +0000
+++ juju/charm/tests/repository/series/new/revision	2011-10-05 18:13:26 +0000
@@ -0,0 +1,1 @@
+2

=== modified file 'juju/charm/tests/repository/series/old/metadata.yaml'
--- juju/charm/tests/repository/series/old/metadata.yaml	2011-09-15 19:24:47 +0000
+++ juju/charm/tests/repository/series/old/metadata.yaml	2011-10-05 18:13:26 +0000
@@ -1,5 +1,4 @@
 name: sample
-revision: 1
 summary: "That's a sample charm."
 description: |
     This is a longer description which

=== added file 'juju/charm/tests/repository/series/old/revision'
--- juju/charm/tests/repository/series/old/revision	1970-01-01 00:00:00 +0000
+++ juju/charm/tests/repository/series/old/revision	2011-10-05 18:13:26 +0000
@@ -0,0 +1,1 @@
+1

=== modified file 'juju/charm/tests/repository/series/riak/metadata.yaml'
--- juju/charm/tests/repository/series/riak/metadata.yaml	2011-09-15 19:24:47 +0000
+++ juju/charm/tests/repository/series/riak/metadata.yaml	2011-10-05 18:13:26 +0000
@@ -1,5 +1,4 @@
 name: riak
-revision: 7
 summary: "K/V storage engine"
 description: "Scalable K/V Store in Erlang with Clocks :-)"
 provides:

=== added file 'juju/charm/tests/repository/series/riak/revision'
--- juju/charm/tests/repository/series/riak/revision	1970-01-01 00:00:00 +0000
+++ juju/charm/tests/repository/series/riak/revision	2011-10-05 18:13:26 +0000
@@ -0,0 +1,1 @@
+7
\ No newline at end of file

=== modified file 'juju/charm/tests/repository/series/varnish-alternative/metadata.yaml'
--- juju/charm/tests/repository/series/varnish-alternative/metadata.yaml	2011-09-28 08:55:57 +0000
+++ juju/charm/tests/repository/series/varnish-alternative/metadata.yaml	2011-10-05 18:13:26 +0000
@@ -1,5 +1,4 @@
 name: varnish-alternative
-revision: 1
 summary: "Database engine"
 description: "Another popular database"
 provides:

=== added file 'juju/charm/tests/repository/series/varnish-alternative/revision'
--- juju/charm/tests/repository/series/varnish-alternative/revision	1970-01-01 00:00:00 +0000
+++ juju/charm/tests/repository/series/varnish-alternative/revision	2011-10-05 18:13:26 +0000
@@ -0,0 +1,1 @@
+1
\ No newline at end of file

=== modified file 'juju/charm/tests/repository/series/varnish/metadata.yaml'
--- juju/charm/tests/repository/series/varnish/metadata.yaml	2011-09-15 19:24:47 +0000
+++ juju/charm/tests/repository/series/varnish/metadata.yaml	2011-10-05 18:13:26 +0000
@@ -1,5 +1,4 @@
 name: varnish
-revision: 1
 summary: "Database engine"
 description: "Another popular database"
 provides:

=== added file 'juju/charm/tests/repository/series/varnish/revision'
--- juju/charm/tests/repository/series/varnish/revision	1970-01-01 00:00:00 +0000
+++ juju/charm/tests/repository/series/varnish/revision	2011-10-05 18:13:26 +0000
@@ -0,0 +1,1 @@
+1
\ No newline at end of file

=== modified file 'juju/charm/tests/repository/series/wordpress/metadata.yaml'
--- juju/charm/tests/repository/series/wordpress/metadata.yaml	2011-09-15 19:24:47 +0000
+++ juju/charm/tests/repository/series/wordpress/metadata.yaml	2011-10-05 18:13:26 +0000
@@ -1,5 +1,4 @@
 name: wordpress
-revision: 3
 summary: "Blog engine"
 description: "A pretty popular blog engine"
 provides:

=== added file 'juju/charm/tests/repository/series/wordpress/revision'
--- juju/charm/tests/repository/series/wordpress/revision	1970-01-01 00:00:00 +0000
+++ juju/charm/tests/repository/series/wordpress/revision	2011-10-05 18:13:26 +0000
@@ -0,0 +1,1 @@
+3
\ No newline at end of file

=== modified file 'juju/charm/tests/test_base.py'
--- juju/charm/tests/test_base.py	2011-09-15 18:50:23 +0000
+++ juju/charm/tests/test_base.py	2011-10-05 18:13:26 +0000
@@ -1,5 +1,9 @@
+import yaml
+
+from juju.charm.base import CharmBase, get_revision
+from juju.charm.metadata import MetaData
+from juju.errors import CharmError
 from juju.lib.testing import TestCase
-from juju.charm.base import CharmBase
 
 
 class MyCharm(CharmBase):
@@ -20,21 +24,13 @@
         else:
             self.fail("MyCharm.%s didn't fail" % attr_name)
 
-    def test_as_bundle_is_unsupported(self):
-        self.assertUnsupported(lambda: self.charm.as_bundle(),
-                               "as_bundle()")
-
-    def test_compute_sha256_is_unsupported(self):
-        self.assertUnsupported(lambda: self.charm.compute_sha256(),
-                               "compute_sha256()")
-
-    def test_get_sha256_fails_if_not_preset(self):
-        """
-        get_sha256() will internally call compute_sha256() if
-        the digest value hasn't been previously set.
-        """
-        self.assertUnsupported(lambda: self.charm.get_sha256(),
-                               "compute_sha256()")
+    def test_unsupported(self):
+        self.assertUnsupported(self.charm.as_bundle, "as_bundle()")
+        self.assertUnsupported(self.charm.get_sha256, "compute_sha256()")
+        self.assertUnsupported(self.charm.compute_sha256, "compute_sha256()")
+        self.assertUnsupported(self.charm.get_revision, "get_revision()")
+        self.assertUnsupported(
+            lambda: self.charm.set_revision(1), "set_revision()")
 
     def test_compute_and_cache_sha256(self):
         """
@@ -53,3 +49,33 @@
         sha256 = ["anothervalue"]
         # Should still be the same, since the old one was cached.
         self.assertEquals(charm.get_sha256(), "mysha")
+
+
+class GetRevisionTest(TestCase):
+
+    def assert_good_content(self, content, value):
+        self.assertEquals(get_revision(content, None, None), value)
+
+    def assert_bad_content(self, content):
+        err = self.assertRaises(
+            CharmError, get_revision, content, None, "path")
+        self.assertEquals(
+            str(err),
+            "Error processing 'path': invalid charm revision %r" % content)
+
+    def test_with_content(self):
+        self.assert_good_content("0\n", 0)
+        self.assert_good_content("123\n", 123)
+        self.assert_bad_content("")
+        self.assert_bad_content("-1\n")
+        self.assert_bad_content("three hundred and six or so")
+
+    def test_metadata_fallback(self):
+        metadata = MetaData()
+        err = self.assertRaises(
+            CharmError, get_revision, None, metadata, "path")
+        self.assertEquals(
+            str(err), "Error processing 'path': has no revision")
+        metadata.parse(yaml.dump({
+            "name": "x", "summary": "y", "description": "z","revision": 33}))
+        self.assertEquals(get_revision(None, metadata, None), 33)

=== modified file 'juju/charm/tests/test_bundle.py'
--- juju/charm/tests/test_bundle.py	2011-09-28 08:55:57 +0000
+++ juju/charm/tests/test_bundle.py	2011-10-05 18:13:26 +0000
@@ -1,7 +1,7 @@
 import os
-import stat
 import hashlib
 import inspect
+import yaml
 import zipfile
 
 from juju.lib.testing import TestCase
@@ -35,16 +35,11 @@
 
     def test_error_not_zip(self):
         filename = self.makeFile("@#$@$")
-        try:
-            CharmBundle(filename)
-        except CharmError, exc:
-            self.assertEquals(
-                str(exc),
-                ("Error processing %r: " % filename) +
-                "must be a zip file (File is not a zip file)")
-            return
-
-        self.fail("Expected charm error.")
+        err = self.assertRaises(CharmError, CharmBundle, filename)
+        self.assertEquals(
+            str(err),
+            "Error processing %r: must be a zip file (File is not a zip file)"
+            % filename)
 
     def test_error_zip_but_doesnt_have_metadata_file(self):
         filename = self.makeFile()
@@ -52,16 +47,60 @@
         zf.writestr("README.txt", "This is not a valid charm.")
         zf.close()
 
-        try:
-            CharmBundle(filename)
-        except CharmError, exc:
-            self.assertEquals(
-                str(exc),
-                ("Error processing %r: " % filename) +
-                "archive does not contain required file \"metadata.yaml\"")
-            return
-
-        self.fail("Expected charm error.")
+        err = self.assertRaises(CharmError, CharmBundle, filename)
+        self.assertEquals(
+            str(err),
+            "Error processing %r: charm does not contain required "
+            "file 'metadata.yaml'" % filename)
+
+    def test_no_revision_at_all(self):
+        filename = self.makeFile()
+        zf_dst = zipfile.ZipFile(filename, "w")
+        zf_src = zipfile.ZipFile(self.filename, "r")
+        for name in zf_src.namelist():
+            if name == "revision":
+                continue
+            zf_dst.writestr(name, zf_src.read(name))
+        zf_src.close()
+        zf_dst.close()
+
+        err = self.assertRaises(CharmError, CharmBundle, filename)
+        self.assertEquals(
+            str(err), "Error processing %r: has no revision" % filename)
+
+    def test_revision_in_metadata(self):
+        filename = self.makeFile()
+        zf_dst = zipfile.ZipFile(filename, "w")
+        zf_src = zipfile.ZipFile(self.filename, "r")
+        for name in zf_src.namelist():
+            if name == "revision":
+                continue
+            content = zf_src.read(name)
+            if name == "metadata.yaml":
+                data = yaml.load(content)
+                data["revision"] = 303
+                content = yaml.dump(data)
+            zf_dst.writestr(name, content)
+        zf_src.close()
+        zf_dst.close()
+
+        charm = CharmBundle(filename)
+        self.assertEquals(charm.get_revision(), 303)
+
+    def test_competing_revisions(self):
+        zf = zipfile.ZipFile(self.filename, "a")
+        zf.writestr("revision", "999")
+        data = yaml.load(zf.read("metadata.yaml"))
+        data["revision"] = 303
+        zf.writestr("metadata.yaml", yaml.dump(data))
+        zf.close()
+
+        charm = CharmBundle(self.filename)
+        self.assertEquals(charm.get_revision(), 999)
+
+    def test_cannot_set_revision(self):
+        charm = CharmBundle(self.filename)
+        self.assertRaises(NotImplementedError, charm.set_revision, 123)
 
     def test_bundled_config(self):
         """Make sure that config is accessible from a bundle."""

=== modified file 'juju/charm/tests/test_directory.py'
--- juju/charm/tests/test_directory.py	2011-09-28 10:38:16 +0000
+++ juju/charm/tests/test_directory.py	2011-10-05 18:13:26 +0000
@@ -1,11 +1,13 @@
+import gc
 import os
 import hashlib
 import inspect
+import shutil
 import tempfile
+import yaml
 import zipfile
-import gc
 
-from juju.errors import FileNotFound
+from juju.errors import CharmError, FileNotFound
 from juju.charm.metadata import MetaData
 from juju.charm.directory import CharmDirectory
 from juju.charm.bundle import CharmBundle
@@ -31,10 +33,62 @@
         if not os.path.isdir(empty_dir):
             os.mkdir(empty_dir)
 
+    def copy_charm(self):
+        dir_ = os.path.join(tempfile.mkdtemp(), "sample")
+        shutil.copytree(sample_directory, dir_)
+        return dir_
+
+    def delete_revision(self, dir_):
+        os.remove(os.path.join(dir_, "revision"))
+
+    def set_metadata_revision(self, dir_, revision):
+        metadata_path = os.path.join(dir_, "metadata.yaml")
+        with open(metadata_path) as f:
+            data = yaml.load(f.read())
+        data["revision"] = 999
+        with open(metadata_path, "w") as f:
+            f.write(yaml.dump(data))
+
     def test_metadata_is_required(self):
         directory = self.makeDir()
         self.assertRaises(FileNotFound, CharmDirectory, directory)
 
+    def test_no_revision(self):
+        dir_ = self.copy_charm()
+        self.delete_revision(dir_)
+        err = self.assertRaises(CharmError, CharmDirectory, dir_)
+        self.assertEquals(
+            str(err), "Error processing %r: has no revision" % dir_)
+
+    def test_revision_in_metadata(self):
+        dir_ = self.copy_charm()
+        self.delete_revision(dir_)
+        self.set_metadata_revision(dir_, 999)
+        log = self.capture_logging("juju.charm")
+        charm = CharmDirectory(dir_)
+        self.assertEquals(charm.get_revision(), 999)
+        self.assertIn(
+            "revision field is obsolete. Move it to the 'revision' file.",
+            log.getvalue())
+
+    def test_competing_revisions(self):
+        dir_ = self.copy_charm()
+        self.set_metadata_revision(dir_, 999)
+        log = self.capture_logging("juju.charm")
+        charm = CharmDirectory(dir_)
+        self.assertEquals(charm.get_revision(), 1)
+        self.assertIn(
+            "revision field is obsolete. Move it to the 'revision' file.",
+            log.getvalue())
+
+    def test_set_revision(self):
+        dir_ = self.copy_charm()
+        charm = CharmDirectory(dir_)
+        charm.set_revision(123)
+        self.assertEquals(charm.get_revision(), 123)
+        with open(os.path.join(dir_, "revision")) as f:
+            self.assertEquals(f.read(), "123\n")
+
     def test_info(self):
         directory = CharmDirectory(sample_directory)
         self.assertTrue(directory.metadata is not None)
@@ -57,7 +111,7 @@
         self.assertEqual(
             set(included),
             set(("metadata.yaml", "empty/", "src/", "src/hello.c",
-                 "config.yaml", "hooks/", "hooks/install")))
+                 "config.yaml", "hooks/", "hooks/install", "revision")))
 
     def test_as_bundle(self):
         directory = CharmDirectory(self.sample_dir1)
@@ -66,10 +120,14 @@
         self.assertEquals(charm_bundle.metadata.name, "sample")
         self.assertIn("sample-1.charm", charm_bundle.path)
 
+        total_compressed = 0
+        total_uncompressed = 0
         zip_file = zipfile.ZipFile(charm_bundle.path)
         for n in zip_file.namelist():
             info = zip_file.getinfo(n)
-            self.assertTrue(info.compress_size < info.file_size)
+            total_compressed += info.compress_size
+            total_uncompressed += info.file_size
+        self.assertTrue(total_compressed < total_uncompressed)
 
     def test_as_bundle_file_lifetime(self):
         """

=== modified file 'juju/charm/tests/test_metadata.py'
--- juju/charm/tests/test_metadata.py	2011-09-28 23:04:39 +0000
+++ juju/charm/tests/test_metadata.py	2011-10-05 18:13:26 +0000
@@ -5,7 +5,6 @@
 import inspect
 
 from juju.charm import tests
-from juju.charm.errors import CharmURLError
 from juju.charm.metadata import (
     MetaData, MetaDataError, InterfaceExpander, SchemaError)
 from juju.errors import FileNotFound
@@ -57,7 +56,7 @@
         Attributes should be set to None before anything is loaded.
         """
         self.assertEquals(self.metadata.name, None)
-        self.assertEquals(self.metadata.revision, None)
+        self.assertEquals(self.metadata.obsolete_revision, None)
         self.assertEquals(self.metadata.summary, None)
         self.assertEquals(self.metadata.description, None)
 
@@ -68,11 +67,38 @@
         """
         self.metadata.parse(self.sample)
         self.assertEquals(self.metadata.name, "dummy")
-        self.assertEquals(self.metadata.revision, 1)
-        self.assertEquals(self.metadata.summary, u"That's a dummy charm.")
-        self.assertEquals(self.metadata.description,
-                          u"This is a longer description which\n"
-                          u"potentially contains multiple lines.\n")
+        self.assertEquals(self.metadata.obsolete_revision, None)
+        self.assertEquals(self.metadata.summary, u"That's a dummy charm.")
+        self.assertEquals(self.metadata.description,
+                          u"This is a longer description which\n"
+                          u"potentially contains multiple lines.\n")
+
+    def assert_parse_with_revision(self, with_path):
+        """
+        Parsing the content file should work. :-)  Basic information will
+        be available as attributes of the info file.
+        """
+        with self.change_sample() as data:
+            data["revision"] = 123
+        log = self.capture_logging("juju.charm")
+        self.metadata.parse(self.sample, "some/path" if with_path else None)
+        if with_path:
+            self.assertIn(
+                "some/path: revision field is obsolete. Move it to the "
+                "'revision' file.",
+                log.getvalue())
+        self.assertEquals(self.metadata.name, "dummy")
+        self.assertEquals(self.metadata.obsolete_revision, 123)
+        self.assertEquals(self.metadata.summary, u"That's a dummy charm.")
+        self.assertEquals(self.metadata.description,
+                          u"This is a longer description which\n"
+                          u"potentially contains multiple lines.\n")
+        self.assertEquals(
+            self.metadata.get_serialization_data()["revision"], 123)
+
+    def test_parse_with_revision(self):
+        self.assert_parse_with_revision(True)
+        self.assert_parse_with_revision(False)
 
     def test_load_calls_parse_calls_parse_serialzation_data(self):
         """
@@ -118,18 +144,6 @@
                                   self.metadata.load, filename)
         self.assertEquals(error.path, filename)
 
-    def test_revision_is_int(self):
-        """
-        Revision numbers are *integers*. Yep, no 1.2.3 versioning.
-        """
-        with self.change_sample() as data:
-            data["revision"] = "1"
-        error = self.assertRaises(MetaDataError,
-                                  self.metadata.parse, self.sample)
-        self.assertEquals(str(error),
-                          "Bad data in charm info: revision: "
-                          "expected int, got '1'")
-
     def test_name_summary_and_description_are_utf8(self):
         """
         Textual fields are decoded to unicode by the schema using UTF-8.

=== modified file 'juju/charm/tests/test_repository.py'
--- juju/charm/tests/test_repository.py	2011-10-05 09:51:16 +0000
+++ juju/charm/tests/test_repository.py	2011-10-05 18:13:26 +0000
@@ -63,16 +63,15 @@
         return CharmURL.parse("local:series/" + name)
 
     @inlineCallbacks
-    def assert_not_there(self, name):
+    def assert_not_there(self, name, repo, revision=None):
         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)
+            name, repo.path)
+        err = yield self.assertFailure(repo.find(url), CharmNotFound)
+        self.assertEquals(str(err), msg)
+        if revision is None:
+            err = yield self.assertFailure(repo.latest(url), CharmNotFound)
+            self.assertEquals(str(err), msg)
 
     def test_find_inappropriate_url(self):
         url = CharmURL.parse("cs:foo/bar")
@@ -80,18 +79,18 @@
         self.assertEquals(str(err), "schema mismatch")
 
     def test_completely_missing(self):
-        return self.assert_not_there("zebra")
+        return self.assert_not_there("zebra", self.repository1)
 
     def test_unkown_files_ignored(self):
         self.makeFile(
             "Foobar",
             path=os.path.join(self.repository1.path, "series", "zebra"))
-        return self.assert_not_there("zebra")
+        return self.assert_not_there("zebra", self.repository1)
 
     def test_unknown_directories_ignored(self):
         self.makeDir(
             path=os.path.join(self.repository1.path, "series", "zebra"))
-        return self.assert_not_there("zebra")
+        return self.assert_not_there("zebra", self.repository1)
 
     def test_broken_charms_ignored(self):
         charm_path = self.makeDir(
@@ -104,37 +103,45 @@
 revision: 0
 summary: hola""")
         fh.close()
-        return self.assert_not_there("zebra")
+        return self.assert_not_there("zebra", self.repository1)
 
     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)
+        self.assertEquals(charm.get_revision(), revision)
         latest = yield repo.latest(url)
         self.assertEquals(latest, latest_revision or revision)
 
+    @inlineCallbacks
     def test_success_unbundled(self):
-        return self.assert_there("sample", self.repository1, 2)
-        return self.assert_there("sample-2", self.repository1, 2)
+        yield self.assert_there("sample", self.repository1, 2)
+        yield self.assert_there("sample-1", self.repository1, 1, 2)
+        yield self.assert_there("sample-2", self.repository1, 2)
+        yield self.assert_not_there("sample-3", self.repository1, 2)
 
+    @inlineCallbacks
     def test_success_bundled(self):
-        return self.assert_there("sample", self.repository2, 2)
-        return self.assert_there("sample-2", self.repository2, 2)
+        yield self.assert_there("sample", self.repository2, 2)
+        yield self.assert_there("sample-1", self.repository2, 1, 2)
+        yield self.assert_there("sample-2", self.repository2, 2)
+        yield self.assert_not_there("sample-3", self.repository2, 2)
 
     @inlineCallbacks
     def test_no_revision_gets_latest(self):
         yield self.assert_there("sample", self.repository1, 2)
+        yield self.assert_there("sample-1", self.repository1, 1, 2)
+        yield self.assert_there("sample-2", self.repository1, 2)
+        yield self.assert_not_there("sample-3", self.repository1, 2)
 
-        file = open(os.path.join(
-            self.repository1.path, "series/old/metadata.yaml"), "rw+")
-        data = yaml.load(file.read())
-        data["revision"] = 3
-        file.seek(0)
-        file.write(yaml.dump(data))
-        file.close()
+        revision_path = os.path.join(
+            self.repository1.path, "series/old/revision")
+        with open(revision_path, "w") as f:
+            f.write("3")
 
         yield self.assert_there("sample", self.repository1, 3)
+        yield self.assert_not_there("sample-1", self.repository1, 3)
         yield self.assert_there("sample-2", self.repository1, 2, 3)
+        yield self.assert_there("sample-3", self.repository1, 3)
 
 
 class RemoteRepositoryTest(RepositoryTestBase):

=== modified file 'juju/control/deploy.py'
--- juju/control/deploy.py	2011-09-30 11:25:41 +0000
+++ juju/control/deploy.py	2011-10-05 18:13:26 +0000
@@ -95,7 +95,7 @@
         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))
+    charm_id = str(charm_url.with_revision(charm.get_revision()))
 
     provider = environment.get_machine_provider()
     placement_policy = provider.get_placement_policy()

=== modified file 'juju/control/tests/test_upgrade_charm.py'
--- juju/control/tests/test_upgrade_charm.py	2011-09-29 03:07:26 +0000
+++ juju/control/tests/test_upgrade_charm.py	2011-10-05 18:13:26 +0000
@@ -15,23 +15,23 @@
 
 class CharmUpgradeTestBase(object):
 
-    def add_charm(self, metadata, repository_dir=None):
+    def add_charm(self, metadata, revision, repository_dir=None):
         if repository_dir is None:
             repository_dir = self.makeDir()
         series_dir = os.path.join(repository_dir, "series")
         os.mkdir(series_dir)
         charm_dir = os.path.join(series_dir, metadata["name"])
         os.mkdir(charm_dir)
-        fh = open(os.path.join(charm_dir, "metadata.yaml"), "w")
-        fh.write(dump(metadata))
-        fh.close()
+        with open(os.path.join(charm_dir, "metadata.yaml"), "w") as f:
+            f.write(dump(metadata))
+        with open(os.path.join(charm_dir, "revision"), "w") as f:
+            f.write(str(revision))
         return LocalCharmRepository(repository_dir)
 
     def increment_charm(self, charm):
         metadata = charm.metadata.get_serialization_data()
         metadata["name"] = "mysql"
-        metadata["revision"] = 2
-        repository = self.add_charm(metadata)
+        repository = self.add_charm(metadata, charm.get_revision() + 1)
         return repository
 
 
@@ -201,7 +201,7 @@
 
         metadata = self.charm.metadata.get_serialization_data()
         metadata["name"] = "mysql"
-        repository = self.add_charm(metadata)
+        repository = self.add_charm(metadata, 1)
         main(["upgrade-charm", "--dry-run",
                   "--repository", repository.path, "mysql"])
         yield finished
@@ -224,7 +224,7 @@
 
         metadata = self.charm.metadata.get_serialization_data()
         metadata["name"] = "mysql"
-        repository = self.add_charm(metadata)
+        repository = self.add_charm(metadata, 1)
         main(["upgrade-charm", "--repository", repository.path, "mysql"])
 
         yield finished

=== modified file 'juju/machine/tests/test_unit_deployment.py'
--- juju/machine/tests/test_unit_deployment.py	2011-10-01 04:32:56 +0000
+++ juju/machine/tests/test_unit_deployment.py	2011-10-05 18:13:26 +0000
@@ -265,7 +265,7 @@
         self.assertTrue(os.path.exists(charm_path))
         charm = get_charm_from_path(charm_path)
         self.assertEqual(
-            charm.metadata.revision, self.charm.metadata.revision)
+            charm.get_revision(), self.charm.get_revision())
 
     def test_unpack_charm_exception_invalid_charm(self):
         """

=== modified file 'juju/state/charm.py'
--- juju/state/charm.py	2011-09-28 23:54:04 +0000
+++ juju/state/charm.py	2011-10-05 18:13:26 +0000
@@ -73,7 +73,6 @@
 
         # Just a health check:
         assert self._metadata.name == self.name
-        assert self._metadata.revision == self.revision
 
         self._sha256 = charm_data["sha256"]
 

=== modified file 'juju/state/tests/test_charm.py'
--- juju/state/tests/test_charm.py	2011-09-28 00:36:30 +0000
+++ juju/state/tests/test_charm.py	2011-10-05 18:13:26 +0000
@@ -79,7 +79,6 @@
             "local:series/dummy-1")
         metadata = yield charm_state.get_metadata()
         self.assertEquals(metadata.name, "dummy")
-        self.assertEquals(metadata.revision, 1)
 
     @inlineCallbacks
     def test_charm_state_config_options(self):

=== modified file 'juju/unit/tests/test_charm.py'
--- juju/unit/tests/test_charm.py	2011-09-28 23:04:39 +0000
+++ juju/unit/tests/test_charm.py	2011-10-05 18:13:26 +0000
@@ -84,7 +84,7 @@
 
         self.assertTrue(os.path.exists(charm_path))
         bundle = CharmBundle(charm_path)
-        self.assertEquals(bundle.metadata.revision, charm.metadata.revision)
+        self.assertEquals(bundle.get_revision(), charm.get_revision())
 
         self.assertEqual(checksum, bundle.get_sha256())