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

Revision history for this message
William Reade (fwereade) wrote :

A few comments, reprising what we discussed at lunch

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

https://codereview.appspot.com/53210044/diff/1/state/apiserver/client/client.go#newcode885
state/apiserver/client/client.go:885: storageURL, err :=
storage.URL(name)
I'd prefer to generate unique names than to potentially have multiple
writers to the same storage location. I think it'd be best to have
multiple independent downloads, and multiple independent uploads, and
then just to have a single worker win the race and set a URL that
definitely worked in state. No need to rename or anything -- as far as
I'm aware, charm URL is totally independent of storage URL anyway, and
if it's not that's a bug that should be fixed anyway ;).

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

https://codereview.appspot.com/53210044/diff/1/state/apiserver/client/client_test.go#newcode1850
state/apiserver/client/client_test.go:1850: }
in light of the previous comment,we *do* want multiple charms uploaded
(but all except one to be deleted). And whichever one isn't deleted
should match the archive url of the charm in state.

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

« Back to merge proposal