Usable, if incomplete, deploy command

Usable, if incomplete, deploy command

Notable missing bits are:

* --config (which depends on us implementing charm format 2, and some other
  state stuff that hasn't yet been done; attempts to use --config will
  currently panic)
* --constraints (which aren't implemented at all; we can't parse them,
  and don't even expose a --constraints flag)

Please note that the charm store is not tested here. This is because the
actual charm store URL is hidden away inside the state package and (AIUI)
explicitly not intended to be changed or even changeable, leaving no route
that allows me to test it directly without either hitting the real charm
store in unit tests, or messing with /etc/hosts... either of which would be
a Bad Thing.

I submit that InferRepository is sufficiently well-tested within the charm
package, and so is the charm store; and that to write direct tests for
deploying charm store charms at this level is not necessary, because the
behavior with local URLs is sufficiently consistent with usage of
InferRepository that we can still have a high level of confidence in the
operation of the command as a whole. (For example, we verify that charm
bundles are not upgradeable; the source of the bundles should therefore not
be relevant).

When we have enough in place that we can have functional tests, we should
certainly verify the behaviour against the actual charm store.

https://code.launchpad.net/~fwereade/juju-core/usable-deploy-command/+merge/111068

(do not edit description out of merge proposal)

[fwereade, 2012-06-20 08:52:22]

Please take a look.

[niemeyer, 2012-06-25 23:19:22]

This is an awesome bootstrap.

Besides the comments below, though, there's a detail to sort out, and I'd
appreciate if you could tackle it in a CL in the near future so that we don't
dive into a path of no return.

Do you recall that conversation we've had in Orlando, regarding what juju.Conn
was supposed to be? This is a type that will enable us to use the interface of
juju as a library, and it may actually end up playing a pretty important role
soon once we move to a remote API model.

To achieve that, I believe pretty much all of the deploy command functionality
should be available via commands of the juju.Conn type. I imagine it would be
simple to refactor this code in a follow up CL to be based on the following
building blocks:

func (conn *juju.Conn) PutCharm(curl *charm.URL) (*state.Charm, error)

type DeployInfo struct {
    Charm *state.Charm
    ServiceName string // Defaults to the charm name
    NumUnits int       // Defaults to 1
    BumpRevision bool
}

func (conn *juju.Conn) Deploy(info *DeployInfo) error


I've filed that as a bug so you can move on with this CL without fixing it:

https://bugs.launchpad.net/juju-core/+bug/1017742

https://codereview.appspot.com/6297101/diff/2001/cmd/juju/cmd_test.go
File cmd/juju/cmd_test.go (right):

https://codereview.appspot.com/6297101/diff/2001/cmd/juju/cmd_test.go#newcode16
cmd/juju/cmd_test.go:16: type envFixture struct {
Let's please continue using the Suite suffix until we explicitly agree with
everyone to do otherwise in well defined cases and replace all instances
matching the well defined case at once. Otherwise we'll be sitting in the
middle, as that kind of change generally goes.

https://codereview.appspot.com/6297101/diff/2001/cmd/juju/cmd_test.go#newcode62
cmd/juju/cmd_test.go:62: s.LoggingSuite.SetUpTest(c)
And here is the proof. :-)

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

https://codereview.appspot.com/6297101/diff/2001/cmd/juju/deploy.go#newcode25
cmd/juju/deploy.go:25: Upgrade     bool
Can we rename this to BumpRevision? Upgrade is such a bad name. We have to keep
--upgrade, but at least for ourselves we can be more clear.

https://codereview.appspot.com/6297101/diff/2001/cmd/juju/deploy.go#newcode58
cmd/juju/deploy.go:58: f.IntVar(&c.NumUnits, "n", 1, "number of service units to
deploy; ignored for subordinate charms")
Might read better if stated on the positive:

"number of service units to deploy for principal charms"

https://codereview.appspot.com/6297101/diff/2001/cmd/juju/deploy.go#newcode87
cmd/juju/deploy.go:87: // if --upgrade is specified, and the charm is capable of
being upgraded,
The doc seems a bit misleading. Every charm is capable of being upgraded. Feel
free to tweak further, but I'd imagine something along those lines:

// getCharm returns the charm and charm URL specified on the command line.
// If --upgrade is specified, the charm must be local, and will have
// its version bumped before being returned.

https://codereview.appspot.com/6297101/diff/2001/cmd/juju/deploy.go#newcode90
cmd/juju/deploy.go:90: repo, curl, err := charm.InferRepository(c.CharmName,
defaultSeries, c.RepoPath)
I now realize that we got the interface for this method slightly wrong, but it's
easy to fix in a follow up later. The two first parameters are only useful for
InferCharm, which means we can separate infering a repository from resolving the
alias. We should have this instead:

repo, err := charm.InferRepository(curl)

Filed as a bug for a future CL:

https://bugs.launchpad.net/juju-core/+bug/1017732

https://codereview.appspot.com/6297101/diff/2001/cmd/juju/deploy.go#newcode106
cmd/juju/deploy.go:106: if chd, ok := ch.(*charm.Dir); ok {
To save one level in the stair case, and bring the error closer to where it is
being generated, this might be done as:

chd, ok := ch.(*charm.Dir)
if !ok {
    return ...
}

https://codereview.appspot.com/6297101/diff/2001/cmd/juju/deploy.go#newcode112
cmd/juju/deploy.go:112: return nil, nil, fmt.Errorf("cannot upgrade charm bundle
%q", curl)
The error message should reflect the test done without assumptions:

fmt.Errorf("can't upgrade: %q is not a charm directory")

https://codereview.appspot.com/6297101/diff/2001/cmd/juju/deploy.go#newcode119
cmd/juju/deploy.go:119: // and adds a state.Charm to zookeeper state.
s/zookeeper/the/

https://codereview.appspot.com/6297101/diff/2001/cmd/juju/deploy.go#newcode121
cmd/juju/deploy.go:121: // TODO get default series from environ
Why is this a TODO? We have EnvironConfig() in the state already, and can even
introduce a DefaultSeries method in the state to avoid using strings everywhere
(and risk typos).

https://codereview.appspot.com/6297101/diff/2001/cmd/juju/deploy.go#newcode133
cmd/juju/deploy.go:133: var buf bytes.Buffer
These functions have streaming interfaces so that we don't need to load big
charms in memory. We should take advantage of that.

Filed as a bug for a future CL:

https://bugs.launchpad.net/juju-core/+bug/1017733

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

https://codereview.appspot.com/6297101/diff/2001/cmd/juju/deploy_test.go#newc...
cmd/juju/deploy_test.go:156: c.Assert(err, NotNil)
ErrorMatches

https://codereview.appspot.com/6297101/diff/2001/cmd/juju/deploy_test.go#newc...
cmd/juju/deploy_test.go:158: c.Assert(err, NotNil)
ErrorMatches

[fwereade, 2012-06-26 10:10:43]

Please take a look.

https://codereview.appspot.com/6297101/diff/2001/cmd/juju/cmd_test.go
File cmd/juju/cmd_test.go (right):

https://codereview.appspot.com/6297101/diff/2001/cmd/juju/cmd_test.go#newcode16
cmd/juju/cmd_test.go:16: type envFixture struct {
On 2012/06/25 23:19:22, niemeyer wrote:
> Let's please continue using the Suite suffix until we explicitly agree with
> everyone to do otherwise in well defined cases and replace all instances
> matching the well defined case at once. Otherwise we'll be sitting in the
> middle, as that kind of change generally goes.

Done.

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

https://codereview.appspot.com/6297101/diff/2001/cmd/juju/deploy.go#newcode25
cmd/juju/deploy.go:25: Upgrade     bool
On 2012/06/25 23:19:22, niemeyer wrote:
> Can we rename this to BumpRevision? Upgrade is such a bad name. We have to
keep
> --upgrade, but at least for ourselves we can be more clear.

SGTM. Done.

https://codereview.appspot.com/6297101/diff/2001/cmd/juju/deploy.go#newcode58
cmd/juju/deploy.go:58: f.IntVar(&c.NumUnits, "n", 1, "number of service units to
deploy; ignored for subordinate charms")
On 2012/06/25 23:19:22, niemeyer wrote:
> Might read better if stated on the positive:
> 
> "number of service units to deploy for principal charms"

Done.

https://codereview.appspot.com/6297101/diff/2001/cmd/juju/deploy.go#newcode87
cmd/juju/deploy.go:87: // if --upgrade is specified, and the charm is capable of
being upgraded,
On 2012/06/25 23:19:22, niemeyer wrote:
> The doc seems a bit misleading. Every charm is capable of being upgraded. Feel
> free to tweak further, but I'd imagine something along those lines:
> 
> // getCharm returns the charm and charm URL specified on the command line.
> // If --upgrade is specified, the charm must be local, and will have
> // its version bumped before being returned.

Done.

https://codereview.appspot.com/6297101/diff/2001/cmd/juju/deploy.go#newcode90
cmd/juju/deploy.go:90: repo, curl, err := charm.InferRepository(c.CharmName,
defaultSeries, c.RepoPath)
On 2012/06/25 23:19:22, niemeyer wrote:
> I now realize that we got the interface for this method slightly wrong, but
it's
> easy to fix in a follow up later. The two first parameters are only useful for
> InferCharm, which means we can separate infering a repository from resolving
the
> alias. We should have this instead:
> 
> repo, err := charm.InferRepository(curl)
> 
> Filed as a bug for a future CL:
> 
> https://bugs.launchpad.net/juju-core/+bug/1017732

Cheers.

https://codereview.appspot.com/6297101/diff/2001/cmd/juju/deploy.go#newcode106
cmd/juju/deploy.go:106: if chd, ok := ch.(*charm.Dir); ok {
On 2012/06/25 23:19:22, niemeyer wrote:
> To save one level in the stair case, and bring the error closer to where it is
> being generated, this might be done as:
> 
> chd, ok := ch.(*charm.Dir)
> if !ok {
>     return ...
> }
> 

Done.

https://codereview.appspot.com/6297101/diff/2001/cmd/juju/deploy.go#newcode112
cmd/juju/deploy.go:112: return nil, nil, fmt.Errorf("cannot upgrade charm bundle
%q", curl)
On 2012/06/25 23:19:22, niemeyer wrote:
> The error message should reflect the test done without assumptions:
> 
> fmt.Errorf("can't upgrade: %q is not a charm directory")

Done.

https://codereview.appspot.com/6297101/diff/2001/cmd/juju/deploy.go#newcode119
cmd/juju/deploy.go:119: // and adds a state.Charm to zookeeper state.
On 2012/06/25 23:19:22, niemeyer wrote:
> s/zookeeper/the/

Done.

https://codereview.appspot.com/6297101/diff/2001/cmd/juju/deploy.go#newcode121
cmd/juju/deploy.go:121: // TODO get default series from environ
On 2012/06/25 23:19:22, niemeyer wrote:
> Why is this a TODO? We have EnvironConfig() in the state already, and can even
> introduce a DefaultSeries method in the state to avoid using strings
everywhere
> (and risk typos).

But default-series is not yet handled anywhere else, and I'm reluctant to extend
this CL to include changes to environs. Can I do a separate
environ-default-series CL please?

https://codereview.appspot.com/6297101/diff/2001/cmd/juju/deploy.go#newcode133
cmd/juju/deploy.go:133: var buf bytes.Buffer
On 2012/06/25 23:19:22, niemeyer wrote:
> These functions have streaming interfaces so that we don't need to load big
> charms in memory. We should take advantage of that.
> 
> Filed as a bug for a future CL:
> 
> https://bugs.launchpad.net/juju-core/+bug/1017733

Thanks.

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

https://codereview.appspot.com/6297101/diff/2001/cmd/juju/deploy_test.go#newc...
cmd/juju/deploy_test.go:156: c.Assert(err, NotNil)
On 2012/06/25 23:19:22, niemeyer wrote:
> ErrorMatches

Done.

https://codereview.appspot.com/6297101/diff/2001/cmd/juju/deploy_test.go#newc...
cmd/juju/deploy_test.go:158: c.Assert(err, NotNil)
On 2012/06/25 23:19:22, niemeyer wrote:
> ErrorMatches

Done.

[niemeyer, 2012-06-26 23:54:57]

LGTM

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

https://codereview.appspot.com/6297101/diff/2001/cmd/juju/deploy.go#newcode121
cmd/juju/deploy.go:121: // TODO get default series from environ
On 2012/06/26 10:10:44, fwereade wrote:
> On 2012/06/25 23:19:22, niemeyer wrote:
> > Why is this a TODO? We have EnvironConfig() in the state already, and can
even
> > introduce a DefaultSeries method in the state to avoid using strings
> everywhere
> > (and risk typos).
> 
> But default-series is not yet handled anywhere else, and I'm reluctant to
extend
> this CL to include changes to environs. Can I do a separate
> environ-default-series CL please?

Sounds good. Filed a bug:

https://bugs.launchpad.net/juju-core/+bug/1018197

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

https://codereview.appspot.com/6297101/diff/8001/cmd/juju/deploy.go#newcode87
cmd/juju/deploy.go:87: // If --upgrade is specified, the charm must be a local
directory , and will
s/ , /, /

[fwereade, 2012-06-27 08:10:41]

*** Submitted:

Usable, if incomplete, deploy command

Notable missing bits are:

* --config (which depends on us implementing charm format 2, and some other
  state stuff that hasn't yet been done; attempts to use --config will
  currently panic)
* --constraints (which aren't implemented at all; we can't parse them,
  and don't even expose a --constraints flag)

Please note that the charm store is not tested here. This is because the
actual charm store URL is hidden away inside the state package and (AIUI)
explicitly not intended to be changed or even changeable, leaving no route
that allows me to test it directly without either hitting the real charm
store in unit tests, or messing with /etc/hosts... either of which would be
a Bad Thing.

I submit that InferRepository is sufficiently well-tested within the charm
package, and so is the charm store; and that to write direct tests for
deploying charm store charms at this level is not necessary, because the
behavior with local URLs is sufficiently consistent with usage of
InferRepository that we can still have a high level of confidence in the
operation of the command as a whole. (For example, we verify that charm
bundles are not upgradeable; the source of the bundles should therefore not
be relevant).

When we have enough in place that we can have functional tests, we should
certainly verify the behaviour against the actual charm store.

R=niemeyer
CC=
https://codereview.appspot.com/6297101

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

https://codereview.appspot.com/6297101/diff/8001/cmd/juju/deploy.go#newcode87
cmd/juju/deploy.go:87: // If --upgrade is specified, the charm must be a local
directory , and will
On 2012/06/26 23:54:57, niemeyer wrote:
> s/ , /, /

Done.