Code review comment for lp:~cmars/juju-core/resolve-cs-series
Revision history for this message
| Casey Marshall (cmars) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
bbfa235
(Get the code!)
PTAL. If everything looks ok, I'd still like to run a few more tests
against the deployed charm store update tomorrow, before landing.
Thanks,
Casey
https:/
File cmd/juju/deploy.go (right):
https:/
cmd/juju/
ctx.AbsPath(
On 2014/03/26 07:11:17, jameinel wrote:
> This feels like stuff that definitely needs to be done, but shouldn't
be done in
> "cmd/juju/
> Is there a reason this can't be part of more "library" code and tested
as such?
> Specifically it feels like it should be a function that takes the
command line
> parameter (CharmName) a client and a conf, and returns a fully
qualified
> charm.URL (or error).
> Especially given that you essentially implemented it 2 times in this
file. (at
> least the code you added below this looks a lot like the code you
added above
> this).
Good call, I've refactored functions to cmd/common.go, shared among
deploy and upgradecharm.
https:/
File cmd/juju/
https:/
cmd/juju/
On 2014/03/26 07:11:17, jameinel wrote:
> And here, though this one seems to have a new SpecializeCharmRepo step
that I
> didn't see before.
Done.
https:/
File cmd/juju/
https:/
cmd/juju/
On 2014/03/26 07:11:17, jameinel wrote:
> And implemented again here?
Done.
https:/
File state/api/client.go (right):
https:/
state/api/
On 2014/03/26 07:11:17, jameinel wrote:
> We need to also add code to handle the fallback case. What do we do if
you are
> using juju 1.18 to deploy against a 1.16 server that doesn't have the
> ResolveCharm API.
> I'm guessing the answer is to print an error of:
> "You must supply a series in your charm URL for juju < 1.18".
> Either that, or we see that the ResolveCharm isn't available, and just
issue a
> warning with something like "server version is too old to support
ResolveCharm
> (juju <1.18) falling back to default series of "precise""
> To be clear, that compatibility code probably shouldn't be here. But
probably
> could be in the common helper I was outlining earlier.
I decided to warn and fall back on "precise" for <1.18 state servers.
These existing environments will almost certainly already have a
default-series set by their bootstrapping.
I had originally intended to resolve the series directly with the charm
store for that case (since 1.16 hits the charm store directly for other
things), but it seems unlikely that such an environment would benefit
from it.
https:/