Code review comment for lp:~cmars/juju-core/no-default-series

Revision history for this message
Casey Marshall (cmars) wrote :

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_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"},
On 2014/03/20 12:13:42, fwereade wrote:
> 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...

This test case demonstrates that the server is capable of resolving the
ambiguous charm URL. If this capability is expanded, I'll certainly add
more.. or did you have something in mind?

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: }
On 2014/03/20 12:13:42, fwereade wrote:
> should we have a fallback here for stores without DefaultSeries set?

Done, if I'm understanding the concern.

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

« Back to merge proposal