Add local charm Repository type

Add local charm Repository type



https://code.launchpad.net/~fwereade/juju/go-local-repo/+merge/108123

(do not edit description out of merge proposal)

[fwereade, 2012-05-31 08:44:27]

Please take a look.

[niemeyer, 2012-05-31 10:19:24]

Very nice.

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

https://codereview.appspot.com/6245075/diff/1/charm/charm.go#newcode13
charm/charm.go:13: // Read reads a Charm from path, which can point to either a
charm bundle or a
Handy.

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

https://codereview.appspot.com/6245075/diff/1/charm/repo.go#newcode148
charm/repo.go:148: // by series.
It be nice to have more details here about what this actually means, so that
someone reading this documentation knows what to expect in a sensible local
repository directory.

https://codereview.appspot.com/6245075/diff/1/charm/repo.go#newcode167
charm/repo.go:167: noCharms := fmt.Errorf("no charms found matching %q",
curl.String())
I'd prefer to have that in a function outside:

func charmNotFound(curl *URL) error

Also, note that the .String() isn't needed there.

https://codereview.appspot.com/6245075/diff/1/charm/repo.go#newcode169
charm/repo.go:169: return nil, fmt.Errorf("bad schema: %q", curl.Schema)
The schema isn't bad.

("charm: local repository got URL with non-local schema: %q", curl)

https://codereview.appspot.com/6245075/diff/1/charm/repo.go#newcode172
charm/repo.go:172: infos, err := ioutil.ReadDir(path)
There should be a distinction between r.Path not existing and r.Path+series not
existing. The latter may be a charm not found error, but the former should be
reported as a non-existent repository directory error.

https://codereview.appspot.com/6245075/diff/1/charm/repo.go#newcode178
charm/repo.go:178: chPath := filepath.Join(path, info.Name())
We should only pick files ending in ".charm" and directories not starting with
".". I believe there's even a bug open about this in Python, so we may as well
take the chance to fix it right now while it's so cheap.

[fwereade, 2012-05-31 11:39:18]

Please take a look.

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

https://codereview.appspot.com/6245075/diff/1/charm/repo.go#newcode148
charm/repo.go:148: // by series.
On 2012/05/31 10:19:24, niemeyer wrote:
> It be nice to have more details here about what this actually means, so that
> someone reading this documentation knows what to expect in a sensible local
> repository directory.

Done.

https://codereview.appspot.com/6245075/diff/1/charm/repo.go#newcode167
charm/repo.go:167: noCharms := fmt.Errorf("no charms found matching %q",
curl.String())
On 2012/05/31 10:19:24, niemeyer wrote:
> I'd prefer to have that in a function outside:
> 
> func charmNotFound(curl *URL) error
> 
> Also, note that the .String() isn't needed there.

Done.

https://codereview.appspot.com/6245075/diff/1/charm/repo.go#newcode169
charm/repo.go:169: return nil, fmt.Errorf("bad schema: %q", curl.Schema)
On 2012/05/31 10:19:24, niemeyer wrote:
> The schema isn't bad.
> 
> ("charm: local repository got URL with non-local schema: %q", curl)

Done.

https://codereview.appspot.com/6245075/diff/1/charm/repo.go#newcode172
charm/repo.go:172: infos, err := ioutil.ReadDir(path)
On 2012/05/31 10:19:24, niemeyer wrote:
> There should be a distinction between r.Path not existing and r.Path+series
not
> existing. The latter may be a charm not found error, but the former should be
> reported as a non-existent repository directory error.

Done.

https://codereview.appspot.com/6245075/diff/1/charm/repo.go#newcode178
charm/repo.go:178: chPath := filepath.Join(path, info.Name())
On 2012/05/31 10:19:24, niemeyer wrote:
> We should only pick files ending in ".charm" and directories not starting with
> ".". I believe there's even a bug open about this in Python, so we may as well
> take the chance to fix it right now while it's so cheap.

Done.

[niemeyer, 2012-05-31 17:15:33]

LGTM

https://codereview.appspot.com/6245075/diff/5001/charm/repo.go
File charm/repo.go (right):

https://codereview.appspot.com/6245075/diff/5001/charm/repo.go#newcode149
charm/repo.go:149: // that series.
//
// For example:
//
//     /path/to/repository/oneiric/mongodb/
//     /path/to/repository/precise/mongodb.charm
//     /path/to/repository/precise/wordpress/
//

https://codereview.appspot.com/6245075/diff/5001/charm/repo.go#newcode184
charm/repo.go:184: return nil, fmt.Errorf("charm: local repository got URL with
non-local schema: %q", curl)
Would you mind to drop the "charm:" prefix here? I know it was my suggestion,
but this is inconsistent with the other error messages above. We need some more
clear definition of how to do that kind of prefixing. We're all over the place
right now.

We just had a good brainstorm here, and we'll probably remove all those blind
package prefixes by default. We may still keep some, but it's a mess right now
and we should start cleaning it up towards something more sensible.

https://codereview.appspot.com/6245075/diff/5001/charm/repo_test.go
File charm/repo_test.go (right):

https://codereview.appspot.com/6245075/diff/5001/charm/repo_test.go#newcode325
charm/repo_test.go:325: func (s *LocalRepoSuite) TestIgnoresUnpromisingNames(c
*C) {
Awesome name. :-)

[fwereade, 2012-06-01 07:43:50]

*** Submitted:

Add local charm Repository type

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

https://codereview.appspot.com/6245075/diff/5001/charm/repo.go
File charm/repo.go (right):

https://codereview.appspot.com/6245075/diff/5001/charm/repo.go#newcode149
charm/repo.go:149: // that series.
On 2012/05/31 17:15:33, niemeyer wrote:
> //
> // For example:
> //
> //     /path/to/repository/oneiric/mongodb/
> //     /path/to/repository/precise/mongodb.charm
> //     /path/to/repository/precise/wordpress/
> //

Done.

https://codereview.appspot.com/6245075/diff/5001/charm/repo.go#newcode184
charm/repo.go:184: return nil, fmt.Errorf("charm: local repository got URL with
non-local schema: %q", curl)
On 2012/05/31 17:15:33, niemeyer wrote:
> Would you mind to drop the "charm:" prefix here? I know it was my suggestion,
> but this is inconsistent with the other error messages above. We need some
more
> clear definition of how to do that kind of prefixing. We're all over the place
> right now.
> 
> We just had a good brainstorm here, and we'll probably remove all those blind
> package prefixes by default. We may still keep some, but it's a mess right now
> and we should start cleaning it up towards something more sensible.

Done.

https://codereview.appspot.com/6245075/diff/5001/charm/repo_test.go
File charm/repo_test.go (right):

https://codereview.appspot.com/6245075/diff/5001/charm/repo_test.go#newcode325
charm/repo_test.go:325: func (s *LocalRepoSuite) TestIgnoresUnpromisingNames(c
*C) {
On 2012/05/31 17:15:33, niemeyer wrote:
> Awesome name. :-)

Cheers :)