Code review comment for lp:~fwereade/pyjuju/isolate-formula-revisions

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)