Code review comment for lp:~dimitern/juju-core/260-lp-1067979-duplicate-charm

Very nice, last round I think

https://codereview.appspot.com/53210044/diff/40001/state/apiserver/client/client_test.go
File state/apiserver/client/client_test.go (right):

https://codereview.appspot.com/53210044/diff/40001/state/apiserver/client/client_test.go#newcode1821
state/apiserver/client/client_test.go:1821: close(ops)
On 2014/01/28 10:55:05, dimitern wrote:
> On 2014/01/28 08:19:41, fwereade wrote:
> > race? we should expect 9 Deletes as well, I think

> There is no way to detect storage removals with the dummy provider I
could find.
> But I am checking below that we do have a single upload at the end.
Not sure
> what race do you mean?

I was worried that the deletes *were* pushing ops, and that channel
might sometimes be closed before the ops stopped. But yeah, looks like
no delete ops; and I'm fine with the test. Just seemed a bit weird to
mix the testing styles -- but I think we're good as is. Thanks.

https://codereview.appspot.com/53210044/diff/60001/state/apiserver/charms.go
File state/apiserver/charms.go (right):

https://codereview.appspot.com/53210044/diff/60001/state/apiserver/charms.go#newcode229
state/apiserver/charms.go:229: func GetEnvironStorage(st *state.State)
(storage.Storage, error) {
maybe put this in export_test rather than growing the interface here?

https://codereview.appspot.com/53210044/diff/60001/state/apiserver/client/client_test.go
File state/apiserver/client/client_test.go (right):

https://codereview.appspot.com/53210044/diff/60001/state/apiserver/client/client_test.go#newcode1828
state/apiserver/client/client_test.go:1828: storage, err :=
apiserver.GetEnvironStorage(s.State)
ah, ok, you can't put it in export_test. Hmm. I think I'd maybe prefer
it if this were accessed via the environ on the test, but I'm not sure
of the tradeoffs -- I'll leave it to your judgment.

https://codereview.appspot.com/53210044/diff/60001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/53210044/diff/60001/state/state.go#newcode480
state/state.go:480: what := D{{"_id", curl}, {"placeholder", false},
{"pendingupload", false}}
Yeah, these bits do need to be fixed. Sorry, but they *will* break
stuff.

https://codereview.appspot.com/53210044/diff/60001/state/state.go#newcode625
state/state.go:625: Insert: uploadedCharm,
Don't we still need to deal with a placeholder here?

https://codereview.appspot.com/53210044/diff/60001/utils/trivial.go
File utils/trivial.go (right):

https://codereview.appspot.com/53210044/diff/60001/utils/trivial.go#newcode127
utils/trivial.go:127: func GetSHA256(source io.Reader) (string, int64,
error) {
ReadSHA256?

https://codereview.appspot.com/53210044/diff/60001/utils/trivial.go#newcode139
utils/trivial.go:139: func GetFileSHA256(filename string) (string,
int64, error) {
ReadFileSHA256?

https://codereview.appspot.com/53210044/diff/60001/utils/trivial.go#newcode146
utils/trivial.go:146: }
Also, please test these. Not quite trivial enough to be skippable IMO.

https://codereview.appspot.com/53210044/