I think we have some common code that should be factored out, and I
think we need to consider how the code will operate with older Juju
server versions, but otherwise I think this looks pretty good.
https://codereview.appspot.com/80280043/diff/1/cmd/juju/deploy.go#newcode155
cmd/juju/deploy.go:155: repo, err := charm.InferRepository(ref,
ctx.AbsPath(c.RepoPath))
This feels like stuff that definitely needs to be done, but shouldn't be
done in "cmd/juju/deploy.go" which is a "main" sort of function.
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).
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 think we have some common code that should be factored out, and I
think we need to consider how the code will operate with older Juju
server versions, but otherwise I think this looks pretty good.
https:/ /codereview. appspot. com/80280043/ diff/1/ cmd/juju/ deploy. go
File cmd/juju/deploy.go (right):
https:/ /codereview. appspot. com/80280043/ diff/1/ cmd/juju/ deploy. go#newcode155 deploy. go:155: repo, err := charm.InferRepo sitory( ref, c.RepoPath) ) deploy. go" which is a "main" sort of function.
cmd/juju/
ctx.AbsPath(
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).
https:/ /codereview. appspot. com/80280043/ diff/1/ cmd/juju/ upgradecharm. go upgradecharm. go (left):
File cmd/juju/
https:/ /codereview. appspot. com/80280043/ diff/1/ cmd/juju/ upgradecharm. go#oldcode208 upgradecharm. go:208: }
cmd/juju/
And here, though this one seems to have a new SpecializeCharmRepo step
that I didn't see before.
https:/ /codereview. appspot. com/80280043/ diff/1/ cmd/juju/ upgradecharm. go upgradecharm. go (right):
File cmd/juju/
https:/ /codereview. appspot. com/80280043/ diff/1/ cmd/juju/ upgradecharm. go#newcode148 upgradecharm. go:148: }
cmd/juju/
And implemented again here?
https:/ /codereview. appspot. com/80280043/ diff/1/ state/api/ client. go
File state/api/client.go (right):
https:/ /codereview. appspot. com/80280043/ diff/1/ state/api/ client. go#newcode573 client. go:573:
state/api/
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.
https:/ /codereview. appspot. com/80280043/