add charm-store-backed charm.Repo

add charm-store-backed charm.Repo

note that a fair chunk of the diff involves moving InfoResponse from store
into charm (to defeat import loops) and Quote from state into charm (because
it appears to be entirely charm-specific at the moment; I was tempted to tack
it directly onto charm.URL, but didn't, because I recall it was originally
intended to have an independent existence...)

https://code.launchpad.net/~fwereade/juju/go-charmstore-repo/+merge/107390

(do not edit description out of merge proposal)

[fwereade, 2012-05-25 14:23:53]

Please take a look.

[niemeyer, 2012-05-25 20:31:53]

Looking nice.

https://codereview.appspot.com/6257056/diff/1/charm/repo.go
File charm/repo.go (right):

https://codereview.appspot.com/6257056/diff/1/charm/repo.go#newcode19
charm/repo.go:19: type InfoResponse struct {
Looks like this might be private. Or maybe we should just expose
store.InfoResponse so we can share?

https://codereview.appspot.com/6257056/diff/1/charm/repo.go#newcode26
charm/repo.go:26: // Repo respresents a collection of charms.
Repository?  Not sure if the abbreviation is common enough, or the use is
frequent enough, to justify it.

https://codereview.appspot.com/6257056/diff/1/charm/repo.go#newcode39
charm/repo.go:39: STORE_URL  = "https://store.juju.ubuntu.com"
It's not following the usual naming convention. I also wonder if this should be
public? storeURL and cachePath?

https://codereview.appspot.com/6257056/diff/1/charm/repo.go#newcode66
charm/repo.go:66: err = fmt.Errorf("missing info for charm: %q", key)
"charm: charm store returned response without charm %q"

https://codereview.appspot.com/6257056/diff/1/charm/repo.go#newcode70
charm/repo.go:70: log.Printf("WARNING: info for %q: %s", key, w)
s/info for/charm store reports for %q/?

https://codereview.appspot.com/6257056/diff/1/charm/repo.go#newcode115
charm/repo.go:115: return nil, fmt.Errorf("bad revision info for %q",
curl.String())
"charm: store returned charm with wrong revision for %q"

https://codereview.appspot.com/6257056/diff/1/charm/repo.go#newcode130
charm/repo.go:130: f.Close()
if cerr := f.Close(); err == nil {
    err = cerr
}

https://codereview.appspot.com/6257056/diff/1/charm/repo.go#newcode135
charm/repo.go:135: if err := os.Rename(dlPath, path); err != nil {
For this to work reliably, you'll have to tweak the TempFile above so it lands
in the same directory. Renames can't be done across filesystems.

https://codereview.appspot.com/6257056/diff/1/charm/repo_test.go
File charm/repo_test.go (right):

https://codereview.appspot.com/6257056/diff/1/charm/repo_test.go#newcode36
charm/repo_test.go:36: func (s *StoreSuite) SetUpSuite(c *C) {
Looks a bit spaghetti-like here. Things below may be properly organized in a
type, that has proper functions for hooking in the mux, etc.

The tests look nice, though.

https://codereview.appspot.com/6257056/diff/1/charm/repo_test.go#newcode95
charm/repo_test.go:95: log.Target = stdlog.New(s.log, "", stdlog.LstdFlags)
log.Target = c?  That said, The juju/go/testing package has a helper for that.
You can grab the log out of c too.

[fwereade, 2012-05-28 11:33:43]

Please take a look.

https://codereview.appspot.com/6257056/diff/1/charm/repo.go
File charm/repo.go (right):

https://codereview.appspot.com/6257056/diff/1/charm/repo.go#newcode19
charm/repo.go:19: type InfoResponse struct {
On 2012/05/25 20:31:53, niemeyer wrote:
> Looks like this might be private. Or maybe we should just expose
> store.InfoResponse so we can share?

I moved this here from store (it used to be store.charmResponse); would prefer
to have kept it there, but it seemed more important to (1) use the same type
rather than repeating myself and (2) put it in an existing package where it
could be used without import cycles, rather than creating a new package (or
rearranging huge amounts of stuff).

https://codereview.appspot.com/6257056/diff/1/charm/repo.go#newcode26
charm/repo.go:26: // Repo respresents a collection of charms.
On 2012/05/25 20:31:53, niemeyer wrote:
> Repository?  Not sure if the abbreviation is common enough, or the use is
> frequent enough, to justify it.

Fair enough. Done.

https://codereview.appspot.com/6257056/diff/1/charm/repo.go#newcode39
charm/repo.go:39: STORE_URL  = "https://store.juju.ubuntu.com"
On 2012/05/25 20:31:53, niemeyer wrote:
> It's not following the usual naming convention. I also wonder if this should
be
> public? storeURL and cachePath?

Done.

https://codereview.appspot.com/6257056/diff/1/charm/repo.go#newcode66
charm/repo.go:66: err = fmt.Errorf("missing info for charm: %q", key)
On 2012/05/25 20:31:53, niemeyer wrote:
> "charm: charm store returned response without charm %q"

Done.

https://codereview.appspot.com/6257056/diff/1/charm/repo.go#newcode70
charm/repo.go:70: log.Printf("WARNING: info for %q: %s", key, w)
On 2012/05/25 20:31:53, niemeyer wrote:
> s/info for/charm store reports for %q/?

Done.

https://codereview.appspot.com/6257056/diff/1/charm/repo.go#newcode115
charm/repo.go:115: return nil, fmt.Errorf("bad revision info for %q",
curl.String())
On 2012/05/25 20:31:53, niemeyer wrote:
> "charm: store returned charm with wrong revision for %q"

Done.

https://codereview.appspot.com/6257056/diff/1/charm/repo.go#newcode130
charm/repo.go:130: f.Close()
On 2012/05/25 20:31:53, niemeyer wrote:
> if cerr := f.Close(); err == nil {
>     err = cerr
> }

Done.

https://codereview.appspot.com/6257056/diff/1/charm/repo.go#newcode135
charm/repo.go:135: if err := os.Rename(dlPath, path); err != nil {
On 2012/05/25 20:31:53, niemeyer wrote:
> For this to work reliably, you'll have to tweak the TempFile above so it lands
> in the same directory. Renames can't be done across filesystems.

Good catch, ty; done.

https://codereview.appspot.com/6257056/diff/1/charm/repo_test.go
File charm/repo_test.go (right):

https://codereview.appspot.com/6257056/diff/1/charm/repo_test.go#newcode36
charm/repo_test.go:36: func (s *StoreSuite) SetUpSuite(c *C) {
On 2012/05/25 20:31:53, niemeyer wrote:
> Looks a bit spaghetti-like here. Things below may be properly organized in a
> type, that has proper functions for hooking in the mux, etc.
> 
> The tests look nice, though.

Done.

[niemeyer, 2012-05-29 09:57:04]

LGTM, thanks!

[fwereade, 2012-05-29 10:10:36]

*** Submitted:

add charm-store-backed charm.Repo

note that a fair chunk of the diff involves moving InfoResponse from store
into charm (to defeat import loops) and Quote from state into charm (because
it appears to be entirely charm-specific at the moment; I was tempted to tack
it directly onto charm.URL, but didn't, because I recall it was originally
intended to have an independent existence...)

R=niemeyer
CC=
https://codereview.appspot.com/6257056

https://codereview.appspot.com/6257056/diff/1/charm/repo_test.go
File charm/repo_test.go (right):

https://codereview.appspot.com/6257056/diff/1/charm/repo_test.go#newcode95
charm/repo_test.go:95: log.Target = stdlog.New(s.log, "", stdlog.LstdFlags)
On 2012/05/25 20:31:53, niemeyer wrote:
> log.Target = c?  That said, The juju/go/testing package has a helper for that.
> You can grab the log out of c too.

Done.