cmd/juju: deploy using the API + 1.16 compat.

cmd/juju: deploy using the API + 1.16 compat.

juju deploy starts to use the API only, if possible.
Older API servers, not supporting charm uploads are
detected and the old 1.16-compatible code with a
direct state connection is executed.

In addition to that, bug #1216830 is fixed (no juju.Conn
usage in state/apiserver antmore), which required some
refactoring, namely moving conn.DeployService and also
conn.AddUnits out of juju.Conn and into a juju/deploy.go,
taking a *state.State as argument.

https://code.launchpad.net/~dimitern/juju-core/227-cli-deploy-via-api/+merge/199655

Requires: https://code.launchpad.net/~dimitern/juju-core/226-addcharm-api/+merge/199478

(do not edit description out of merge proposal)

[dimitern, 2013-12-19 12:46:56]

Please take a look.

[rog, 2013-12-19 14:20:12]

Looks OK in general, but some suggestions and comments below.

https://codereview.appspot.com/44240045/diff/1/cmd/juju/deploy.go
File cmd/juju/deploy.go (right):

https://codereview.appspot.com/44240045/diff/1/cmd/juju/deploy.go#newcode141
cmd/juju/deploy.go:141: attrs, err := client.EnvironmentGet()
We can actually avoid this round trip if the charm name has specified a series
(we could call InferURL with "" for the default series to find out).
Perhaps leave it as a TODO.

https://codereview.appspot.com/44240045/diff/1/cmd/juju/deploy.go#newcode144
cmd/juju/deploy.go:144: "faling back to 1.16 compatibility mode (direct DB
access)")
s/faling/falling/

https://codereview.appspot.com/44240045/diff/1/cmd/juju/deploy.go#newcode170
cmd/juju/deploy.go:170: need1dot16Compatibility := false
I'm finding it difficult to follow the logic here because the code for at least
three different versions is scattered around.

I'd be happier to see just two straight-line code paths, one for compatibility
mode and one not. That way it's trivial to remove the compatibility path by just
deleting some code.

The only wrinkle is that we'd need to find out in advance which path to take. We
could do that by initially probing to see if AddCharm is supported by the API
(there are a few possible ways to do that)

https://codereview.appspot.com/44240045/diff/1/cmd/juju/deploy.go#newcode210
cmd/juju/deploy.go:210: conn, err = juju.NewConnFromName(c.EnvName)
I known this happens only in compatibility mode, but it seems a pity that we're
caling NewConnFromName (a really slow operation - it takes > 10s for me) twice
when we don't need to. If we go with my suggestion above logic above, then it
becomes easy to do this.

https://codereview.appspot.com/44240045/diff/1/cmd/juju/deploy.go#newcode215
cmd/juju/deploy.go:215: sch, err := conn.PutCharm(curl, repo, c.BumpRevision)
I think we only need to do this if the charm's local (see comment on the
conn.DeployService call below)

