Merge lp:~cmars/juju-core/no-default-series into lp:~go-bot/juju-core/trunk

[Casey Marshall]

Resolve implicit series with charm store.

Charm URLs without a series are "unresolved". A charm repository can resolve
the series. Charm store service resolves the series by adding CanonicalURL to
the charm info if it is found & resolved. Local repository uses default
series as configured in the environment. Command-line tools interacting with
a charm repo are responsible for making requests with resolved charm URLs.
State server requires a resolved charm URL for such requests. Charm store
service initially resolves series to a hard-coded value -- to be replaced by
more advanced logic as we transition to new LTS. Updated unit tests & mocks
accordingly.

https://codereview.appspot.com/76860044/

[Casey Marshall (cmars)]

Reviewers: mp+211389_code.launchpad.net,

Message:
Please take a look.

Description:
Resolve implicit series with charm store.

Charm URLs without a series are "unresolved". A charm repository can
resolve
the series. Charm store service resolves the series by adding
CanonicalURL to
the charm info if it is found & resolved. Local repository uses default
series as configured in the environment. Command-line tools interacting
with
a charm repo are responsible for making requests with resolved charm
URLs.
State server requires a resolved charm URL for such requests. Charm
store
service initially resolves series to a hard-coded value -- to be
replaced by
more advanced logic as we transition to new LTS. Updated unit tests &
mocks
accordingly.

https://code.launchpad.net/~cmars/juju-core/no-default-series/+merge/211389

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/76860044/

Affected files (+222, -79 lines):
   A [revision details]
   M charm/charm.go
   M charm/charm_test.go
   M charm/repo.go
   M charm/repo_test.go
   M charm/testing/mockstore.go
   M charm/url.go
   M charm/url_test.go
   M cmd/juju/deploy.go
   M cmd/juju/publish.go
   M cmd/juju/publish_test.go
   M cmd/juju/upgradecharm.go
   M cmd/juju/upgradecharm_test.go
   M environs/config/config.go
   M environs/jujutest/livetests.go
   M juju/conn_test.go
   M state/apiserver/client/client.go
   M state/apiserver/client/client_test.go
   M store/server.go
   M store/server_test.go
   M testing/charm.go

[Casey Marshall (cmars)]

Please take a look.

https://codereview.appspot.com/76860044/

[Casey Marshall (cmars)]

Please take a look.

https://codereview.appspot.com/76860044/

[Casey Marshall (cmars)]

Please take a look.

https://codereview.appspot.com/76860044/

[Casey Marshall (cmars)]

