jujud: principal unit agent deploys subordinates

jujud: principal unit agent deploys subordinates

...and the machine agent's principal-deployment is now tested.

https://code.launchpad.net/~fwereade/juju-core/omg-subordinates/+merge/141737

(do not edit description out of merge proposal)

[fwereade, 2013-01-03 11:43:28]

Please take a look.

[dimitern, 2013-01-03 21:56:59]

LGTM, but I have some questions.

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

https://codereview.appspot.com/7028049/diff/1/cmd/jujud/deploy.go#newcode33
cmd/jujud/deploy.go:33: func (m *fakeManager) RecallUnit(unitName string) error
{
What's the idea here? RecallUnit does something like "undeploy unit".

https://codereview.appspot.com/7028049/diff/1/cmd/jujud/deploy.go#newcode100
cmd/jujud/deploy.go:100: if reflect.DeepEqual(got, want) {
Why return if equal?

[fwereade, 2013-01-04 08:06:09]

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

https://codereview.appspot.com/7028049/diff/1/cmd/jujud/deploy.go#newcode33
cmd/jujud/deploy.go:33: func (m *fakeManager) RecallUnit(unitName string) error
{
On 2013/01/03 21:56:59, dimitern wrote:
> What's the idea here? RecallUnit does something like "undeploy unit".

fakeManager is a deployer.Manager, which is an interface with these 3 methods.
(FWIW, deploy/recall are opposites)

https://codereview.appspot.com/7028049/diff/1/cmd/jujud/deploy.go#newcode100
cmd/jujud/deploy.go:100: if reflect.DeepEqual(got, want) {
On 2013/01/03 21:56:59, dimitern wrote:
> Why return if equal?

Because the units we expect are deployed, and we don't need to wait any more.

[dimitern, 2013-01-04 09:21:05]

On 2013/01/04 08:06:09, fwereade wrote:
> https://codereview.appspot.com/7028049/diff/1/cmd/jujud/deploy.go
> File cmd/jujud/deploy.go (right):
> 
> https://codereview.appspot.com/7028049/diff/1/cmd/jujud/deploy.go#newcode33
> cmd/jujud/deploy.go:33: func (m *fakeManager) RecallUnit(unitName string)
error
> {
> On 2013/01/03 21:56:59, dimitern wrote:
> > What's the idea here? RecallUnit does something like "undeploy unit".
> 
> fakeManager is a deployer.Manager, which is an interface with these 3 methods.
> (FWIW, deploy/recall are opposites)
> 
> https://codereview.appspot.com/7028049/diff/1/cmd/jujud/deploy.go#newcode100
> cmd/jujud/deploy.go:100: if reflect.DeepEqual(got, want) {
> On 2013/01/03 21:56:59, dimitern wrote:
> > Why return if equal?
> 
> Because the units we expect are deployed, and we don't need to wait any more.

Thanks, it's clearer now. It wasn't obvious for me at first recall as a term is
the opposite of deploy in this context (I guess I expected "undeploy" or
something - even though it's uglier it seems to convey the purpose better, but
recall looks better now).

[rog, 2013-01-07 14:04:21]

this is fantastic in principal, but the testing needs a bit of shuffling.

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

https://codereview.appspot.com/7028049/diff/1/cmd/jujud/deploy.go#newcode19
cmd/jujud/deploy.go:19: type fakeManager struct {
this should live in a _test.go file.
i'm not comfortable seeing purely test-related code in a normal file. we have
never so far AFAIK had a need to import gocheck in a non-test file, and i don't
think there's a need here.

how about:

var newManager = deployer.NewSimpleManager

?

then in a test file, we can do:
newManager = func(st *State, dataDir string) deployer.Manager {
    return &fakeManager {
        ...etc
    }
}

and all the other purely test-related code can go there too.

in that case, there's probably no longer any need for this file, as it's just
one small function.

https://codereview.appspot.com/7028049/diff/1/cmd/jujud/machine_test.go
File cmd/jujud/machine_test.go (right):

https://codereview.appspot.com/7028049/diff/1/cmd/jujud/machine_test.go#newco...
cmd/jujud/machine_test.go:119: go func() { c.Assert(a.Run(nil), IsNil) }()
this and the below line should probably use Check, not Assert.

[fwereade, 2013-01-07 21:48:06]

Please take a look.

[rog, 2013-01-08 16:13:08]

LGTM

https://codereview.appspot.com/7028049/diff/8001/cmd/jujud/deploy_test.go
File cmd/jujud/deploy_test.go (right):

https://codereview.appspot.com/7028049/diff/8001/cmd/jujud/deploy_test.go#new...
cmd/jujud/deploy_test.go:77: c.Assert(info.Addrs, DeepEquals, expectInfo.Addrs)
these should be Check, not Assert, as they're not called in a testing goroutine.

[fwereade, 2013-01-08 22:48:33]

*** Submitted:

jujud: principal unit agent deploys subordinates

...and the machine agent's principal-deployment is now tested.

R=dimitern, rog
CC=
https://codereview.appspot.com/7028049

https://codereview.appspot.com/7028049/diff/8001/cmd/jujud/deploy_test.go
File cmd/jujud/deploy_test.go (right):

https://codereview.appspot.com/7028049/diff/8001/cmd/jujud/deploy_test.go#new...
cmd/jujud/deploy_test.go:77: c.Assert(info.Addrs, DeepEquals, expectInfo.Addrs)
On 2013/01/08 16:13:09, rog wrote:
> these should be Check, not Assert, as they're not called in a testing
goroutine.

Done.