https://codereview.appspot.com/44240045/diff/1/cmd/juju/deploy.go#newcode228
cmd/juju/deploy.go:228: if ch.Meta().Subordinate {
How does this work if we're not in compatibility mode and we're not using a
local charm? AFAICS in that case this will just panic because ch is nil.
(I'm surprised a test case hasn't caught this, but also see my request for more
straightforward logic so that this kind of thing is more obvious)

https://codereview.appspot.com/44240045/diff/1/cmd/juju/deploy.go#newcode259
cmd/juju/deploy.go:259: if !need1dot16Compatibility {
Could we invert the condition here, so it matches the other "if
need1dot16compatibility" statements?

https://codereview.appspot.com/44240045/diff/1/cmd/juju/deploy.go#newcode271
cmd/juju/deploy.go:271: _, err = conn.DeployService(juju.DeployServiceParams{
Do we need to do this? I thought 1.16 supported ServiceDeploy in the API.

https://codereview.appspot.com/44240045/diff/1/state/apiserver/client/client.go
File state/apiserver/client/client.go (left):

https://codereview.appspot.com/44240045/diff/1/state/apiserver/client/client....
state/apiserver/client/client.go:251: ch, err := conn.PutCharm(curl, store,
false)
ISTM that we don't need any of the new logic.
I think we can just add a comment above this line:

// When we no longer need to be compatible with the 1.16
// API, replace this with a check that the
// charm is already uploaded to the state.

I think that will be functionally equivalent to the logic
you've added.

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

https://codereview.appspot.com/44240045/diff/1/state/apiserver/client/client....
state/apiserver/client/client.go:21: jujuerrors "launchpad.net/juju-core/errors"
d

https://codereview.appspot.com/44240045/diff/1/state/apiserver/client/client....
state/apiserver/client/client.go:288: _, err =
conn.DeployService(juju.DeployServiceParams{
It's probably not time yet, but I'd like to move DeployService out of Conn (Conn
will disappear in time).

[dimitern, 2013-12-19 15:36:18]

Please take a look.

https://codereview.appspot.com/44240045/diff/1/cmd/juju/deploy.go
File cmd/juju/deploy.go (right):

https://codereview.appspot.com/44240045/diff/1/cmd/juju/deploy.go#newcode141
cmd/juju/deploy.go:141: attrs, err := client.EnvironmentGet()
On 2013/12/19 14:20:12, rog wrote:
> We can actually avoid this round trip if the charm name has specified a series
> (we could call InferURL with "" for the default series to find out).
> Perhaps leave it as a TODO.

I don't think skipping EnvironmentGet for the sake of 1 less round-trip is worth
it, because we need the config later anyway (for local charms), and most
importantly will allow me to gate the 1.16 compatibility code nicely below.
As you said online, InferURL should probably return a specific error when
there's no series in the URL and no defaultSeries is given, but  it's out of
scope for this CL.

https://codereview.appspot.com/44240045/diff/1/cmd/juju/deploy.go#newcode144
cmd/juju/deploy.go:144: "faling back to 1.16 compatibility mode (direct DB
access)")
On 2013/12/19 14:20:12, rog wrote:
> s/faling/falling/

Done.

https://codereview.appspot.com/44240045/diff/1/cmd/juju/deploy.go#newcode170
cmd/juju/deploy.go:170: need1dot16Compatibility := false
On 2013/12/19 14:20:12, rog wrote:
> I'm finding it difficult to follow the logic here because the code for at
least
> three different versions is scattered around.
> 
> I'd be happier to see just two straight-line code paths, one for compatibility
> mode and one not. That way it's trivial to remove the compatibility path by
just
> deleting some code.
> 
> The only wrinkle is that we'd need to find out in advance which path to take.
We
> could do that by initially probing to see if AddCharm is supported by the API
> (there are a few possible ways to do that)

I agree it will be clearer to separate the code paths better, but wasn't sure
that EnvironmentGet is entirely unsupported in 1.16. I checked it isn't, so I'll
use that as the early indication to branch the code.

https://codereview.appspot.com/44240045/diff/1/cmd/juju/deploy.go#newcode210
cmd/juju/deploy.go:210: conn, err = juju.NewConnFromName(c.EnvName)
On 2013/12/19 14:20:12, rog wrote:
> I known this happens only in compatibility mode, but it seems a pity that
we're
> caling NewConnFromName (a really slow operation - it takes > 10s for me) twice
> when we don't need to. If we go with my suggestion above logic above, then it
> becomes easy to do this.

This is now changed - compatibility mode uses different code.

https://codereview.appspot.com/44240045/diff/1/cmd/juju/deploy.go#newcode215
cmd/juju/deploy.go:215: sch, err := conn.PutCharm(curl, repo, c.BumpRevision)
On 2013/12/19 14:20:12, rog wrote:
> I think we only need to do this if the charm's local (see comment on the
> conn.DeployService call below)

ditto

https://codereview.appspot.com/44240045/diff/1/cmd/juju/deploy.go#newcode228
cmd/juju/deploy.go:228: if ch.Meta().Subordinate {
On 2013/12/19 14:20:12, rog wrote:
> How does this work if we're not in compatibility mode and we're not using a
> local charm? AFAICS in that case this will just panic because ch is nil.
> (I'm surprised a test case hasn't caught this, but also see my request for
more
> straightforward logic so that this kind of thing is more obvious)

You're right, but this is fixed by splitting the code paths.

https://codereview.appspot.com/44240045/diff/1/cmd/juju/deploy.go#newcode259
cmd/juju/deploy.go:259: if !need1dot16Compatibility {
On 2013/12/19 14:20:13, rog wrote:
> Could we invert the condition here, so it matches the other "if
> need1dot16compatibility" statements?

This is now changed.

https://codereview.appspot.com/44240045/diff/1/cmd/juju/deploy.go#newcode271
cmd/juju/deploy.go:271: _, err = conn.DeployService(juju.DeployServiceParams{
On 2013/12/19 14:20:13, rog wrote:
> Do we need to do this? I thought 1.16 supported ServiceDeploy in the API.

I'm keeping the compatibility code unchanged, since it'll go away eventually
anyway.

https://codereview.appspot.com/44240045/diff/1/state/apiserver/client/client.go
File state/apiserver/client/client.go (left):

https://codereview.appspot.com/44240045/diff/1/state/apiserver/client/client....
state/apiserver/client/client.go:251: ch, err := conn.PutCharm(curl, store,
false)
On 2013/12/19 14:20:13, rog wrote:
> ISTM that we don't need any of the new logic.
> I think we can just add a comment above this line:
> 
> // When we no longer need to be compatible with the 1.16
> // API, replace this with a check that the
> // charm is already uploaded to the state.
> 
> I think that will be functionally equivalent to the logic
> you've added.

Of course we do! The changes actually assume we're using the API properly,
instead of in a compatible mode. When should we implement the new logic? When we
drop the 1.16 compat? I don't see the point not having it now. The new logic
should remain when we drop the 1.16 compatibility, but it also works with the
old code. I think I commented well enough what should be removed and what not.

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

https://codereview.appspot.com/44240045/diff/1/state/apiserver/client/client....
state/apiserver/client/client.go:288: _, err =
conn.DeployService(juju.DeployServiceParams{
On 2013/12/19 14:20:13, rog wrote:
> It's probably not time yet, but I'd like to move DeployService out of Conn
(Conn
> will disappear in time).

I agree 100%, hence the bug # in that TODO above. Once we drop 1.16
compatibility we can stop using Conn.DeployService (and get rid of a bunch of
duplicated code as a side-effect).

[rog, 2013-12-19 17:06:12]

https://codereview.appspot.com/44240045/diff/1/cmd/juju/deploy.go
File cmd/juju/deploy.go (right):

https://codereview.appspot.com/44240045/diff/1/cmd/juju/deploy.go#newcode141
cmd/juju/deploy.go:141: attrs, err := client.EnvironmentGet()
On 2013/12/19 15:36:19, dimitern wrote:
> On 2013/12/19 14:20:12, rog wrote:
> > We can actually avoid this round trip if the charm name has specified a
series
> > (we could call InferURL with "" for the default series to find out).
> > Perhaps leave it as a TODO.
> 
> I don't think skipping EnvironmentGet for the sake of 1 less round-trip is
worth
> it, because we need the config later anyway (for local charms), and most
> importantly will allow me to gate the 1.16 compatibility code nicely below.

I accept the second reason, but AFAICS the only reason we need EnvironmentGet is
to fill in the series of any charm URL (local *or* cs) without one.

> As you said online, InferURL should probably return a specific error when
> there's no series in the URL and no defaultSeries is given, but  it's out of
> scope for this CL.

Yeah, we can implement the optimisation later if we fancy it.

https://codereview.appspot.com/44240045/diff/20001/cmd/juju/deploy.go
File cmd/juju/deploy.go (right):

https://codereview.appspot.com/44240045/diff/20001/cmd/juju/deploy.go#newcode268
cmd/juju/deploy.go:268: latest, err := repo.Latest(curl)
Thought for the future: it might be a good idea to try to avoid any direct
client->charm-store communication - that way we can avoid the client needing to
know the charm store credentials. It would be easy to do we allowed AddCharm to
accept a charm URL without a revision - it could do the Latest call on the
server side.

https://codereview.appspot.com/44240045/diff/20001/cmd/juju/deploy_test.go
File cmd/juju/deploy_test.go (right):

https://codereview.appspot.com/44240045/diff/20001/cmd/juju/deploy_test.go#ne...
cmd/juju/deploy_test.go:4: package main
I don't see any tests here that test deploying a cs: charm.
I appreciate that there were none before, but the fact that in an earlier
version of this branch, the most commonly used code path would have resulted in
a panic (and that was untested) makes me slightly nervous.

Is there a reasonable way to do this?

https://codereview.appspot.com/44240045/diff/20001/juju/testing/repo.go
File juju/testing/repo.go (right):

https://codereview.appspot.com/44240045/diff/20001/juju/testing/repo.go#newco...
juju/testing/repo.go:45: // only "precise" charms are available.
perhaps we should just use "quantal" above.

https://codereview.appspot.com/44240045/diff/20001/state/apiserver/client/cli...
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/44240045/diff/20001/state/apiserver/client/cli...
state/apiserver/client/client.go:251: need1dot16Compatibility = true
I think we can be quite a bit simpler here:

if jujuerrors.IsNotFoundError(err) {
     // Remove this code when 1.16 compatibility is dropped.
     err := c.AddCharm(params.CharmURL{args.CharmURL}
     if err != nil {
         return err
     }
}

[dimitern, 2013-12-19 18:24:26]

Please take a look.

https://codereview.appspot.com/44240045/diff/20001/cmd/juju/deploy.go
File cmd/juju/deploy.go (right):

https://codereview.appspot.com/44240045/diff/20001/cmd/juju/deploy.go#newcode268
cmd/juju/deploy.go:268: latest, err := repo.Latest(curl)
On 2013/12/19 17:06:12, rog wrote:
> Thought for the future: it might be a good idea to try to avoid any direct
> client->charm-store communication - that way we can avoid the client needing
to
> know the charm store credentials. It would be easy to do we allowed AddCharm
to
> accept a charm URL without a revision - it could do the Latest call on the
> server side.

Agreed, this is a good idea, but not for now.

https://codereview.appspot.com/44240045/diff/20001/cmd/juju/deploy_test.go
File cmd/juju/deploy_test.go (right):

https://codereview.appspot.com/44240045/diff/20001/cmd/juju/deploy_test.go#ne...
cmd/juju/deploy_test.go:4: package main
On 2013/12/19 17:06:12, rog wrote:
> I don't see any tests here that test deploying a cs: charm.
> I appreciate that there were none before, but the fact that in an earlier
> version of this branch, the most commonly used code path would have resulted
in
> a panic (and that was untested) makes me slightly nervous.
> 
> Is there a reasonable way to do this?

There is, using testing.MockCharmStore, but it's not well suited without some
refactoring, which is way out of scope for this CL. I tested the deploy changes
live on EC2 with cs: urls in several forms and it seems to work fine.

https://codereview.appspot.com/44240045/diff/20001/juju/testing/repo.go
File juju/testing/repo.go (right):

https://codereview.appspot.com/44240045/diff/20001/juju/testing/repo.go#newco...
juju/testing/repo.go:45: // only "precise" charms are available.
On 2013/12/19 17:06:12, rog wrote:
> perhaps we should just use "quantal" above.

I tried that, it breaks a huge amount of tests, written to expect "precise".
It's a huge mess, I tried several approaches to fix it, but completely fixing it
requires changing a lot of unrelated code, so a symlink works unobtrusively.

https://codereview.appspot.com/44240045/diff/20001/state/apiserver/client/cli...
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/44240045/diff/20001/state/apiserver/client/cli...
state/apiserver/client/client.go:251: need1dot16Compatibility = true
On 2013/12/19 17:06:12, rog wrote:
> I think we can be quite a bit simpler here:
> 
> if jujuerrors.IsNotFoundError(err) {
>      // Remove this code when 1.16 compatibility is dropped.
>      err := c.AddCharm(params.CharmURL{args.CharmURL}
>      if err != nil {
>          return err
>      }
> }

Done slightly differently (AddCharm supports only cs: charms, so we still need
the check), in addition to moving conn.DeployService and conn.AddUnits out of
juju.Conn and making them independent functions, taking *state.State, as
discussed. This also fixes bug #1216830.

[rog, 2013-12-19 18:41:24]

LGTM modulo the below.

It'd be nice to fix lp#1216830 but I don't mind if it happens in another CL.

https://codereview.appspot.com/44240045/diff/20001/state/apiserver/client/cli...
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/44240045/diff/20001/state/apiserver/client/cli...
state/apiserver/client/client.go:251: need1dot16Compatibility = true
On 2013/12/19 18:24:26, dimitern wrote:
> On 2013/12/19 17:06:12, rog wrote:
> > I think we can be quite a bit simpler here:
> > 
> > if jujuerrors.IsNotFoundError(err) {
> >      // Remove this code when 1.16 compatibility is dropped.
> >      err := c.AddCharm(params.CharmURL{args.CharmURL}
> >      if err != nil {
> >          return err
> >      }
> > }
> 
> Done slightly differently (AddCharm supports only cs: charms, so we still need
> the check)

See comment on new code.

Bug 1216830 isn't fixed unless you change serviceSetCharm too, BTW.

https://codereview.appspot.com/44240045/diff/40001/state/apiserver/client/cli...
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/44240045/diff/40001/state/apiserver/client/cli...
state/apiserver/client/client.go:228: // It assumes
It assumes... ?

I guess you wanted to say something like:

// AddCharm or AddLocalCharm should be called to add the charm
// before calling ServiceDeploy, although for backward compatibility
// this is not necessary until 1.16 support is removed.

https://codereview.appspot.com/44240045/diff/40001/state/apiserver/client/cli...
state/apiserver/client/client.go:241: // Remove this while if block when 1.16
compatibility is dropped.
s/while/whole/ ?

https://codereview.appspot.com/44240045/diff/40001/state/apiserver/client/cli...
state/apiserver/client/client.go:242: if curl.Schema != "cs" {
Isn't this check replicated in AddCharm itself?

[dimitern, 2013-12-19 19:38:10]

Please take a look.

https://codereview.appspot.com/44240045/diff/20001/state/apiserver/client/cli...
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/44240045/diff/20001/state/apiserver/client/cli...
state/apiserver/client/client.go:251: need1dot16Compatibility = true
On 2013/12/19 18:41:25, rog wrote:
> On 2013/12/19 18:24:26, dimitern wrote:
> > On 2013/12/19 17:06:12, rog wrote:
> > > I think we can be quite a bit simpler here:
> > > 
> > > if jujuerrors.IsNotFoundError(err) {
> > >      // Remove this code when 1.16 compatibility is dropped.
> > >      err := c.AddCharm(params.CharmURL{args.CharmURL}
> > >      if err != nil {
> > >          return err
> > >      }
> > > }
> > 
> > Done slightly differently (AddCharm supports only cs: charms, so we still
need
> > the check)
> 
> See comment on new code.
> 
> Bug 1216830 isn't fixed unless you change serviceSetCharm too, BTW.

You're correct, I'll reassign the bug to the follow-up, which fixes this.

https://codereview.appspot.com/44240045/diff/40001/state/apiserver/client/cli...
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/44240045/diff/40001/state/apiserver/client/cli...
state/apiserver/client/client.go:228: // It assumes
On 2013/12/19 18:41:25, rog wrote:
> It assumes... ?
> 
> I guess you wanted to say something like:
> 
> // AddCharm or AddLocalCharm should be called to add the charm
> // before calling ServiceDeploy, although for backward compatibility
> // this is not necessary until 1.16 support is removed.

Done.

https://codereview.appspot.com/44240045/diff/40001/state/apiserver/client/cli...
state/apiserver/client/client.go:241: // Remove this while if block when 1.16
compatibility is dropped.
On 2013/12/19 18:41:25, rog wrote:
> s/while/whole/ ?

Done.

https://codereview.appspot.com/44240045/diff/40001/state/apiserver/client/cli...
state/apiserver/client/client.go:242: if curl.Schema != "cs" {
On 2013/12/19 18:41:25, rog wrote:
> Isn't this check replicated in AddCharm itself?

Not exactly with the same message, and I decided to keep this one for
compatibility.