Merge lp:~fwereade/pyjuju/go-charm-resolve into lp:pyjuju/go

[William Reade]

add charm.InferRepository

https://codereview.appspot.com/6261058/

[William Reade (fwereade)]

Reviewers: mp+108337_code.launchpad.net,

Message:
Please take a look.

Description:
add charm.Resolve

https://code.launchpad.net/~fwereade/juju/go-charm-resolve/+merge/108337

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/6261058/

Affected files:
   A [revision details]
   M charm/charm.go
   M charm/charm_test.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: charm/charm.go
=== modified file 'charm/charm.go'
--- charm/charm.go 2012-05-29 09:31:12 +0000
+++ charm/charm.go 2012-06-01 12:58:01 +0000
@@ -1,6 +1,9 @@
  package charm

-import "os"
+import (
+ "fmt"
+ "os"
+)

  // The Charm interface is implemented by any type that
  // may be handled as a charm.
@@ -22,3 +25,22 @@
   }
   return ReadBundle(path)
  }
+
+// Resolve takes a (potentially vaguely-specified) charm name, the path to
the
+// local charm repository, and the environment's default Ubuntu series, and
+// assembles them into a charm URL and a repository which is likely to
contain
+// a charm matching that URL.
+func Resolve(name, repoPath, defaultSeries string) (repo Repository, curl
*URL, err error) {
+ if curl, err = InferURL(name, defaultSeries); err != nil {
+ return
+ }
+ switch curl.Schema {
+ case "cs":
+ repo = Store()
+ case "local":
+ repo = &LocalRepository{repoPath}
+ default:
+ panic(fmt.Errorf("unknown schema for charm URL %q", curl))
+ }
+ return
+}

Index: charm/charm_test.go
=== modified file 'charm/charm_test.go'
--- charm/charm_test.go 2012-05-29 09:31:12 +0000
+++ charm/charm_test.go 2012-06-01 12:58:01 +0000
@@ -30,6 +30,34 @@
   c.Assert(ch.Meta().Name, Equals, "dummy")
  }

+var resolveTests = []struct {
+ name string
+ path string
+ series string
+ curl string
+}{
+ {"wordpress", "anything", "precise", "cs:precise/wordpress"},
+ {"oneiric/wordpress", "anything", "anything", "cs:oneiric/wordpress"},
+ {"cs:oneiric/wordpress", "anything", "anything", "cs:oneiric/wordpress"},
+ {"local:wordpress", "/some/path", "precise", "local:precise/wordpress"},
+
{"local:oneiric/wordpress", "/some/path", "anything", "local:oneiric/wordpress"},
+}
+
+func (s *CharmSuite) TestResolve(c *C) {
+ for _, t := range resolveTests {
+ repo, curl, err := charm.Resolve(t.name, t.path, t.series)
+ c.Assert(err, IsNil)
+ expectCurl := charm.MustParseURL(t.curl)
+ c.Assert(curl, DeepEquals, expectCurl)
+ if localRepo, ok := repo.(*charm.LocalRepository); ok {
+ c.Assert(localRepo.Path, Equals, t.path)
+ c.Assert(curl.Schema, Equals, "local")
+ } else {
+ c.Assert(curl.Schema, Equals, "cs")
+ }
+ }
+}
+
  func checkDummy(c *C, f charm.Charm, path string) {
   c.Assert(f.Revision(), Equals, 1)
   c.Assert(f.Meta().Name, Equals, "dummy")

[Gustavo Niemeyer (niemeyer)]

LGTM

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

https://codereview.appspot.com/6261058/diff/1/charm/charm.go#newcode32
charm/charm.go:32: // a charm matching that URL.
// InferRepository returns a charm repository and URL inferred from the
provided
// parameters. charmAlias may hold an exact charm URL, or an alias in a
// format supported by InferURL.

https://codereview.appspot.com/6261058/diff/1/charm/charm.go#newcode33
charm/charm.go:33: func Resolve(name, repoPath, defaultSeries string)
(repo Repository, curl *URL, err error) {
This is InferURL + repository. I suggest this signature:

func InferRepository(charmAlias, defaultSeries, localRepoPath string)
(repo Repository, curl *URL, err error)

(note the parameter ordering matching InferURL)

https://codereview.appspot.com/6261058/diff/1/charm/charm.go#newcode41
charm/charm.go:41: repo = &LocalRepository{repoPath}
Should we support the case where localRepoPath is ""? Not sure about how
the call site for this will look like.

https://codereview.appspot.com/6261058/

[lp:~fwereade/pyjuju/go-charm-resolve]

199. By William Reade on 2012-06-01

address review points

[William Reade (fwereade)]

*** Submitted:

add charm.InferRepository

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

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

https://codereview.appspot.com/6261058/diff/1/charm/charm.go#newcode32
charm/charm.go:32: // a charm matching that URL.
On 2012/06/01 13:32:34, niemeyer wrote:
> // InferRepository returns a charm repository and URL inferred from
the provided
> // parameters. charmAlias may hold an exact charm URL, or an alias in
a
> // format supported by InferURL.

Done.

https://codereview.appspot.com/6261058/diff/1/charm/charm.go#newcode33
charm/charm.go:33: func Resolve(name, repoPath, defaultSeries string)
(repo Repository, curl *URL, err error) {
On 2012/06/01 13:32:34, niemeyer wrote:
> This is InferURL + repository. I suggest this signature:

> func InferRepository(charmAlias, defaultSeries, localRepoPath string)
(repo
> Repository, curl *URL, err error)

> (note the parameter ordering matching InferURL)

Done.

https://codereview.appspot.com/6261058/diff/1/charm/charm.go#newcode41
charm/charm.go:41: repo = &LocalRepository{repoPath}
On 2012/06/01 13:32:34, niemeyer wrote:
> Should we support the case where localRepoPath is ""? Not sure about
how the
> call site for this will look like.

I think an explicit error here is a good idea. Done.

https://codereview.appspot.com/6261058/