Code review comment for lp:~dimitern/juju-core/260-lp-1067979-duplicate-charm
Revision history for this message
| Dimiter Naydenov (dimitern) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
bbfa235
(Get the code!)
Please take a look.
https:/
File charm/url.go (right):
https:/
charm/url.go:261: func (u *URL) ArchiveName() (string, error) {
On 2014/01/28 08:19:41, fwereade wrote:
> I'm certain this is not part of the charm package -- np with the code,
but it's
> very definitely an internal detail of the state server.
I'll move it to the state package then.
https:/
File state/apiserver
https:/
state/apiserver
int64, error) {
On 2014/01/28 08:19:41, fwereade wrote:
> whereas this feels maybe utilsy -- and we do this *all over the
place*, so
> please hunt them down and make them all use the same utils func,
assuming that
> is practical
Done.
https:/
File state/apiserver
https:/
state/apiserver
duplicated charm from storage: %v", err)
On 2014/01/28 08:19:41, fwereade wrote:
> log it, I think, but don't spoil the user's day -- we still managed to
do what
> she asked, after all
Wasn't sure whether to log an error or warning. I'll just log it as an
error then, because it seems serious enough (it shouldn't happen
usually).
https:/
File state/apiserver
https:/
state/apiserver
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?
https:/
File state/state.go (right):
https:/
state/state.go:606: err = st.charms.
{"placeholder", false}}
On 2014/01/28 08:19:41, fwereade wrote:
> placeholder might be false in a new installation, or missing in an old
one
I'll modify the condition, but this applies to other places in the code
as well.
https:/
state/state.go:624: Insert: uploadedCharm,
On 2014/01/28 08:19:41, fwereade wrote:
> if there *was* a placeholder charm, the insert would fail, wouldn't
it?
Right, but since I changed how we select the document above it shouldn't
be a problem.
https:/
state/state.go:642: StillPlaceholder = D{{"placeholder", true}}
On 2014/01/28 08:19:41, fwereade wrote:
> I think these should probably not be exported
I just reformatted the block - they were there before from Ian's branch
about placeholders. I'll unexport them.
https:/