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

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 ;-).

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.

[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.

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 :).

[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?

> [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.

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.

[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.