Code review comment for lp:~cmars/juju-core/resolve-cs-series

Revision history for this message
John A Meinel (jameinel) wrote :

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
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).

https://codereview.appspot.com/80280043/diff/1/cmd/juju/upgradecharm.go
File cmd/juju/upgradecharm.go (left):

https://codereview.appspot.com/80280043/diff/1/cmd/juju/upgradecharm.go#oldcode208
cmd/juju/upgradecharm.go:208: }
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
File cmd/juju/upgradecharm.go (right):

https://codereview.appspot.com/80280043/diff/1/cmd/juju/upgradecharm.go#newcode148
cmd/juju/upgradecharm.go:148: }
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
state/api/client.go:573:
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/

« Back to merge proposal