Implementation of the charm state.

This branch implements the charm and removes the
so far used charm mock.

https://code.launchpad.net/~themue/juju/go-state-implemented-charm/+merge/93249

(do not edit description out of merge proposal)

[TheMue, 2012-02-15 17:09:12]

Please take a look.

[rog, 2012-02-16 14:29:47]

looks good.

BTW the state tests are getting really slow (>30s before this merge on my
machine).
i wonder if there's some way we can speed them up. i don't think gocheck
supports concurrent tests, but perhaps there's another way?

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

https://codereview.appspot.com/5671055/diff/1/state/charm.go#newcode35
state/charm.go:35: panic(err)
given that this function returns an error, this should definitely not be a panic

https://codereview.appspot.com/5671055/diff/1/state/charm.go#newcode49
state/charm.go:49: panic("illegal charm name")
i'd return an error.

https://codereview.appspot.com/5671055/diff/1/state/internal_test.go
File state/internal_test.go (right):

https://codereview.appspot.com/5671055/diff/1/state/internal_test.go#newcode844
state/internal_test.go:844: // Check that invalid chars a translated correctly.
s/a translated/are translated/

https://codereview.appspot.com/5671055/diff/1/state/state.go
File state/state.go (right):

https://codereview.appspot.com/5671055/diff/1/state/state.go#newcode64
state/state.go:64: } else if err != nil {
no else needed here

https://codereview.appspot.com/5671055/diff/1/state/state.go#newcode68
state/state.go:68: if err = goyaml.Unmarshal([]byte(yaml), data); err != nil {
since you can, i'd probably use err := ...
here and avoid the unnecessary reference to the
previous err variable.

https://codereview.appspot.com/5671055/diff/1/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/5671055/diff/1/state/state_test.go#newcode16
state/state_test.go:16: // charmRepoDir returns a directory containing test
charms.
this seems to return the directory of a particular test charm, rather than a
directory containing test charms.
maybe s/charmRepoDir/charmDir/ ?

https://codereview.appspot.com/5671055/diff/1/state/state_test.go#newcode118
state/state_test.go:118: dummy, err := s.st.AddCharm(localCharmId(dummyCharm),
dummyCharm, "http://example.com/abc")
s/dummy/_/
because it doesn't get used.

https://codereview.appspot.com/5671055/diff/1/state/state_test.go#newcode130
state/state_test.go:130: dummy, err := s.st.AddCharm(localCharmId(dummyCharm),
dummyCharm, "http://example.com/abc")
s/dummy/_/

https://codereview.appspot.com/5671055/diff/1/state/util.go
File state/util.go (right):

https://codereview.appspot.com/5671055/diff/1/state/util.go#newcode262
state/util.go:262: for _, b := range []byte(unsafe) {
the allocation here is unnecessary:
       for i := 0; i < len(unsafe); i++ {
           b := unsafe[i]
       etc

https://codereview.appspot.com/5671055/diff/1/state/util.go#newcode263
state/util.go:263: if (b >= 'a' && b <= 'z') || (b >= 'A' && b <= 'Z') || (b >=
'0' && b <= '9') || b == '.' || b == '-' {
the brackets aren't necessary here - && has higher priority than ||.

alternative phrasing, arguably easier to read:
    switch {
    case b >= 'a' && b <= 'z',
         b >= 'A' && b <= 'Z',
         etc:
    }
}

https://codereview.appspot.com/5671055/diff/1/state/util.go#newcode266
state/util.go:266: safe = append(safe, []byte(fmt.Sprintf("_%02x_", b))...)
you don't need the []byte conversion here.

[TheMue, 2012-02-17 14:24:19]

Please take a look.

[TheMue, 2012-02-17 14:37:21]

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

https://codereview.appspot.com/5671055/diff/1/state/charm.go#newcode35
state/charm.go:35: panic(err)
On 2012/02/16 14:29:47, rog wrote:
> given that this function returns an error, this should definitely not be a
panic

Done.

https://codereview.appspot.com/5671055/diff/1/state/charm.go#newcode49
state/charm.go:49: panic("illegal charm name")
On 2012/02/16 14:29:47, rog wrote:
> i'd return an error.

Done.

https://codereview.appspot.com/5671055/diff/1/state/internal_test.go
File state/internal_test.go (right):

https://codereview.appspot.com/5671055/diff/1/state/internal_test.go#newcode844
state/internal_test.go:844: // Check that invalid chars a translated correctly.
On 2012/02/16 14:29:47, rog wrote:
> s/a translated/are translated/

Done.

https://codereview.appspot.com/5671055/diff/1/state/state.go
File state/state.go (right):

https://codereview.appspot.com/5671055/diff/1/state/state.go#newcode64
state/state.go:64: } else if err != nil {
On 2012/02/16 14:29:47, rog wrote:
> no else needed here

Done.

https://codereview.appspot.com/5671055/diff/1/state/state.go#newcode68
state/state.go:68: if err = goyaml.Unmarshal([]byte(yaml), data); err != nil {
On 2012/02/16 14:29:47, rog wrote:
> since you can, i'd probably use err := ...
> here and avoid the unnecessary reference to the
> previous err variable.

Done.

https://codereview.appspot.com/5671055/diff/1/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/5671055/diff/1/state/state_test.go#newcode16
state/state_test.go:16: // charmRepoDir returns a directory containing test
charms.
On 2012/02/16 14:29:47, rog wrote:
> this seems to return the directory of a particular test charm, rather than a
> directory containing test charms.
> maybe s/charmRepoDir/charmDir/ ?

Done.

https://codereview.appspot.com/5671055/diff/1/state/state_test.go#newcode118
state/state_test.go:118: dummy, err := s.st.AddCharm(localCharmId(dummyCharm),
dummyCharm, "http://example.com/abc")
On 2012/02/16 14:29:47, rog wrote:
> s/dummy/_/
> because it doesn't get used.

It's not used here, but below, also in the next tests. Initializing/declaring
dummy with the first AddCharm/Charm creates a homogeneous structure. This helps
to maintain the tests.

https://codereview.appspot.com/5671055/diff/1/state/state_test.go#newcode130
state/state_test.go:130: dummy, err := s.st.AddCharm(localCharmId(dummyCharm),
dummyCharm, "http://example.com/abc")
On 2012/02/16 14:29:47, rog wrote:
> s/dummy/_/

Done.

https://codereview.appspot.com/5671055/diff/1/state/util.go
File state/util.go (right):

https://codereview.appspot.com/5671055/diff/1/state/util.go#newcode262
state/util.go:262: for _, b := range []byte(unsafe) {
On 2012/02/16 14:29:47, rog wrote:
> the allocation here is unnecessary:
>        for i := 0; i < len(unsafe); i++ {
>            b := unsafe[i]
>        etc

Done.

https://codereview.appspot.com/5671055/diff/1/state/util.go#newcode263
state/util.go:263: if (b >= 'a' && b <= 'z') || (b >= 'A' && b <= 'Z') || (b >=
'0' && b <= '9') || b == '.' || b == '-' {
On 2012/02/16 14:29:47, rog wrote:
> the brackets aren't necessary here - && has higher priority than ||.
> 
> alternative phrasing, arguably easier to read:
>     switch {
>     case b >= 'a' && b <= 'z',
>          b >= 'A' && b <= 'Z',
>          etc:
>     }
> }

Done.

https://codereview.appspot.com/5671055/diff/1/state/util.go#newcode266
state/util.go:266: safe = append(safe, []byte(fmt.Sprintf("_%02x_", b))...)
On 2012/02/16 14:29:47, rog wrote:
> you don't need the []byte conversion here.

Done.

[rog, 2012-02-17 15:14:02]

LGTM modulo the minor comment fix and "pretty please" dummy->_ changes.

https://codereview.appspot.com/5671055/diff/3003/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/5671055/diff/3003/state/state_test.go#newcode24
state/state_test.go:24: // charmDir returns a directory containing test charms.
s/test charms/the given test charm/

https://codereview.appspot.com/5671055/diff/3003/state/state_test.go#newcode125
state/state_test.go:125: dummy, err := s.st.AddCharm(localCharmId(dummyCharm),
dummyCharm, "")
s/dummy/_/

https://codereview.appspot.com/5671055/diff/3003/state/state_test.go#newcode137
state/state_test.go:137: dummy, err := s.st.AddCharm(localCharmId(dummyCharm),
dummyCharm, "")
s/dummy/_/

[TheMue, 2012-02-20 10:07:24]

Please take a look.

[TheMue, 2012-02-23 08:27:21]

Please take a look.

[TheMue, 2012-02-23 08:27:46]

https://codereview.appspot.com/5671055/diff/3003/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/5671055/diff/3003/state/state_test.go#newcode24
state/state_test.go:24: // charmDir returns a directory containing test charms.
On 2012/02/17 15:14:02, rog wrote:
> s/test charms/the given test charm/

Done.

https://codereview.appspot.com/5671055/diff/3003/state/state_test.go#newcode125
state/state_test.go:125: dummy, err := s.st.AddCharm(localCharmId(dummyCharm),
dummyCharm, "")
On 2012/02/17 15:14:02, rog wrote:
> s/dummy/_/

Done.

https://codereview.appspot.com/5671055/diff/3003/state/state_test.go#newcode137
state/state_test.go:137: dummy, err := s.st.AddCharm(localCharmId(dummyCharm),
dummyCharm, "")
On 2012/02/17 15:14:02, rog wrote:
> s/dummy/_/

Done.

[fwereade, 2012-02-23 14:51:23]

LGTM

https://codereview.appspot.com/5671055/diff/9001/state/export_test.go
File state/export_test.go (left):

https://codereview.appspot.com/5671055/diff/9001/state/export_test.go#oldcode1
state/export_test.go:1: package state
I still think it would be a good idea to have these live somewhere outside state
(we suggested juju/go/testing IIRC).

https://codereview.appspot.com/5671055/diff/9001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/5671055/diff/9001/state/state_test.go#newcode26
state/state_test.go:26: return filepath.Join("..", "charm", "testrepo",
"series", name)
I already commented on this in the other branch, I think, but i can't tell
*which* branch I should *actually* be commenting in because I don't see either
branch marked as a prereq of the other...

[niemeyer, 2012-02-23 14:57:46]

That's going in a good direction. Some comments:

https://codereview.appspot.com/5671055/diff/9001/state/charm.go
File state/charm.go (right):

https://codereview.appspot.com/5671055/diff/9001/state/charm.go#newcode28
state/charm.go:28: }
Please add this here:

    var _ charm.Charm = (*Charm)(nil)

and fix what's needed to make that compile correctly.

https://codereview.appspot.com/5671055/diff/9001/state/charm.go#newcode31
state/charm.go:31: func readCharm(zk *zookeeper.Conn, charmURL *charm.URL)
(*Charm, error) {
Let's please standardize on curl for the variable name of a charm URL.

https://codereview.appspot.com/5671055/diff/9001/state/charm.go#newcode34
state/charm.go:34: return nil, fmt.Errorf("charm %q not found", charmURL)
"charm not found: %q"

https://codereview.appspot.com/5671055/diff/9001/state/charm.go#newcode46
state/charm.go:46: // newCharm creates a new charm.
Please either drop the comment or make it more relevant.

https://codereview.appspot.com/5671055/diff/9001/state/charm.go#newcode47
state/charm.go:47: func newCharm(zk *zookeeper.Conn, charmURL *charm.URL, data
*charmData) (*Charm, error) {
s/charmURL/curl/

https://codereview.appspot.com/5671055/diff/9001/state/charm.go#newcode63
state/charm.go:63: func (c *Charm) Name() string {
Why do we need this? That's c.Meta().Name

https://codereview.appspot.com/5671055/diff/9001/state/charm.go#newcode81
state/charm.go:81: clone := *c.meta
This data is inherently read-only, in the sense that there's no reason for a
*charm.Meta to be modified. It's fine to not clone it, I believe.

https://codereview.appspot.com/5671055/diff/9001/state/charm.go#newcode87
state/charm.go:87: clone := *c.config
Ditto.

https://codereview.appspot.com/5671055/diff/9001/state/charm.go#newcode98
state/charm.go:98: // based on the URL.
/URL/charm URL/

https://codereview.appspot.com/5671055/diff/9001/state/charm.go#newcode99
state/charm.go:99: func charmPath(charmURL *charm.URL) string {
s/charmURL/curl/

https://codereview.appspot.com/5671055/diff/9001/state/charm.go#newcode100
state/charm.go:100: return fmt.Sprintf("/charms/%s", Quote(charmURL.String()))
Before this line, please check that curl has a Revision, and return an error if
it's < 0. Charms stored in zk must always have a revision set.

https://codereview.appspot.com/5671055/diff/9001/state/export_test.go
File state/export_test.go (right):

https://codereview.appspot.com/5671055/diff/9001/state/export_test.go#newcode7
state/export_test.go:7: // managed by juju.
Please remove the package comment. This is a test file.

https://codereview.appspot.com/5671055/diff/9001/state/export_test.go#newcode41
state/export_test.go:41: // ZkTearDownEnvironment destroys the ZooKeeper Test
environment.
s/Test/test/

https://codereview.appspot.com/5671055/diff/9001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/5671055/diff/9001/state/service.go#newcode65
state/service.go:65: return readCharm(s.zk, url)
Oops.. we're making a similar mistake to the one we've made in the Python
implementation. We must have access to the State from here, so that we can do:

    return s.st.Charm(url)

Rather than having lots of external helper functions. That's the whole point of
centralizing the State into a single type, rather than having all the Manager
classes.

https://codereview.appspot.com/5671055/diff/9001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/5671055/diff/9001/state/state.go#newcode40
state/state.go:40: // AddCharm registers metadata about the provided Charm.
Please improve this comment.

https://codereview.appspot.com/5671055/diff/9001/state/state.go#newcode41
state/state.go:41: func (s *State) AddCharm(charmURL *charm.URL, ch charm.Charm,
url string) (*Charm, error) {
(ch charm.Charm, curl *charm.URL, bundleUrl string), in this order please.

https://codereview.appspot.com/5671055/diff/9001/state/state.go#newcode59
state/state.go:59: func (s *State) Charm(charmURL *charm.URL) (*Charm, error) {
s/charmURL/curl/

https://codereview.appspot.com/5671055/diff/9001/state/state.go#newcode60
state/state.go:60: return readCharm(s.zk, charmURL)
We shouldn't need this helper function, unless there's somewhere we really have
no access to a *State that must read a charm directly from zk, which I don't
think is the case.

The content of readCharm should be inlined here.

https://codereview.appspot.com/5671055/diff/9001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/5671055/diff/9001/state/state_test.go#newcode29
state/state_test.go:29: // readCharm returns a test charm by a name.
s/a name/its name/

https://codereview.appspot.com/5671055/diff/9001/state/state_test.go#newcode39
state/state_test.go:39: charmURL, err := charm.ParseURL(url)
return charm.MustParseURL(url)

https://codereview.appspot.com/5671055/diff/9001/state/state_test.go#newcode47
state/state_test.go:47: // where it's needed.
// addDummyCharm adds the 'dummy' charm state to st.

https://codereview.appspot.com/5671055/diff/9001/state/state_test.go#newcode48
state/state_test.go:48: func addDummyCharm(c *C, st *state.State) *state.Charm {
Return the charm URL too, so that we can use it in tests below rather than
reparsing it all the time.

I actually suggest something like this instead:

func (s *StateSuite) addDummyCharm() (*state.Charm, *charm.URL)

and panic inside it if something goes wrong. This is not what we're testing. It
should always work in this context.

https://codereview.appspot.com/5671055/diff/9001/state/state_test.go#newcode50
state/state_test.go:50: dummy, err := st.AddCharm(localCharmURL(ch), ch,
"http://example.com/abc")
Please use "http://bundle.url" as the URL at the end, so we know what it is when
reading this.

https://codereview.appspot.com/5671055/diff/9001/state/state_test.go#newcode92
state/state_test.go:92: dummy, err := s.st.AddCharm(localCharmURL(dummyCharm),
dummyCharm, "http://example.com/abc")
Isn't this copy & pasting addDummyCharm in all those cases too?

dummy, curl := ...

https://codereview.appspot.com/5671055/diff/9001/state/state_test.go#newcode94
state/state_test.go:94: c.Assert(dummy.URL().String(), Equals,
"local:series/dummy-1")
Please use curl.String() as the expected value. Same for all below.

https://codereview.appspot.com/5671055/diff/9001/state/util.go
File state/util.go (right):

https://codereview.appspot.com/5671055/diff/9001/state/util.go#newcode261
state/util.go:261: safe := []byte{}
safe := make([]byte, 0, len(unsafe)*4)

This will prevent reallocating safe.

https://codereview.appspot.com/5671055/diff/9001/state/util.go#newcode262
state/util.go:262: for i := 0; i < len(unsafe); i++ {
No need to check len on every iteration:

for i, n := 0, len(unsafe); i < n; i++ {

[niemeyer, 2012-02-23 15:00:52]

https://codereview.appspot.com/5671055/diff/9001/state/charm.go
File state/charm.go (right):

https://codereview.appspot.com/5671055/diff/9001/state/charm.go#newcode18
state/charm.go:18: URL    string
Sorry, I missed this one. This should be:

    BundleURL string `yaml:"url"`

The name in yaml was a mistake we can't fix, but we can have a reasonable name
to ourselves.

[rog, 2012-02-23 15:24:15]

https://codereview.appspot.com/5671055/diff/9001/state/util.go
File state/util.go (right):

https://codereview.appspot.com/5671055/diff/9001/state/util.go#newcode261
state/util.go:261: safe := []byte{}
On 2012/02/23 14:57:46, niemeyer wrote:
> safe := make([]byte, 0, len(unsafe)*4)
> 
> This will prevent reallocating safe.

better i think to use len(unsafe). then we optimise for the common case where no
characters need quoting - most of the time we can avoid allocating four times as
much memory as needed.

https://codereview.appspot.com/5671055/diff/9001/state/util.go#newcode262
state/util.go:262: for i := 0; i < len(unsafe); i++ {
On 2012/02/23 14:57:46, niemeyer wrote:
> No need to check len on every iteration:
> 
> for i, n := 0, len(unsafe); i < n; i++ {

FWIW i don't think this is any clearer, and i believe it's no more efficient.
(the length of unsafe will be registerised just as well as n).

[niemeyer, 2012-02-23 16:07:39]

https://codereview.appspot.com/5671055/diff/9001/state/util.go
File state/util.go (right):

https://codereview.appspot.com/5671055/diff/9001/state/util.go#newcode261
state/util.go:261: safe := []byte{}
On 2012/02/23 15:24:15, rog wrote:
> On 2012/02/23 14:57:46, niemeyer wrote:
> > safe := make([]byte, 0, len(unsafe)*4)
> > 
> > This will prevent reallocating safe.
> 
> better i think to use len(unsafe). then we optimise for the common case where
no
> characters need quoting - most of the time we can avoid allocating four times
as
> much memory as needed.

You mean four times as in 128 bytes rather than 32? It doesn't make any
difference for this algorithm. In a few nanoseconds the memory will be available
for GC, with a single allocation, since the right size is converted to a string.

Please follow my original suggestion Frank.

https://codereview.appspot.com/5671055/diff/9001/state/util.go#newcode262
state/util.go:262: for i := 0; i < len(unsafe); i++ {
On 2012/02/23 15:24:15, rog wrote:
> On 2012/02/23 14:57:46, niemeyer wrote:
> > No need to check len on every iteration:
> > 
> > for i, n := 0, len(unsafe); i < n; i++ {
> 
> FWIW i don't think this is any clearer, and i believe it's no more efficient.
> (the length of unsafe will be registerised just as well as n).

You're right. And this is not even a fast path. Please keep it as you had
originally Frank, sorry for the false alarm.

[TheMue, 2012-02-24 10:38:00]

Please take a look.

[TheMue, 2012-02-24 10:38:37]

PTAL

https://codereview.appspot.com/5671055/diff/9001/state/charm.go
File state/charm.go (right):

https://codereview.appspot.com/5671055/diff/9001/state/charm.go#newcode18
state/charm.go:18: URL    string
On 2012/02/23 15:00:52, niemeyer wrote:
> Sorry, I missed this one. This should be:
> 
>     BundleURL string `yaml:"url"`
> 
> The name in yaml was a mistake we can't fix, but we can have a reasonable name
> to ourselves.

Done.

https://codereview.appspot.com/5671055/diff/9001/state/charm.go#newcode28
state/charm.go:28: }
On 2012/02/23 14:57:46, niemeyer wrote:
> Please add this here:
> 
>     var _ charm.Charm = (*Charm)(nil)
> 
> and fix what's needed to make that compile correctly.

Done.

https://codereview.appspot.com/5671055/diff/9001/state/charm.go#newcode31
state/charm.go:31: func readCharm(zk *zookeeper.Conn, charmURL *charm.URL)
(*Charm, error) {
On 2012/02/23 14:57:46, niemeyer wrote:
> Let's please standardize on curl for the variable name of a charm URL.

Done.

https://codereview.appspot.com/5671055/diff/9001/state/charm.go#newcode34
state/charm.go:34: return nil, fmt.Errorf("charm %q not found", charmURL)
On 2012/02/23 14:57:46, niemeyer wrote:
> "charm not found: %q"

Done.

https://codereview.appspot.com/5671055/diff/9001/state/charm.go#newcode46
state/charm.go:46: // newCharm creates a new charm.
On 2012/02/23 14:57:46, niemeyer wrote:
> Please either drop the comment or make it more relevant.

Done.

https://codereview.appspot.com/5671055/diff/9001/state/charm.go#newcode47
state/charm.go:47: func newCharm(zk *zookeeper.Conn, charmURL *charm.URL, data
*charmData) (*Charm, error) {
On 2012/02/23 14:57:46, niemeyer wrote:
> s/charmURL/curl/

Done.

https://codereview.appspot.com/5671055/diff/9001/state/charm.go#newcode63
state/charm.go:63: func (c *Charm) Name() string {
On 2012/02/23 14:57:46, niemeyer wrote:
> Why do we need this? That's c.Meta().Name

Done.Has been part of the Python implementation.

https://codereview.appspot.com/5671055/diff/9001/state/charm.go#newcode81
state/charm.go:81: clone := *c.meta
On 2012/02/23 14:57:46, niemeyer wrote:
> This data is inherently read-only, in the sense that there's no reason for a
> *charm.Meta to be modified. It's fine to not clone it, I believe.

Done.

https://codereview.appspot.com/5671055/diff/9001/state/charm.go#newcode87
state/charm.go:87: clone := *c.config
On 2012/02/23 14:57:46, niemeyer wrote:
> Ditto.

Done.

https://codereview.appspot.com/5671055/diff/9001/state/charm.go#newcode98
state/charm.go:98: // based on the URL.
On 2012/02/23 14:57:46, niemeyer wrote:
> /URL/charm URL/

Done.

https://codereview.appspot.com/5671055/diff/9001/state/charm.go#newcode99
state/charm.go:99: func charmPath(charmURL *charm.URL) string {
On 2012/02/23 14:57:46, niemeyer wrote:
> s/charmURL/curl/

Done.

https://codereview.appspot.com/5671055/diff/9001/state/charm.go#newcode100
state/charm.go:100: return fmt.Sprintf("/charms/%s", Quote(charmURL.String()))
On 2012/02/23 14:57:46, niemeyer wrote:
> Before this line, please check that curl has a Revision, and return an error
if
> it's < 0. Charms stored in zk must always have a revision set.

Done.

https://codereview.appspot.com/5671055/diff/9001/state/export_test.go
File state/export_test.go (left):

https://codereview.appspot.com/5671055/diff/9001/state/export_test.go#oldcode1
state/export_test.go:1: package state
On 2012/02/23 14:51:23, fwereade wrote:
> I still think it would be a good idea to have these live somewhere outside
state
> (we suggested juju/go/testing IIRC).

ACK. Should be discussed with Gustavo in IRC.

https://codereview.appspot.com/5671055/diff/9001/state/export_test.go
File state/export_test.go (right):

https://codereview.appspot.com/5671055/diff/9001/state/export_test.go#newcode7
state/export_test.go:7: // managed by juju.
On 2012/02/23 14:57:46, niemeyer wrote:
> Please remove the package comment. This is a test file.

Done.

https://codereview.appspot.com/5671055/diff/9001/state/export_test.go#newcode41
state/export_test.go:41: // ZkTearDownEnvironment destroys the ZooKeeper Test
environment.
On 2012/02/23 14:57:46, niemeyer wrote:
> s/Test/test/

Done.

https://codereview.appspot.com/5671055/diff/9001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/5671055/diff/9001/state/service.go#newcode65
state/service.go:65: return readCharm(s.zk, url)
On 2012/02/23 14:57:46, niemeyer wrote:
> Oops.. we're making a similar mistake to the one we've made in the Python
> implementation. We must have access to the State from here, so that we can do:
> 
>     return s.st.Charm(url)
> 
> Rather than having lots of external helper functions. That's the whole point
of
> centralizing the State into a single type, rather than having all the Manager
> classes.

Done.

https://codereview.appspot.com/5671055/diff/9001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/5671055/diff/9001/state/state.go#newcode40
state/state.go:40: // AddCharm registers metadata about the provided Charm.
On 2012/02/23 14:57:46, niemeyer wrote:
> Please improve this comment.

Done.

https://codereview.appspot.com/5671055/diff/9001/state/state.go#newcode41
state/state.go:41: func (s *State) AddCharm(charmURL *charm.URL, ch charm.Charm,
url string) (*Charm, error) {
On 2012/02/23 14:57:46, niemeyer wrote:
> (ch charm.Charm, curl *charm.URL, bundleUrl string), in this order please.

Done.

https://codereview.appspot.com/5671055/diff/9001/state/state.go#newcode59
state/state.go:59: func (s *State) Charm(charmURL *charm.URL) (*Charm, error) {
On 2012/02/23 14:57:46, niemeyer wrote:
> s/charmURL/curl/

Done.

https://codereview.appspot.com/5671055/diff/9001/state/state.go#newcode60
state/state.go:60: return readCharm(s.zk, charmURL)
On 2012/02/23 14:57:46, niemeyer wrote:
> We shouldn't need this helper function, unless there's somewhere we really
have
> no access to a *State that must read a charm directly from zk, which I don't
> think is the case.
> 
> The content of readCharm should be inlined here.

Done.

https://codereview.appspot.com/5671055/diff/9001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/5671055/diff/9001/state/state_test.go#newcode26
state/state_test.go:26: return filepath.Join("..", "charm", "testrepo",
"series", name)
On 2012/02/23 14:51:23, fwereade wrote:
> I already commented on this in the other branch, I think, but i can't tell
> *which* branch I should *actually* be commenting in because I don't see either
> branch marked as a prereq of the other...

The mentioned and discussed idea with a testing package containing the data and
common code sounds good.

I would like to know which problem you expect with .. in the path?

https://codereview.appspot.com/5671055/diff/9001/state/state_test.go#newcode29
state/state_test.go:29: // readCharm returns a test charm by a name.
On 2012/02/23 14:57:46, niemeyer wrote:
> s/a name/its name/

Done.

https://codereview.appspot.com/5671055/diff/9001/state/state_test.go#newcode39
state/state_test.go:39: charmURL, err := charm.ParseURL(url)
On 2012/02/23 14:57:46, niemeyer wrote:
> return charm.MustParseURL(url)

Done.

https://codereview.appspot.com/5671055/diff/9001/state/state_test.go#newcode47
state/state_test.go:47: // where it's needed.
On 2012/02/23 14:57:46, niemeyer wrote:
> // addDummyCharm adds the 'dummy' charm state to st.

Done.

https://codereview.appspot.com/5671055/diff/9001/state/state_test.go#newcode48
state/state_test.go:48: func addDummyCharm(c *C, st *state.State) *state.Charm {
On 2012/02/23 14:57:46, niemeyer wrote:
> Return the charm URL too, so that we can use it in tests below rather than
> reparsing it all the time.
> 
> I actually suggest something like this instead:
> 
> func (s *StateSuite) addDummyCharm() (*state.Charm, *charm.URL)
> 
> and panic inside it if something goes wrong. This is not what we're testing.
It
> should always work in this context.

Done.

https://codereview.appspot.com/5671055/diff/9001/state/state_test.go#newcode50
state/state_test.go:50: dummy, err := st.AddCharm(localCharmURL(ch), ch,
"http://example.com/abc")
On 2012/02/23 14:57:46, niemeyer wrote:
> Please use "http://bundle.url" as the URL at the end, so we know what it is
when
> reading this.

Done.

https://codereview.appspot.com/5671055/diff/9001/state/state_test.go#newcode92
state/state_test.go:92: dummy, err := s.st.AddCharm(localCharmURL(dummyCharm),
dummyCharm, "http://example.com/abc")
On 2012/02/23 14:57:46, niemeyer wrote:
> Isn't this copy & pasting addDummyCharm in all those cases too?
> 
> dummy, curl := ...

Done.

https://codereview.appspot.com/5671055/diff/9001/state/state_test.go#newcode94
state/state_test.go:94: c.Assert(dummy.URL().String(), Equals,
"local:series/dummy-1")
On 2012/02/23 14:57:46, niemeyer wrote:
> Please use curl.String() as the expected value. Same for all below.

Done.

[TheMue, 2012-02-24 11:48:14]

Please take a look.

[TheMue, 2012-02-27 10:28:52]

Please take a look.

[niemeyer, 2012-02-27 15:58:10]

LGTM, given the minors below are addressed:

https://codereview.appspot.com/5671055/diff/9001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/5671055/diff/9001/state/state.go#newcode41
state/state.go:41: func (s *State) AddCharm(charmURL *charm.URL, ch charm.Charm,
url string) (*Charm, error) {
On 2012/02/24 10:38:38, TheMue wrote:
> On 2012/02/23 14:57:46, niemeyer wrote:
> > (ch charm.Charm, curl *charm.URL, bundleUrl string), in this order please.
> 
> Done.

Half Done. Please fix the bundle URL argument too.

https://codereview.appspot.com/5671055/diff/9011/state/charm.go
File state/charm.go (right):

https://codereview.appspot.com/5671055/diff/9011/state/charm.go#newcode22
state/charm.go:22: zk        *zookeeper.Conn
zk should be dropped. It's now st.zk.

https://codereview.appspot.com/5671055/diff/9011/state/state.go
File state/state.go (right):

https://codereview.appspot.com/5671055/diff/9011/state/state.go#newcode41
state/state.go:41: // and its bundle URL.
Still a bit vague. Please use this instead:

// AddCharm adds the ch charm with curl to the state.
// bundleUrl must be set to a URL where the bundle for ch
// may be downloaded from.
// On success the newly added charm state is returned.

https://codereview.appspot.com/5671055/diff/9011/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/5671055/diff/9011/state/state_test.go#newcode104
state/state_test.go:104: c.Assert(dummy.URL().String(), Equals, curl.String())
This is testing that reading the URL works correctly, and interestingly the URL
doesn't even come from the stored charm data, so it's not doing what it claims.

This is also redundant with the test below, so please just drop it.

https://codereview.appspot.com/5671055/diff/9011/state/state_test.go#newcode126
state/state_test.go:126: c.Assert(meta.Name, Equals, "dummy")
Half of this test is doing the same as the test above. Just take these last two
lines and add at the bottom of the previous one.

https://codereview.appspot.com/5671055/diff/9011/state/state_test.go#newcode142
state/state_test.go:142: )
Ditto.

https://codereview.appspot.com/5671055/diff/9011/state/state_test.go#newcode146
state/state_test.go:146: // Check that getting a charm before anyone has been
added fails nicely.
s/anyone/it/

[TheMue, 2012-02-27 16:58:11]

Please take a look.

[TheMue, 2012-02-27 16:59:54]

https://codereview.appspot.com/5671055/diff/9011/state/charm.go
File state/charm.go (right):

https://codereview.appspot.com/5671055/diff/9011/state/charm.go#newcode22
state/charm.go:22: zk        *zookeeper.Conn
On 2012/02/27 15:58:10, niemeyer wrote:
> zk should be dropped. It's now st.zk.

Done.

https://codereview.appspot.com/5671055/diff/9011/state/state.go
File state/state.go (right):

https://codereview.appspot.com/5671055/diff/9011/state/state.go#newcode41
state/state.go:41: // and its bundle URL.
On 2012/02/27 15:58:10, niemeyer wrote:
> Still a bit vague. Please use this instead:
> 
> // AddCharm adds the ch charm with curl to the state.
> // bundleUrl must be set to a URL where the bundle for ch
> // may be downloaded from.
> // On success the newly added charm state is returned.

Done.

https://codereview.appspot.com/5671055/diff/9011/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/5671055/diff/9011/state/state_test.go#newcode104
state/state_test.go:104: c.Assert(dummy.URL().String(), Equals, curl.String())
On 2012/02/27 15:58:10, niemeyer wrote:
> This is testing that reading the URL works correctly, and interestingly the
URL
> doesn't even come from the stored charm data, so it's not doing what it
claims.
> 
> This is also redundant with the test below, so please just drop it.

Done.

https://codereview.appspot.com/5671055/diff/9011/state/state_test.go#newcode126
state/state_test.go:126: c.Assert(meta.Name, Equals, "dummy")
On 2012/02/27 15:58:10, niemeyer wrote:
> Half of this test is doing the same as the test above. Just take these last
two
> lines and add at the bottom of the previous one.

Done.

https://codereview.appspot.com/5671055/diff/9011/state/state_test.go#newcode142
state/state_test.go:142: )
On 2012/02/27 15:58:10, niemeyer wrote:
> Ditto.

Done.

[TheMue, 2012-02-27 17:57:59]

Please take a look.

[TheMue, 2012-02-27 18:35:41]

*** Submitted:

Implementation of the charm state.

This branch implements the charm and removes the
so far used charm mock.

R=rog, fwereade, niemeyer
CC=
https://codereview.appspot.com/5671055