Tim,
Please review my proposal for eliminating default series from the juju
client (or pass it along to someone who's up for it). In a nutshell:

- Allow charm.URL to have an implied "empty" series.
- charm.Repository can Resolve them.
- The charm store "resolves" series by using a hard-coded value (for
now).
- A local repository uses the environment config default series.
- It's a client command's responsibility to make sure a charm URL is
resolved before calling the state server with it.

Thanks,
Casey

https://codereview.appspot.com/76860044/

[Dave Cheney (dave-cheney)]

On 2014/03/17 22:55:29, cmars wrote:
> Tim,
> Please review my proposal for eliminating default series from the juju
client
> (or pass it along to someone who's up for it). In a nutshell:

> - Allow charm.URL to have an implied "empty" series.
> - charm.Repository can Resolve them.
> - The charm store "resolves" series by using a hard-coded value (for
now).
> - A local repository uses the environment config default series.
> - It's a client command's responsibility to make sure a charm URL is
resolved
> before calling the state server with it.

> Thanks,
> Casey

I think this needs to be broken out into two changes. The store changes
need to land well before the release that contains the client changes.

https://codereview.appspot.com/76860044/

[Dave Cheney (dave-cheney)]

Some minor comments.

A larger meta comment, a lot of this change is noise due to charm.URL
being treated as a pointer where it is actually a value.

If all the references to *charm.URL were replaced with charm.URL the
amount of defensive coding in this change, and elsewhere would be
reduced.

https://codereview.appspot.com/76860044/diff/60001/charm/repo.go
File charm/repo.go (right):

https://codereview.appspot.com/76860044/diff/60001/charm/repo.go#newcode154
charm/repo.go:154: curl = &*curl
I don't understand what this line does. I think it does nothing. If if
does something then it need a comment to explain the magic.

https://codereview.appspot.com/76860044/diff/60001/charm/repo.go#newcode429
charm/repo.go:429: result := &*curl
yuk, how about

result := *curl
if !result ... {

}
return &result, nil

https://codereview.appspot.com/76860044/diff/60001/store/server.go
File store/server.go (right):

https://codereview.appspot.com/76860044/diff/60001/store/server.go#newcode80
store/server.go:80: curl.Series = "precise"
please make this a constant at the top of the file with a comment
explaining the effect of changing the constant.

https://codereview.appspot.com/76860044/

[Casey Marshall (cmars)]

Please take a look.

https://codereview.appspot.com/76860044/diff/60001/charm/repo.go
File charm/repo.go (right):

https://codereview.appspot.com/76860044/diff/60001/charm/repo.go#newcode154
charm/repo.go:154: curl = &*curl
On 2014/03/18 01:01:10, dfc wrote:
> I don't understand what this line does. I think it does nothing. If if
does
> something then it need a comment to explain the magic.

It doesn't do what I think it did. :\

https://codereview.appspot.com/76860044/diff/60001/charm/repo.go#newcode429
charm/repo.go:429: result := &*curl
On 2014/03/18 01:01:10, dfc wrote:
> yuk, how about

> result := *curl
> if !result ... {

> }
> return &result, nil

Done.

https://codereview.appspot.com/76860044/diff/60001/store/server.go
File store/server.go (right):

https://codereview.appspot.com/76860044/diff/60001/store/server.go#newcode80
store/server.go:80: curl.Series = "precise"
On 2014/03/18 01:01:10, dfc wrote:
> please make this a constant at the top of the file with a comment
explaining the
> effect of changing the constant.

Done.

https://codereview.appspot.com/76860044/

[Casey Marshall (cmars)]

On 2014/03/18 01:01:10, dfc wrote:
> Some minor comments.

> A larger meta comment, a lot of this change is noise due to charm.URL
being
> treated as a pointer where it is actually a value.

> If all the references to *charm.URL were replaced with charm.URL the
amount of
> defensive coding in this change, and elsewhere would be reduced.

I agree that referencing charm.URL as a value type would be ideal. This
morning I explored what it would take. I really don't feel comfortable
landing such a refactoring in this specific proposal -- much more of the
codebase would be affected.

https://codereview.appspot.com/76860044/

[Casey Marshall (cmars)]

dfc wrote:
> I think this needs to be broken out into two changes. The store
changes need to
> land well before the release that contains the client changes.

I've removed all the client-side changes from this proposal. PTAL.

Thanks,
Casey

https://codereview.appspot.com/76860044/

[William Reade (fwereade)]

A few comments, ping me when you're online.

https://codereview.appspot.com/76860044/diff/80001/charm/repo.go
File charm/repo.go (right):

https://codereview.appspot.com/76860044/diff/80001/charm/repo.go#newcode27
charm/repo.go:27: CanonicalURL string `json:"canonicalUrl,omitempty"`
can we make it canonical-url, please, for general consistency across the
codebase

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

https://codereview.appspot.com/76860044/diff/80001/state/apiserver/client/client.go#newcode857
state/apiserver/client/client.go:857: return fmt.Errorf("charm URL not
resolved")
I'm a little concerned that the url definition change requires that we
be defensive like this at all possible entry points into the system.
(and, to guard against misses in any one layer, to be defensive at all
subsequent layers)

OTOH, I imagine this is forward-looking in the sense that we *will* end
up with, eg, cs:nova-compute actually referencing a cross-series charm.
If that's the case we'll just hve to grow handling for unresolved urls
through the codebase as we need it, and can then drop the defensiveness.
Is that where we're going with this? If so, +1... but if you can think
of a way to help everybody know they shouldn't be letting unresolved
urls through, I'd love to hear it. As it is it's just a subtle required
feature of the API that anybody could easily miss.

Ha. We need a "how to write API stuff" document *anyway* I guess.

https://codereview.appspot.com/76860044/diff/80001/store/server.go
File store/server.go (right):

https://codereview.appspot.com/76860044/diff/80001/store/server.go#newcode19
store/server.go:19: const DefaultSeries = "precise"
So, we'll need some care for the switchover here. Are you up to date
with what needs to be done when, and where, to change this over to
trusty and get it deployed? I'm not 100% sure myself, but we need to
figure this out pretty much now...

https://codereview.appspot.com/76860044/diff/80001/store/server_test.go
File store/server_test.go (right):

https://codereview.appspot.com/76860044/diff/80001/store/server_test.go#newcode43
store/server_test.go:43: {"cs:wordpress", curl.String(), "", "", "entry
not found"},
hmm, the code in the previous file seemed to imply that we do expect to
turn cs:wordpress into cs:precise/wordpress. Feels like some tests are
missing somewhere...

https://codereview.appspot.com/76860044/diff/80001/testing/charm.go
File testing/charm.go (right):

https://codereview.appspot.com/76860044/diff/80001/testing/charm.go#newcode158
testing/charm.go:158: }
should we have a fallback here for stores without DefaultSeries set?

https://codereview.appspot.com/76860044/

[Casey Marshall (cmars)]

Please take a look.

https://codereview.appspot.com/76860044/

[Casey Marshall (cmars)]

https://codereview.appspot.com/76860044/diff/80001/charm/repo.go
File charm/repo.go (right):

https://codereview.appspot.com/76860044/diff/80001/charm/repo.go#newcode27
charm/repo.go:27: CanonicalURL string `json:"canonicalUrl,omitempty"`
On 2014/03/20 12:13:42, fwereade wrote:
> can we make it canonical-url, please, for general consistency across
the
> codebase

Done.

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

https://codereview.appspot.com/76860044/diff/80001/state/apiserver/client/client.go#newcode857
state/apiserver/client/client.go:857: return fmt.Errorf("charm URL not
resolved")
On 2014/03/20 12:13:42, fwereade wrote:
> I'm a little concerned that the url definition change requires that we
be
> defensive like this at all possible entry points into the system.
(and, to guard
> against misses in any one layer, to be defensive at all subsequent
layers)

> OTOH, I imagine this is forward-looking in the sense that we *will*
end up with,
> eg, cs:nova-compute actually referencing a cross-series charm. If
that's the
> case we'll just hve to grow handling for unresolved urls through the
codebase as
> we need it, and can then drop the defensiveness. Is that where we're
going with
> this? If so, +1... but if you can think of a way to help everybody
know they
> shouldn't be letting unresolved urls through, I'd love to hear it. As
it is it's
> just a subtle required feature of the API that anybody could easily
miss.

> Ha. We need a "how to write API stuff" document *anyway* I guess.

There should not be a cross-series charm. Charms will declare the series
they support in the charm metadata. To support multiple series, a charm
will have to have per-series branches and content; they'd effectively be
distinct charms from a deployment perspective.

When a client deploys or upgrades a charm, it is responsible for
resolving the URL with the charm repository before making the API call.
As far as I know, these are the only entry points for new charm URLs
from the user into the API server. If this is naive, let's iterate on
improving the defensiveness & I'm open to suggestion.

I'd like the state server to never have to resolve a charm URL series,
and reject all unresolved URLs.

https://codereview.appspot.com/76860044/diff/80001/store/server.go
File store/server.go (right):

https://codereview.appspot.com/76860044/diff/80001/store/server.go#newcode19
store/server.go:19: const DefaultSeries = "precise"
On 2014/03/20 12:13:42, fwereade wrote:
> So, we'll need some care for the switchover here. Are you up to date
with what
> needs to be done when, and where, to change this over to trusty and
get it
> deployed? I'm not 100% sure myself, but we need to figure this out
pretty much
> now...

The first step is to give the server the capability to resolve a series
for the client, land and deploy this first.

We should probably discuss how much we can get in this first landing. A
hard-coded default is probably the most pessimistic thing that could
possibly work while allowing clients to be forward compatible.

https://codereview.appspot.com/76860044/diff/80001/store/server_...

[Casey Marshall (cmars)]

Please take a look.

https://codereview.appspot.com/76860044/

[Casey Marshall (cmars)]

On 2014/03/20 20:01:10, cmars wrote:
> Please take a look.

William,
This latest change has the charm.URL improvements we discussed earlier,
PTAL.

I'll hold off on the charm metadata series and associated "charm store
series solver" for a follow-up branch which I'll work on next.

Thanks,
Casey

https://codereview.appspot.com/76860044/

[John A Meinel (jameinel)]

On Thu, Mar 20, 2014 at 6:51 PM, Casey Marshall
<email address hidden> wrote:
...
>
> When a client deploys or upgrades a charm, it is responsible for
> resolving the URL with the charm repository before making the API call.
> As far as I know, these are the only entry points for new charm URLs
> from the user into the API server. If this is naive, let's iterate on
> improving the defensiveness & I'm open to suggestion.
>
> I'd like the state server to never have to resolve a charm URL series,
> and reject all unresolved URLs.

I'd like to understand this a bit better. Given the API server already
supports "deploy cs:precise/foo" which it will then go to the charm
store and download, it seems perfectly reasonable that the client
could connect to *1* remote machine (the API Server) rather than also
needing to know who the Charm Store is and how it is configured, and
how it can resolve information there.
Why do you feel it is better to push this work into needing Clients to
be smart? (One argument, I guess, is that Juju GUI is already listing
the charm store in order to present anything to clients, so the only
other client today is juju-core.)
But given that the API Server already needs to know how to talk to the
charm store (to install cs:precise/foo) it doesn't seem that bad to
have it also resolve URLs.

John
=:->

[Casey Marshall (cmars)]

On 03/23/2014 04:30 AM, John A Meinel wrote:
> On Thu, Mar 20, 2014 at 6:51 PM, Casey Marshall
> <email address hidden> wrote:
> ...
>>
>> When a client deploys or upgrades a charm, it is responsible for
>> resolving the URL with the charm repository before making the API call.
>> As far as I know, these are the only entry points for new charm URLs
>> from the user into the API server. If this is naive, let's iterate on
>> improving the defensiveness & I'm open to suggestion.
>>
>> I'd like the state server to never have to resolve a charm URL series,
>> and reject all unresolved URLs.
>
> I'd like to understand this a bit better. Given the API server already
> supports "deploy cs:precise/foo" which it will then go to the charm
> store and download, it seems perfectly reasonable that the client
> could connect to *1* remote machine (the API Server) rather than also
> needing to know who the Charm Store is and how it is configured, and
> how it can resolve information there.
> Why do you feel it is better to push this work into needing Clients to
> be smart? (One argument, I guess, is that Juju GUI is already listing
> the charm store in order to present anything to clients, so the only
> other client today is juju-core.)
> But given that the API Server already needs to know how to talk to the
> charm store (to install cs:precise/foo) it doesn't seem that bad to
> have it also resolve URLs.
>

My primary goal there is to keep the state server simple, backwards-
(and forwards-) compatible. I decided to keep the current requirements
of a fully-resolved charm URL in deploy, publish, and
upgradecharm-related API calls. In my latest changes to this proposal,
this is actually enforced by the type-system -- there is a distinction
between charm.URL (a fully-resolved charm location with series) and
charm.Reference (a charm location without a series resolved).

You raise a good point that the (>= 1.18) juju client will not contact
the charm store directly. What would you think of adding an API method,
call it "ResolveCharm", which exposes the charm store series-resolving
functionality back to the state client? The state client would still be
responsible for resolving the charm, but it will not need to contact the
charm store directly to resolve it.

I don't think "ResolveCharm" would necessarily need to land in this
merge proposal. It could go into the follow-up proposal with the
client-side changes.

> John
> =:->
>

-Casey

[John A Meinel (jameinel)]

It does mean 2 round trips for something that we are resolving in the
state server anyway, but it isn't a huge problem. Better than having
the client have to go to the Charm store directly. As you say, it
doesn't have to happen in this proposal, and this is probably still a
reasonable way forward.

On Mon, Mar 24, 2014 at 6:24 AM, Casey Marshall
<email address hidden> wrote:
> On 03/23/2014 04:30 AM, John A Meinel wrote:
>> On Thu, Mar 20, 2014 at 6:51 PM, Casey Marshall
>> <email address hidden> wrote:
>> ...
>>>
>>> When a client deploys or upgrades a charm, it is responsible for
>>> resolving the URL with the charm repository before making the API call.
>>> As far as I know, these are the only entry points for new charm URLs
>>> from the user into the API server. If this is naive, let's iterate on
>>> improving the defensiveness & I'm open to suggestion.
>>>
>>> I'd like the state server to never have to resolve a charm URL series,
>>> and reject all unresolved URLs.
>>
>> I'd like to understand this a bit better. Given the API server already
>> supports "deploy cs:precise/foo" which it will then go to the charm
>> store and download, it seems perfectly reasonable that the client
>> could connect to *1* remote machine (the API Server) rather than also
>> needing to know who the Charm Store is and how it is configured, and
>> how it can resolve information there.
>> Why do you feel it is better to push this work into needing Clients to
>> be smart? (One argument, I guess, is that Juju GUI is already listing
>> the charm store in order to present anything to clients, so the only
>> other client today is juju-core.)
>> But given that the API Server already needs to know how to talk to the
>> charm store (to install cs:precise/foo) it doesn't seem that bad to
>> have it also resolve URLs.
>>
>
> My primary goal there is to keep the state server simple, backwards-
> (and forwards-) compatible. I decided to keep the current requirements
> of a fully-resolved charm URL in deploy, publish, and
> upgradecharm-related API calls. In my latest changes to this proposal,
> this is actually enforced by the type-system -- there is a distinction
> between charm.URL (a fully-resolved charm location with series) and
> charm.Reference (a charm location without a series resolved).
>
> You raise a good point that the (>= 1.18) juju client will not contact
> the charm store directly. What would you think of adding an API method,
> call it "ResolveCharm", which exposes the charm store series-resolving
> functionality back to the state client? The state client would still be
> responsible for resolving the charm, but it will not need to contact the
> charm store directly to resolve it.
>
> I don't think "ResolveCharm" would necessarily need to land in this
> merge proposal. It could go into the follow-up proposal with the
> client-side changes.
>
>> John
>> =:->
>>
>
> -Casey
>
>
> --
> https://code.launchpad.net/~cmars/juju-core/no-default-series/+merge/211389
> You are subscribed to branch lp:juju-core.

[William Reade (fwereade)]

LGTM, I think these are all trivials

https://codereview.appspot.com/76860044/diff/120001/charm/url_test.go
File charm/url_test.go (right):

https://codereview.appspot.com/76860044/diff/120001/charm/url_test.go#newcode167
charm/url_test.go:167: c.Assert(err.Error(), gc.Equals,
charm.ErrUnresolvedUrl.Error())
We'd usually check for direct equality with ErrUnresolvedUrl, and
otherwise use gc.ErrorMatches to check the string.

https://codereview.appspot.com/76860044/diff/120001/charm/url_test.go#newcode213
charm/url_test.go:213: {charm.IsValidSeries, "pre-c1se", true},
Explicit test that precise-1 is not allowed, but precise1 is?

https://codereview.appspot.com/76860044/diff/120001/state/apiserver/client/client_test.go
File state/apiserver/client/client_test.go (left):

https://codereview.appspot.com/76860044/diff/120001/state/apiserver/client/client_test.go#oldcode663
state/apiserver/client/client_test.go:663: // TODO(fwereade) make these
errors consistent one day.
If these errors are starting to look nicer, please drop the comment (and
the one below). If they're not, would you upgrade this to a tech-debt
bug and fill in the usual TODO template (name, date, bug#, \n, short
description)?

https://codereview.appspot.com/76860044/diff/120001/store/server.go
File store/server.go (right):

https://codereview.appspot.com/76860044/diff/120001/store/server.go#newcode80
store/server.go:80: return &charm.URL{Reference: ref, Series:
DefaultSeries}
I'm not seeing where this is called. Dead code?

https://codereview.appspot.com/76860044/

[Casey Marshall (cmars)]

I've restricted the series name to not include dashes, and resubmitted
the MP in launchpad with the prior branch prereq:
https://code.launchpad.net/~cmars/juju-core/cs-series-solver/+merge/212407

PTAL, thanks!

I'm a bit unsure whether/how lbox & rietveld will follow the LP
resubmit..

-Casey

https://codereview.appspot.com/76860044/diff/120001/charm/url_test.go
File charm/url_test.go (right):

https://codereview.appspot.com/76860044/diff/120001/charm/url_test.go#newcode167
charm/url_test.go:167: c.Assert(err.Error(), gc.Equals,
charm.ErrUnresolvedUrl.Error())
On 2014/03/24 09:24:09, fwereade wrote:
> We'd usually check for direct equality with ErrUnresolvedUrl, and
otherwise use
> gc.ErrorMatches to check the string.

Done.

https://codereview.appspot.com/76860044/diff/120001/charm/url_test.go#newcode213
charm/url_test.go:213: {charm.IsValidSeries, "pre-c1se", true},
On 2014/03/24 09:24:09, fwereade wrote:
> Explicit test that precise-1 is not allowed, but precise1 is?

Done.

https://codereview.appspot.com/76860044/diff/120001/state/apiserver/client/client_test.go
File state/apiserver/client/client_test.go (left):

https://codereview.appspot.com/76860044/diff/120001/state/apiserver/client/client_test.go#oldcode663
state/apiserver/client/client_test.go:663: // TODO(fwereade) make these
errors consistent one day.
On 2014/03/24 09:24:09, fwereade wrote:
> If these errors are starting to look nicer, please drop the comment
(and the one
> below). If they're not, would you upgrade this to a tech-debt bug and
fill in
> the usual TODO template (name, date, bug#, \n, short description)?

Done.

https://codereview.appspot.com/76860044/diff/120001/store/server.go
File store/server.go (right):

https://codereview.appspot.com/76860044/diff/120001/store/server.go#newcode80
store/server.go:80: return &charm.URL{Reference: ref, Series:
DefaultSeries}
On 2014/03/24 09:24:09, fwereade wrote:
> I'm not seeing where this is called. Dead code?

Yes, it is now. I've removed it.

https://codereview.appspot.com/76860044/

[John A Meinel (jameinel)]

You have to "lbox propose" again, and then it will generate a new diff on
Reitveld, it doesn't notice plain 'bzr push' to Launchpad.

On Mon, Mar 24, 2014 at 7:57 PM, Casey Marshall <
<email address hidden>> wrote:

> I've restricted the series name to not include dashes, and resubmitted
> the MP in launchpad with the prior branch prereq:
> https://code.launchpad.net/~cmars/juju-core/cs-series-solver/+merge/212407
>
> PTAL, thanks!
>
> I'm a bit unsure whether/how lbox & rietveld will follow the LP
> resubmit..
>
> -Casey
>
>
> https://codereview.appspot.com/76860044/diff/120001/charm/url_test.go
> File charm/url_test.go (right):
>
>
> https://codereview.appspot.com/76860044/diff/120001/charm/url_test.go#newcode167
> charm/url_test.go:167: c.Assert(err.Error(), gc.Equals,
> charm.ErrUnresolvedUrl.Error())
> On 2014/03/24 09:24:09, fwereade wrote:
> > We'd usually check for direct equality with ErrUnresolvedUrl, and
> otherwise use
> > gc.ErrorMatches to check the string.
>
> Done.
>
>
> https://codereview.appspot.com/76860044/diff/120001/charm/url_test.go#newcode213
> charm/url_test.go:213: {charm.IsValidSeries, "pre-c1se", true},
> On 2014/03/24 09:24:09, fwereade wrote:
> > Explicit test that precise-1 is not allowed, but precise1 is?
>
> Done.
>
>
> https://codereview.appspot.com/76860044/diff/120001/state/apiserver/client/client_test.go
> File state/apiserver/client/client_test.go (left):
>
>
> https://codereview.appspot.com/76860044/diff/120001/state/apiserver/client/client_test.go#oldcode663
> state/apiserver/client/client_test.go:663: // TODO(fwereade) make these
> errors consistent one day.
> On 2014/03/24 09:24:09, fwereade wrote:
> > If these errors are starting to look nicer, please drop the comment
> (and the one
> > below). If they're not, would you upgrade this to a tech-debt bug and
> fill in
> > the usual TODO template (name, date, bug#, \n, short description)?
>
> Done.
>
> https://codereview.appspot.com/76860044/diff/120001/store/server.go
> File store/server.go (right):
>
>
> https://codereview.appspot.com/76860044/diff/120001/store/server.go#newcode80
> store/server.go:80: return &charm.URL{Reference: ref, Series:
> DefaultSeries}
> On 2014/03/24 09:24:09, fwereade wrote:
> > I'm not seeing where this is called. Dead code?
>
> Yes, it is now. I've removed it.
>
> https://codereview.appspot.com/76860044/
>
> --
> https://code.launchpad.net/~cmars/juju-core/no-default-series/+merge/211389
> You are subscribed to branch lp:juju-core.
>

=== modified file 'charm/charm.go'
--- charm/charm.go	2013-11-07 08:51:14 +0000
+++ charm/charm.go	2014-03-20 19:57:15 +0000
@@ -40,7 +40,7 @@
 		if localRepoPath == "" {
 			return nil, errors.New("path to local repository not specified")
 		}
-		repo = &LocalRepository{localRepoPath}
+		repo = &LocalRepository{Path: localRepoPath}
 	default:
 		return nil, fmt.Errorf("unknown schema for charm URL %q", curl)
 	}

=== modified file 'charm/repo.go'
--- charm/repo.go	2014-03-07 13:56:22 +0000
+++ charm/repo.go	2014-03-20 19:57:15 +0000
@@ -24,11 +24,12 @@
 
 // InfoResponse is sent by the charm store in response to charm-info requests.
 type InfoResponse struct {
-	Revision int      `json:"revision"` // Zero is valid. Can't omitempty.
-	Sha256   string   `json:"sha256,omitempty"`
-	Digest   string   `json:"digest,omitempty"`
-	Errors   []string `json:"errors,omitempty"`
-	Warnings []string `json:"warnings,omitempty"`
+	CanonicalURL string   `json:"canonical-url,omitempty"`
+	Revision     int      `json:"revision"` // Zero is valid. Can't omitempty.
+	Sha256       string   `json:"sha256,omitempty"`
+	Digest       string   `json:"digest,omitempty"`
+	Errors       []string `json:"errors,omitempty"`
+	Warnings     []string `json:"warnings,omitempty"`
 }
 
 // EventResponse is sent by the charm store in response to charm-event requests.
@@ -53,6 +54,7 @@
 type Repository interface {
 	Get(curl *URL) (Charm, error)
 	Latest(curls ...*URL) ([]CharmRevision, error)
+	Resolve(ref Reference) (*URL, error)
 }
 
 // Latest returns the latest revision of the charm referenced by curl, regardless
@@ -137,8 +139,27 @@
 	return http.DefaultClient.Do(req)
 }
 
+// Resolve canonicalizes charm URLs, resolving references and implied series.
+func (s *CharmStore) Resolve(ref Reference) (*URL, error) {
+	infos, err := s.Info(ref)
+	if err != nil {
+		return nil, err
+	}
+	if len(infos) == 0 {
+		return nil, fmt.Errorf("missing response when resolving charm URL: %q", ref)
+	}
+	if infos[0].CanonicalURL == "" {
+		return nil, fmt.Errorf("cannot resolve charm URL: %q", ref)
+	}
+	curl, err := ParseURL(infos[0].CanonicalURL)
+	if err != nil {
+		return nil, err
+	}
+	return curl, nil
+}
+
 // Info returns details for all the specified charms in the charm store.
-func (s *CharmStore) Info(curls ...*URL) ([]*InfoResponse, error) {
+func (s *CharmStore) Info(curls ...Location) ([]*InfoResponse, error) {
 	baseURL := s.BaseURL + "/charm-info?"
 	queryParams := make([]string, len(curls), len(curls)+1)
 	for i, curl := range curls {
@@ -218,7 +239,7 @@
 }
 
 // revisions returns the revisions of the charms referenced by curls.
-func (s *CharmStore) revisions(curls ...*URL) (revisions []CharmRevision, err error) {
+func (s *CharmStore) revisions(curls ...Location) (revisions []CharmRevision, err error) {
 	infos, err := s.Info(curls...)
 	if err != nil {
 		return nil, err
@@ -246,7 +267,7 @@
 // Latest returns the latest revision of the charms referenced by curls, regardless
 // of the revision set on each curl.
 func (s *CharmStore) Latest(curls ...*URL) ([]CharmRevision, error) {
-	baseCurls := make([]*URL, len(curls))
+	baseCurls := make([]Location, len(curls))
 	for i, curl := range curls {
 		baseCurls[i] = curl.WithRevision(-1)
 	}
@@ -387,11 +408,27 @@
 //   /path/to/repository/precise/mongodb.charm
 //   /path/to/repository/precise/wordpress/
 type LocalRepository struct {
-	Path string
+	Path          string
+	defaultSeries string
 }
 
 var _ Repository = (*LocalRepository)(nil)
 
+// WithDefaultSeries returns a Repository with the default series set.
+func (r *LocalRepository) WithDefaultSeries(defaultSeries string) Repository {
+	localRepo := *r
+	localRepo.defaultSeries = defaultSeries
+	return &localRepo
+}
+
+// Resolve canonicalizes charm URLs, resolving references and implied series.
+func (r *LocalRepository) Resolve(ref Reference) (*URL, error) {
+	if r.defaultSeries == "" {
+		return nil, fmt.Errorf("cannot resolve, repository has no default series: %q", ref)
+	}
+	return &URL{Reference: ref, Series: r.defaultSeries}, nil
+}
+
 // Latest returns the latest revision of the charm referenced by curl, regardless
 // of the revision set on curl itself.
 func (r *LocalRepository) Latest(curls ...*URL) ([]CharmRevision, error) {

=== modified file 'charm/repo_test.go'
--- charm/repo_test.go	2014-03-07 13:56:22 +0000
+++ charm/repo_test.go	2014-03-20 19:57:15 +0000
@@ -173,7 +173,7 @@
 // The following tests cover the low-level CharmStore-specific API.
 
 func (s *StoreSuite) TestInfo(c *gc.C) {
-	charmURLs := []*charm.URL{
+	charmURLs := []charm.Location{
 		charm.MustParseURL("cs:series/good"),
 		charm.MustParseURL("cs:series/better"),
 		charm.MustParseURL("cs:series/best"),
@@ -387,7 +387,7 @@
 func (s *LocalRepoSuite) SetUpTest(c *gc.C) {
 	s.LoggingSuite.SetUpTest(c)
 	root := c.MkDir()
-	s.repo = &charm.LocalRepository{root}
+	s.repo = &charm.LocalRepository{Path: root}
 	s.seriesPath = filepath.Join(root, "quantal")
 	c.Assert(os.Mkdir(s.seriesPath, 0777), gc.IsNil)
 }

=== modified file 'charm/testing/mockstore.go'
--- charm/testing/mockstore.go	2014-03-07 14:15:28 +0000
+++ charm/testing/mockstore.go	2014-03-20 19:57:15 +0000
@@ -6,6 +6,7 @@
 import (
 	"bytes"
 	"encoding/json"
+	"fmt"
 	"io"
 	"net"
 	"net/http"
@@ -35,13 +36,14 @@
 	Metadata                []string
 	InfoRequestCount        int
 	InfoRequestCountNoStats int
+	DefaultSeries           string
 
 	charms map[string]int
 }
 
 // NewMockStore creates a mock charm store containing the specified charms.
 func NewMockStore(c *gc.C, charms map[string]int) *MockStore {
-	s := &MockStore{charms: charms}
+	s := &MockStore{charms: charms, DefaultSeries: "precise"}
 	f, err := os.Open(testing.Charms.BundlePath(c.MkDir(), "dummy"))
 	c.Assert(err, gc.IsNil)
 	defer f.Close()
@@ -98,7 +100,17 @@
 	for _, url := range r.Form["charms"] {
 		cr := &charm.InfoResponse{}
 		response[url] = cr
-		charmURL := charm.MustParseURL(url)
+		charmURL, err := charm.ParseURL(url)
+		if err == charm.ErrUnresolvedUrl {
+			ref, _, err := charm.ParseReference(url)
+			if err != nil {
+				panic(err)
+			}
+			if s.DefaultSeries == "" {
+				panic(fmt.Errorf("mock store lacks a default series cannot resolve charm URL: %q", url))
+			}
+			charmURL = &charm.URL{Reference: ref, Series: s.DefaultSeries}
+		}
 		switch charmURL.Name {
 		case "borken":
 			cr.Errors = append(cr.Errors, "badness")
@@ -115,6 +127,7 @@
 					cr.Revision = charmURL.Revision
 				}
 				cr.Sha256 = s.bundleSha256
+				cr.CanonicalURL = charmURL.String()
 			} else {
 				cr.Errors = append(cr.Errors, "entry not found")
 			}

=== modified file 'charm/url.go'
--- charm/url.go	2014-01-28 10:45:55 +0000
+++ charm/url.go	2014-03-20 19:57:15 +0000
@@ -13,23 +13,42 @@
 	"labix.org/v2/mgo/bson"
 )
 
-// A charm URL represents charm locations such as:
+// Location represents a charm location, which must declare a path component
+// and a string representaion.
+type Location interface {
+	Path() string
+	String() string
+}
+
+// Reference represents a charm location with an unresolved, untargeted series,
+// such as:
+//
+//     cs:~joe/wordpress
+//     cs:wordpress-42
+type Reference struct {
+	Schema   string // "cs" or "local"
+	User     string // "joe"
+	Name     string // "wordpress"
+	Revision int    // -1 if unset, N otherwise
+}
+
+// URL represents a fully resolved charm location with a specific series, such
+// as:
 //
 //     cs:~joe/oneiric/wordpress
 //     cs:oneiric/wordpress-42
 //     local:oneiric/wordpress
 //
 type URL struct {
-	Schema   string // "cs" or "local"
-	User     string // "joe"
-	Series   string // "oneiric"
-	Name     string // "wordpress"
-	Revision int    // -1 if unset, N otherwise
+	Reference
+	Series string // "oneiric"
 }
 
+var ErrUnresolvedUrl error = fmt.Errorf("charm url series is not resolved")
+
 var (
 	validUser   = regexp.MustCompile("^[a-z0-9][a-zA-Z0-9+.-]+$")
-	validSeries = regexp.MustCompile("^[a-z]+([a-z-]+[a-z])?$")
+	validSeries = regexp.MustCompile("^[a-z]+([a-z0-9-]+[a-z0-9])?$")
 	validName   = regexp.MustCompile("^[a-z][a-z0-9]*(-[a-z0-9]*[a-z][a-z0-9]*)*$")
 )
 
@@ -68,67 +87,83 @@
 // ParseURL parses the provided charm URL string into its respective
 // structure.
 func ParseURL(url string) (*URL, error) {
-	u := &URL{}
+	r, series, err := ParseReference(url)
+	if err != nil {
+		return nil, err
+	}
+	if series == "" {
+		return nil, ErrUnresolvedUrl
+	}
+	return &URL{Reference: r, Series: series}, nil
+}
+
+// ParseReference parses the provided charm Reference string into its
+// respective structure and the targeted series, if present.
+func ParseReference(url string) (Reference, string, error) {
+	r := Reference{Schema: "cs"}
+	series := ""
 	i := strings.Index(url, ":")
-	if i > 0 {
-		u.Schema = url[:i]
+	if i >= 0 {
+		r.Schema = url[:i]
 		i++
+	} else {
+		i = 0
 	}
 	// cs: or local:
-	if u.Schema != "cs" && u.Schema != "local" {
-		return nil, fmt.Errorf("charm URL has invalid schema: %q", url)
+	if r.Schema != "cs" && r.Schema != "local" {
+		return Reference{}, "", fmt.Errorf("charm URL has invalid schema: %q", url)
 	}
 	parts := strings.Split(url[i:], "/")
 	if len(parts) < 1 || len(parts) > 3 {
-		return nil, fmt.Errorf("charm URL has invalid form: %q", url)
+		return Reference{}, "", fmt.Errorf("charm URL has invalid form: %q", url)
 	}
 
 	// ~<username>
 	if strings.HasPrefix(parts[0], "~") {
-		if u.Schema == "local" {
-			return nil, fmt.Errorf("local charm URL with user name: %q", url)
+		if r.Schema == "local" {
+			return Reference{}, "", fmt.Errorf("local charm URL with user name: %q", url)
 		}
-		u.User = parts[0][1:]
-		if !IsValidUser(u.User) {
-			return nil, fmt.Errorf("charm URL has invalid user name: %q", url)
+		r.User = parts[0][1:]
+		if !IsValidUser(r.User) {
+			return Reference{}, "", fmt.Errorf("charm URL has invalid user name: %q", url)
 		}
 		parts = parts[1:]
 	}
 
 	// <series>
-	if len(parts) < 2 {
-		return nil, fmt.Errorf("charm URL without series: %q", url)
-	}
 	if len(parts) == 2 {
-		u.Series = parts[0]
-		if !IsValidSeries(u.Series) {
-			return nil, fmt.Errorf("charm URL has invalid series: %q", url)
+		series = parts[0]
+		if !IsValidSeries(series) {
+			return Reference{}, "", fmt.Errorf("charm URL has invalid series: %q", url)
 		}
 		parts = parts[1:]
 	}
+	if len(parts) < 1 {
+		return Reference{}, "", fmt.Errorf("charm URL without charm name: %q", url)
+	}
 
 	// <name>[-<revision>]
-	u.Name = parts[0]
-	u.Revision = -1
-	for i := len(u.Name) - 1; i > 0; i-- {
-		c := u.Name[i]
+	r.Name = parts[0]
+	r.Revision = -1
+	for i := len(r.Name) - 1; i > 0; i-- {
+		c := r.Name[i]
 		if c >= '0' && c <= '9' {
 			continue
 		}
-		if c == '-' && i != len(u.Name)-1 {
+		if c == '-' && i != len(r.Name)-1 {
 			var err error
-			u.Revision, err = strconv.Atoi(u.Name[i+1:])
+			r.Revision, err = strconv.Atoi(r.Name[i+1:])
 			if err != nil {
 				panic(err) // We just checked it was right.
 			}
-			u.Name = u.Name[:i]
+			r.Name = r.Name[:i]
 		}
 		break
 	}
-	if !IsValidName(u.Name) {
-		return nil, fmt.Errorf("charm URL has invalid charm name: %q", url)
+	if !IsValidName(r.Name) {
+		return Reference{}, "", fmt.Errorf("charm URL has invalid charm name: %q", url)
 	}
-	return u, nil
+	return r, series, nil
 }
 
 // InferURL returns a charm URL inferred from src. The provided
@@ -148,9 +183,12 @@
 // when src does not include that information; similarly, a missing
 // schema is assumed to be 'cs'.
 func InferURL(src, defaultSeries string) (*URL, error) {
-	if u, err := ParseURL(src); err == nil {
-		// src was a valid charm URL already
-		return u, nil
+	r, series, err := ParseReference(src)
+	if err != nil {
+		return nil, err
+	}
+	if series != "" {
+		return &URL{Reference: r, Series: series}, nil
 	}
 	if strings.HasPrefix(src, "~") {
 		return nil, fmt.Errorf("cannot infer charm URL with user but no schema: %q", src)
@@ -187,22 +225,37 @@
 }
 
 func (u *URL) Path() string {
-	if u.User != "" {
-		if u.Revision >= 0 {
-			return fmt.Sprintf("~%s/%s/%s-%d", u.User, u.Series, u.Name, u.Revision)
-		}
-		return fmt.Sprintf("~%s/%s/%s", u.User, u.Series, u.Name)
-	}
-	if u.Revision >= 0 {
-		return fmt.Sprintf("%s/%s-%d", u.Series, u.Name, u.Revision)
-	}
-	return fmt.Sprintf("%s/%s", u.Series, u.Name)
+	return u.path(u.Series)
+}
+
+func (r Reference) path(series string) string {
+	var parts []string
+	if r.User != "" {
+		parts = append(parts, fmt.Sprintf("~%s", r.User))
+	}
+	if series != "" {
+		parts = append(parts, series)
+	}
+	if r.Revision >= 0 {
+		parts = append(parts, fmt.Sprintf("%s-%d", r.Name, r.Revision))
+	} else {
+		parts = append(parts, r.Name)
+	}
+	return strings.Join(parts, "/")
+}
+
+func (r Reference) Path() string {
+	return r.path("")
 }
 
 func (u *URL) String() string {
 	return fmt.Sprintf("%s:%s", u.Schema, u.Path())
 }
 
+func (r Reference) String() string {
+	return fmt.Sprintf("%s:%s", r.Schema, r.Path())
+}
+
 // GetBSON turns u into a bson.Getter so it can be saved directly
 // on a MongoDB database with mgo.
 func (u *URL) GetBSON() (interface{}, error) {

=== modified file 'charm/url_test.go'
--- charm/url_test.go	2014-01-28 10:45:55 +0000
+++ charm/url_test.go	2014-03-20 19:57:15 +0000
@@ -6,6 +6,7 @@
 import (
 	"encoding/json"
 	"fmt"
+	"strings"
 
 	"labix.org/v2/mgo/bson"
 	gc "launchpad.net/gocheck"
@@ -21,38 +22,63 @@
 	s, err string
 	url    *charm.URL
 }{
-	{"cs:~user/series/name", "", &charm.URL{"cs", "user", "series", "name", -1}},
-	{"cs:~user/series/name-0", "", &charm.URL{"cs", "user", "series", "name", 0}},
-	{"cs:series/name", "", &charm.URL{"cs", "", "series", "name", -1}},
-	{"cs:series/name-42", "", &charm.URL{"cs", "", "series", "name", 42}},
-	{"local:series/name-1", "", &charm.URL{"local", "", "series", "name", 1}},
-	{"local:series/name", "", &charm.URL{"local", "", "series", "name", -1}},
-	{"local:series/n0-0n-n0", "", &charm.URL{"local", "", "series", "n0-0n-n0", -1}},
+	{"cs:~user/series/name", "", &charm.URL{charm.Reference{"cs", "user", "name", -1}, "series"}},
+	{"cs:~user/series/name-0", "", &charm.URL{charm.Reference{"cs", "user", "name", 0}, "series"}},
+	{"cs:series/name", "", &charm.URL{charm.Reference{"cs", "", "name", -1}, "series"}},
+	{"cs:series/name-42", "", &charm.URL{charm.Reference{"cs", "", "name", 42}, "series"}},
+	{"local:series/name-1", "", &charm.URL{charm.Reference{"local", "", "name", 1}, "series"}},
+	{"local:series/name", "", &charm.URL{charm.Reference{"local", "", "name", -1}, "series"}},
+	{"local:series/n0-0n-n0", "", &charm.URL{charm.Reference{"local", "", "n0-0n-n0", -1}, "series"}},
+	{"cs:~user/name", "", &charm.URL{charm.Reference{"cs", "user", "name", -1}, ""}},
+	{"cs:name", "", &charm.URL{charm.Reference{"cs", "", "name", -1}, ""}},
+	{"local:name", "", &charm.URL{charm.Reference{"local", "", "name", -1}, ""}},
 
 	{"bs:~user/series/name-1", "charm URL has invalid schema: .*", nil},
 	{"cs:~1/series/name-1", "charm URL has invalid user name: .*", nil},
+	{"cs:~user", "charm URL without charm name: .*", nil},
 	{"cs:~user/1/name-1", "charm URL has invalid series: .*", nil},
 	{"cs:~user/series/name-1-2", "charm URL has invalid charm name: .*", nil},
 	{"cs:~user/series/name-1-name-2", "charm URL has invalid charm name: .*", nil},
 	{"cs:~user/series/name--name-2", "charm URL has invalid charm name: .*", nil},
 	{"cs:~user/series/huh/name-1", "charm URL has invalid form: .*", nil},
-	{"cs:~user/name", "charm URL without series: .*", nil},
-	{"cs:name", "charm URL without series: .*", nil},
+	{"cs:/name", "charm URL has invalid series: .*", nil},
 	{"local:~user/series/name", "local charm URL with user name: .*", nil},
 	{"local:~user/name", "local charm URL with user name: .*", nil},
-	{"local:name", "charm URL without series: .*", nil},
 }
 
 func (s *URLSuite) TestParseURL(c *gc.C) {
 	for i, t := range urlTests {
 		c.Logf("test %d", i)
-		url, err := charm.ParseURL(t.s)
+		url, uerr := charm.ParseURL(t.s)
+		ref, series, rerr := charm.ParseReference(t.s)
 		comment := gc.Commentf("ParseURL(%q)", t.s)
-		if t.err != "" {
-			c.Check(err.Error(), gc.Matches, t.err, comment)
+		if t.url != nil && t.url.Series == "" {
+			if t.err != "" {
+				// Expected error should match
+				c.Assert(rerr, gc.NotNil, comment)
+				c.Check(rerr.Error(), gc.Matches, t.err, comment)
+			} else {
+				// Expected charm reference should match
+				c.Check(ref, gc.DeepEquals, t.url.Reference, comment)
+				c.Check(t.url.Reference.String(), gc.Equals, t.s)
+			}
+			if rerr != nil {
+				// If ParseReference has an error, ParseURL should share it
+				c.Check(uerr.Error(), gc.Equals, rerr.Error(), comment)
+			} else {
+				// Otherwise, ParseURL with an empty series should error unresolved.
+				c.Check(uerr.Error(), gc.Equals, charm.ErrUnresolvedUrl.Error(), comment)
+			}
 		} else {
-			c.Check(url, gc.DeepEquals, t.url, comment)
-			c.Check(t.url.String(), gc.Equals, t.s)
+			if t.err != "" {
+				c.Assert(uerr, gc.NotNil, comment)
+				c.Check(uerr.Error(), gc.Matches, t.err, comment)
+				c.Check(uerr.Error(), gc.Equals, rerr.Error(), comment)
+			} else {
+				c.Check(url.Series, gc.Equals, series, comment)
+				c.Check(url, gc.DeepEquals, t.url, comment)
+				c.Check(t.url.String(), gc.Equals, t.s)
+			}
 		}
 	}
 }
@@ -82,31 +108,35 @@
 		comment := gc.Commentf("InferURL(%q, %q)", t.vague, "defseries")
 		inferred, ierr := charm.InferURL(t.vague, "defseries")
 		parsed, perr := charm.ParseURL(t.exact)
-		if parsed != nil {
+		if perr == nil {
 			c.Check(inferred, gc.DeepEquals, parsed, comment)
+			c.Check(ierr, gc.IsNil)
 		} else {
 			expect := perr.Error()
 			if t.vague != t.exact {
-				expect = fmt.Sprintf("%s (URL inferred from %q)", expect, t.vague)
+				if colIdx := strings.Index(expect, ":"); colIdx > 0 {
+					expect = expect[:colIdx]
+				}
 			}
-			c.Check(ierr.Error(), gc.Equals, expect, comment)
+			c.Check(ierr.Error(), gc.Matches, expect+".*", comment)
 		}
 	}
 	u, err := charm.InferURL("~blah", "defseries")
 	c.Assert(u, gc.IsNil)
-	c.Assert(err, gc.ErrorMatches, "cannot infer charm URL with user but no schema: .*")
+	c.Assert(err, gc.ErrorMatches, "charm URL without charm name: .*")
 }
 
 var inferNoDefaultSeriesTests = []struct {
 	vague, exact string
+	resolved     bool
 }{
-	{"foo", ""},
-	{"foo-1", ""},
-	{"cs:foo", ""},
-	{"cs:~user/foo", ""},
-	{"series/foo", "cs:series/foo"},
-	{"cs:series/foo", "cs:series/foo"},
-	{"cs:~user/series/foo", "cs:~user/series/foo"},
+	{"foo", "", false},
+	{"foo-1", "", false},
+	{"cs:foo", "", false},
+	{"cs:~user/foo", "", false},
+	{"series/foo", "cs:series/foo", true},
+	{"cs:series/foo", "cs:series/foo", true},
+	{"cs:~user/series/foo", "cs:~user/series/foo", true},
 }
 
 func (s *URLSuite) TestInferURLNoDefaultSeries(c *gc.C) {
@@ -122,6 +152,23 @@
 	}
 }
 
+func (s *URLSuite) TestParseUnresolved(c *gc.C) {
+	for _, t := range inferNoDefaultSeriesTests {
+		if t.resolved {
+			url, err := charm.ParseURL(t.vague)
+			c.Assert(err, gc.IsNil)
+			c.Assert(url.Series, gc.Not(gc.Equals), "")
+		} else {
+			_, series, err := charm.ParseReference(t.vague)
+			c.Assert(err, gc.IsNil)
+			c.Assert(series, gc.Equals, "")
+			_, err = charm.ParseURL(t.vague)
+			c.Assert(err, gc.NotNil)
+			c.Assert(err.Error(), gc.Equals, charm.ErrUnresolvedUrl.Error())
+		}
+	}
+}
+
 var validTests = []struct {
 	valid  func(string) bool
 	string string
@@ -160,31 +207,38 @@
 	{charm.IsValidSeries, "pre cise", false},
 	{charm.IsValidSeries, "pre-cise", true},
 	{charm.IsValidSeries, "pre^cise", false},
-	{charm.IsValidSeries, "prec1se", false},
+	{charm.IsValidSeries, "prec1se", true},
 	{charm.IsValidSeries, "-precise", false},
 	{charm.IsValidSeries, "precise-", false},
-	{charm.IsValidSeries, "pre-c1se", false},
+	{charm.IsValidSeries, "pre-c1se", true},
 }
 
 func (s *URLSuite) TestValidCheckers(c *gc.C) {
 	for i, t := range validTests {
 		c.Logf("test %d: %s", i, t.string)
-		c.Assert(t.valid(t.string), gc.Equals, t.expect)
+		c.Assert(t.valid(t.string), gc.Equals, t.expect, gc.Commentf("%s", t.string))
 	}
 }
 
 func (s *URLSuite) TestMustParseURL(c *gc.C) {
 	url := charm.MustParseURL("cs:series/name")
-	c.Assert(url, gc.DeepEquals, &charm.URL{"cs", "", "series", "name", -1})
-	f := func() { charm.MustParseURL("local:name") }
-	c.Assert(f, gc.PanicMatches, "charm URL without series: .*")
+	c.Assert(url, gc.DeepEquals,
+		&charm.URL{Reference: charm.Reference{"cs", "", "name", -1}, Series: "series"})
+	f := func() { charm.MustParseURL("local:@@/name") }
+	c.Assert(f, gc.PanicMatches, "charm URL has invalid series: .*")
+	f = func() { charm.MustParseURL("cs:~user") }
+	c.Assert(f, gc.PanicMatches, "charm URL without charm name: .*")
+	f = func() { charm.MustParseURL("cs:~user") }
+	c.Assert(f, gc.PanicMatches, "charm URL without charm name: .*")
+	f = func() { charm.MustParseURL("cs:name") }
+	c.Assert(f, gc.PanicMatches, "charm url series is not resolved")
 }
 
 func (s *URLSuite) TestWithRevision(c *gc.C) {
 	url := charm.MustParseURL("cs:series/name")
 	other := url.WithRevision(1)
-	c.Assert(url, gc.DeepEquals, &charm.URL{"cs", "", "series", "name", -1})
-	c.Assert(other, gc.DeepEquals, &charm.URL{"cs", "", "series", "name", 1})
+	c.Assert(url, gc.DeepEquals, &charm.URL{charm.Reference{"cs", "", "name", -1}, "series"})
+	c.Assert(other, gc.DeepEquals, &charm.URL{charm.Reference{"cs", "", "name", 1}, "series"})
 
 	// Should always copy. The opposite behavior is error prone.
 	c.Assert(other.WithRevision(1), gc.Not(gc.Equals), other)

=== modified file 'cmd/juju/bootstrap_test.go'
--- cmd/juju/bootstrap_test.go	2014-03-19 04:06:58 +0000
+++ cmd/juju/bootstrap_test.go	2014-03-20 19:57:15 +0000
@@ -292,8 +292,8 @@
 	err:  `invalid value "bad=wrong" for flag --constraints: unknown constraint "bad"`,
 }, {
 	info: "bad --series",
-	args: []string{"--series", "bad1"},
-	err:  `invalid value "bad1" for flag --series: invalid series name "bad1"`,
+	args: []string{"--series", "1bad1"},
+	err:  `invalid value "1bad1" for flag --series: invalid series name "1bad1"`,
 }, {
 	info: "lonely --series",
 	args: []string{"--series", "fine"},

=== modified file 'environs/jujutest/livetests.go'
--- environs/jujutest/livetests.go	2014-03-14 02:19:35 +0000
+++ environs/jujutest/livetests.go	2014-03-20 19:57:15 +0000
@@ -450,7 +450,7 @@
 	c.Logf("deploying service")
 	repoDir := c.MkDir()
 	url := coretesting.Charms.ClonedURL(repoDir, mtools0.Version.Series, "dummy")
-	sch, err := conn.PutCharm(url, &charm.LocalRepository{repoDir}, false)
+	sch, err := conn.PutCharm(url, &charm.LocalRepository{Path: repoDir}, false)
 	c.Assert(err, gc.IsNil)
 	svc, err := conn.State.AddService("dummy", "user-admin", sch)
 	c.Assert(err, gc.IsNil)

=== modified file 'juju/conn_test.go'
--- juju/conn_test.go	2014-03-17 22:42:51 +0000
+++ juju/conn_test.go	2014-03-20 19:57:15 +0000
@@ -337,10 +337,12 @@
 	// Invent a URL that points to the bundled charm, and
 	// test putting that.
 	curl := &charm.URL{
-		Schema:   "local",
-		Series:   "quantal",
-		Name:     "riak",
-		Revision: -1,
+		Reference: charm.Reference{
+			Schema:   "local",
+			Name:     "riak",
+			Revision: -1,
+		},
+		Series: "quantal",
 	}
 	_, err = s.conn.PutCharm(curl, s.repo, true)
 	c.Assert(err, gc.ErrorMatches, `cannot increment revision of charm "local:quantal/riak-7": not a directory`)
@@ -356,7 +358,7 @@
 }
 
 func (s *ConnSuite) TestPutCharmUpload(c *gc.C) {
-	repo := &charm.LocalRepository{c.MkDir()}
+	repo := &charm.LocalRepository{Path: c.MkDir()}
 	curl := coretesting.Charms.ClonedURL(repo.Path, "quantal", "riak")
 
 	// Put charm for the first time.

=== modified file 'state/apiserver/charms.go'
--- state/apiserver/charms.go	2014-03-13 05:57:38 +0000
+++ state/apiserver/charms.go	2014-03-20 19:57:15 +0000
@@ -197,10 +197,12 @@
 	}
 	// We got it, now let's reserve a charm URL for it in state.
 	archiveURL := &charm.URL{
-		Schema:   "local",
-		Series:   series,
-		Name:     archive.Meta().Name,
-		Revision: archive.Revision(),
+		Reference: charm.Reference{
+			Schema:   "local",
+			Name:     archive.Meta().Name,
+			Revision: archive.Revision(),
+		},
+		Series: series,
 	}
 	preparedURL, err := h.state.PrepareLocalCharmUpload(archiveURL)
 	if err != nil {

=== modified file 'state/apiserver/client/client_test.go'
--- state/apiserver/client/client_test.go	2014-03-19 03:18:41 +0000
+++ state/apiserver/client/client_test.go	2014-03-20 19:57:15 +0000
@@ -220,7 +220,12 @@
 	{
 		about: "invalid URL",
 		url:   "not-valid",
-		err:   `charm URL has invalid schema: "not-valid"`,
+		err:   "charm url series is not resolved",
+	},
+	{
+		about: "invalid schema",
+		url:   "not-valid:your-arguments",
+		err:   `charm URL has invalid schema: "not-valid:your-arguments"`,
 	},
 	{
 		about: "unknown charm",
@@ -661,8 +666,8 @@
 	defer restore()
 	for url, expect := range map[string]string{
 		// TODO(fwereade) make these errors consistent one day.
-		"wordpress":                   `charm URL has invalid schema: "wordpress"`,
-		"cs:wordpress":                `charm URL without series: "cs:wordpress"`,
+		"wordpress":                   "charm url series is not resolved",
+		"cs:wordpress":                "charm url series is not resolved",
 		"cs:precise/wordpress":        "charm url must include revision",
 		"cs:precise/wordpress-999999": `cannot download charm ".*": charm not found in mock store: cs:precise/wordpress-999999`,
 	} {
@@ -839,8 +844,8 @@
 	s.AddTestingService(c, "wordpress", s.AddTestingCharm(c, "wordpress"))
 	for charmUrl, expect := range map[string]string{
 		// TODO(fwereade,Makyo) make these errors consistent one day.
-		"wordpress":                   `charm URL has invalid schema: "wordpress"`,
-		"cs:wordpress":                `charm URL without series: "cs:wordpress"`,
+		"wordpress":                   "charm url series is not resolved",
+		"cs:wordpress":                "charm url series is not resolved",
 		"cs:precise/wordpress":        "charm url must include revision",
 		"cs:precise/wordpress-999999": `cannot download charm ".*": charm not found in mock store: cs:precise/wordpress-999999`,
 	} {
@@ -1073,8 +1078,8 @@
 	s.AddTestingService(c, "wordpress", s.AddTestingCharm(c, "wordpress"))
 	for url, expect := range map[string]string{
 		// TODO(fwereade,Makyo) make these errors consistent one day.
-		"wordpress":                   `charm URL has invalid schema: "wordpress"`,
-		"cs:wordpress":                `charm URL without series: "cs:wordpress"`,
+		"wordpress":                   "charm url series is not resolved",
+		"cs:wordpress":                "charm url series is not resolved",
 		"cs:precise/wordpress":        "charm url must include revision",
 		"cs:precise/wordpress-999999": `cannot download charm ".*": charm not found in mock store: cs:precise/wordpress-999999`,
 	} {
@@ -1839,8 +1844,8 @@
 
 	client := s.APIState.Client()
 	// First test the sanity checks.
-	err := client.AddCharm(&charm.URL{Name: "nonsense"})
-	c.Assert(err, gc.ErrorMatches, `charm URL has invalid schema: ":/nonsense-0"`)
+	err := client.AddCharm(&charm.URL{Reference: charm.Reference{Name: "nonsense"}})
+	c.Assert(err, gc.ErrorMatches, `charm URL has invalid schema: ":nonsense-0"`)
 	err = client.AddCharm(charm.MustParseURL("local:precise/dummy"))
 	c.Assert(err, gc.ErrorMatches, "only charm store charm URLs are supported, with cs: schema")
 	err = client.AddCharm(charm.MustParseURL("cs:precise/wordpress"))

=== modified file 'store/lpad.go'
--- store/lpad.go	2013-07-09 10:32:23 +0000
+++ store/lpad.go	2014-03-20 19:57:15 +0000
@@ -69,7 +69,10 @@
 		urls := []*charm.URL{curl}
 		schema, name := curl.Schema, curl.Name
 		for _, series := range tip.OfficialSeries {
-			curl = &charm.URL{Schema: schema, Name: name, Series: series, Revision: -1}
+			curl = &charm.URL{
+				Reference: charm.Reference{Schema: schema, Name: name, Revision: -1},
+				Series:    series,
+			}
 			curl.Series = series
 			curl.User = ""
 			urls = append(urls, curl)

=== modified file 'store/server.go'
--- store/server.go	2013-11-12 14:39:29 +0000
+++ store/server.go	2014-03-20 19:57:15 +0000
@@ -16,6 +16,8 @@
 	"launchpad.net/juju-core/log"
 )
 
+const DefaultSeries = "precise"
+
 // Server is an http.Handler that serves the HTTP API of juju
 // so that juju clients can retrieve published charms.
 type Server struct {
@@ -74,6 +76,21 @@
 	return []string{kind, curl.Series, curl.Name, curl.User}
 }
 
+func (s *Server) resolveSeries(ref charm.Reference) *charm.URL {
+	return &charm.URL{Reference: ref, Series: DefaultSeries}
+}
+
+func (s *Server) resolveURL(url string) (*charm.URL, error) {
+	ref, series, err := charm.ParseReference(url)
+	if err != nil {
+		return nil, err
+	}
+	if series == "" {
+		return &charm.URL{Reference: ref, Series: DefaultSeries}, nil
+	}
+	return &charm.URL{Reference: ref, Series: series}, nil
+}
+
 func (s *Server) serveInfo(w http.ResponseWriter, r *http.Request) {
 	if r.URL.Path != "/charm-info" {
 		w.WriteHeader(http.StatusNotFound)
@@ -84,7 +101,7 @@
 	for _, url := range r.Form["charms"] {
 		c := &charm.InfoResponse{}
 		response[url] = c
-		curl, err := charm.ParseURL(url)
+		curl, err := s.resolveURL(url)
 		var info *CharmInfo
 		if err == nil {
 			info, err = s.store.CharmInfo(curl)
@@ -92,6 +109,7 @@
 		var skey []string
 		if err == nil {
 			skey = charmStatsKey(curl, "charm-info")
+			c.CanonicalURL = curl.String()
 			c.Sha256 = info.BundleSha256()
 			c.Revision = info.Revision()
 			c.Digest = info.Digest()
@@ -132,7 +150,7 @@
 		}
 		c := &charm.EventResponse{}
 		response[url] = c
-		curl, err := charm.ParseURL(url)
+		curl, err := s.resolveURL(url)
 		var event *CharmEvent
 		if err == nil {
 			event, err = s.store.CharmEvent(curl, digest)
@@ -169,7 +187,7 @@
 	if !strings.HasPrefix(r.URL.Path, "/charm/") {
 		panic("serveCharm: bad url")
 	}
-	curl, err := charm.ParseURL("cs:" + r.URL.Path[len("/charm/"):])
+	curl, err := s.resolveURL("cs:" + r.URL.Path[len("/charm/"):])
 	if err != nil {
 		w.WriteHeader(http.StatusNotFound)
 		return

=== modified file 'store/server_test.go'
--- store/server_test.go	2013-11-12 14:39:29 +0000
+++ store/server_test.go	2014-03-20 19:57:15 +0000
@@ -21,7 +21,7 @@
 )
 
 func (s *StoreSuite) prepareServer(c *gc.C) (*store.Server, *charm.URL) {
-	curl := charm.MustParseURL("cs:oneiric/wordpress")
+	curl := charm.MustParseURL("cs:precise/wordpress")
 	pub, err := s.store.CharmPublisher([]*charm.URL{curl}, "some-digest")
 	c.Assert(err, gc.IsNil)
 	err = pub.Publish(&FakeCharmDir{})
@@ -37,10 +37,12 @@
 	req, err := http.NewRequest("GET", "/charm-info", nil)
 	c.Assert(err, gc.IsNil)
 
-	var tests = []struct{ url, sha, digest, err string }{
-		{curl.String(), fakeRevZeroSha, "some-digest", ""},
-		{"cs:oneiric/non-existent", "", "", "entry not found"},
-		{"cs:bad", "", "", `charm URL without series: "cs:bad"`},
+	var tests = []struct{ url, canonical, sha, digest, err string }{
+		{curl.String(), curl.String(), fakeRevZeroSha, "some-digest", ""},
+		{"cs:oneiric/non-existent", "", "", "", "entry not found"},
+		{"cs:wordpress", curl.String(), fakeRevZeroSha, "some-digest", ""},
+		{"cs:/bad", "", "", "", `charm URL has invalid series: "cs:/bad"`},
+		{"gopher:archie-server", "", "", "", `charm URL has invalid schema: "gopher:archie-server"`},
 	}
 
 	for _, t := range tests {
@@ -51,9 +53,10 @@
 		expected := make(map[string]interface{})
 		if t.sha != "" {
 			expected[t.url] = map[string]interface{}{
-				"revision": float64(0),
-				"sha256":   t.sha,
-				"digest":   t.digest,
+				"canonical-url": t.canonical,
+				"revision":      float64(0),
+				"sha256":        t.sha,
+				"digest":        t.digest,
 			}
 		} else {
 			expected[t.url] = map[string]interface{}{
@@ -64,11 +67,12 @@
 		obtained := map[string]interface{}{}
 		err = json.NewDecoder(rec.Body).Decode(&obtained)
 		c.Assert(err, gc.IsNil)
-		c.Assert(obtained, gc.DeepEquals, expected)
+		c.Assert(obtained, gc.DeepEquals, expected, gc.Commentf("URL: %s", t.url))
 		c.Assert(rec.Header().Get("Content-Type"), gc.Equals, "application/json")
 	}
 
-	s.checkCounterSum(c, []string{"charm-info", curl.Series, curl.Name}, false, 1)
+	// 2 charm-info events, one for resolved URL, one for the reference.
+	s.checkCounterSum(c, []string{"charm-info", curl.Series, curl.Name}, false, 2)
 	s.checkCounterSum(c, []string{"charm-missing", "oneiric", "non-existent"}, false, 1)
 }
 

=== modified file 'testing/charm.go'
--- testing/charm.go	2014-03-07 13:56:22 +0000
+++ testing/charm.go	2014-03-20 19:57:15 +0000
@@ -96,10 +96,12 @@
 	}
 	clone(dst, r.DirPath(name))
 	return &charm.URL{
-		Schema:   "local",
-		Series:   series,
-		Name:     name,
-		Revision: -1,
+		Reference: charm.Reference{
+			Schema:   "local",
+			Name:     name,
+			Revision: -1,
+		},
+		Series: series,
 	}
 }
 
@@ -126,9 +128,10 @@
 // MockCharmStore implements charm.Repository and is used to isolate tests
 // that would otherwise need to hit the real charm store.
 type MockCharmStore struct {
-	charms    map[string]map[int]*charm.Bundle
-	AuthAttrs string
-	TestMode  bool
+	charms        map[string]map[int]*charm.Bundle
+	AuthAttrs     string
+	TestMode      bool
+	DefaultSeries string
 }
 
 func NewMockCharmStore() *MockCharmStore {
@@ -145,6 +148,18 @@
 	return s
 }
 
+func (s *MockCharmStore) WithDefaultSeries(series string) charm.Repository {
+	s.DefaultSeries = series
+	return s
+}
+
+func (s *MockCharmStore) Resolve(ref charm.Reference) (*charm.URL, error) {
+	if s.DefaultSeries == "" {
+		return nil, fmt.Errorf("missing default series, cannot resolve charm url: %q", ref)
+	}
+	return &charm.URL{Reference: ref, Series: s.DefaultSeries}, nil
+}
+
 // SetCharm adds and removes charms in s. The affected charm is identified by
 // charmURL, which must be revisioned. If bundle is nil, the charm will be
 // removed; otherwise, it will be stored. It is an error to store a bundle