various: use agent package, lose agent flags

various: use agent package, lose agent flags

Unfortunate circular dependencies mean this
comes in one big lump.

https://code.launchpad.net/~rogpeppe/juju-core/180-jujud-lose-flags/+merge/142938

(do not edit description out of merge proposal)

[rog, 2013-01-11 16:56:54]

Please take a look.

[dimitern, 2013-01-11 17:27:24]

On 2013/01/11 16:56:54, rog wrote:
> Please take a look.

LGTM with a few trivial suggestions and questions.

[dimitern, 2013-01-11 17:27:56]

LGTM with these..

https://codereview.appspot.com/7102043/diff/5001/cmd/jujud/bootstrap_test.go
File cmd/jujud/bootstrap_test.go (right):

https://codereview.appspot.com/7102043/diff/5001/cmd/jujud/bootstrap_test.go#...
cmd/jujud/bootstrap_test.go:44: func (s *BootstrapSuite) initBootstrapCommand(c
*C, args ...string) (*agent.Conf, *BootstrapCommand, error) {
Nice to use varargs instead of []string!

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

https://codereview.appspot.com/7102043/diff/5001/cmd/jujud/deploy_test.go#new...
cmd/jujud/deploy_test.go:82: e0 := *expectInfo
What's the purpose of this?

https://codereview.appspot.com/7102043/diff/5001/environs/cloudinit/cloudinit.go
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/7102043/diff/5001/environs/cloudinit/cloudinit...
environs/cloudinit/cloudinit.go:205: acfg := cfg.agentConfig(entityName)
Why don't you replace these few lines with a call to addAgentInfo() above? It
seems like unnecessary duplication.

https://codereview.appspot.com/7102043/diff/5001/environs/cloudinit/cloudinit...
File environs/cloudinit/cloudinit_test.go (right):

https://codereview.appspot.com/7102043/diff/5001/environs/cloudinit/cloudinit...
environs/cloudinit/cloudinit_test.go:279: cfg.StateInfo = &state.Info{Addrs:
[]string{"host:98765"}}
Maybe adding a constant MONGODB_HOST = "host:98765" will make it better?

https://codereview.appspot.com/7102043/diff/5001/environs/cloudinit/cloudinit...
environs/cloudinit/cloudinit_test.go:285: Addrs:      []string{"host:98765"},
ditto

https://codereview.appspot.com/7102043/diff/5001/environs/cloudinit/cloudinit...
environs/cloudinit/cloudinit_test.go:344: Addrs:  []string{"host:98765"},
ditto

[fwereade, 2013-01-11 17:50:37]

On 2013/01/11 17:27:56, dimitern wrote:
> LGTM with these..
> 
> https://codereview.appspot.com/7102043/diff/5001/cmd/jujud/bootstrap_test.go
> File cmd/jujud/bootstrap_test.go (right):
> 
>
https://codereview.appspot.com/7102043/diff/5001/cmd/jujud/bootstrap_test.go#...
> cmd/jujud/bootstrap_test.go:44: func (s *BootstrapSuite)
initBootstrapCommand(c
> *C, args ...string) (*agent.Conf, *BootstrapCommand, error) {
> Nice to use varargs instead of []string!
> 
> https://codereview.appspot.com/7102043/diff/5001/cmd/jujud/deploy_test.go
> File cmd/jujud/deploy_test.go (right):
> 
>
https://codereview.appspot.com/7102043/diff/5001/cmd/jujud/deploy_test.go#new...
> cmd/jujud/deploy_test.go:82: e0 := *expectInfo
> What's the purpose of this?
> 
>
https://codereview.appspot.com/7102043/diff/5001/environs/cloudinit/cloudinit.go
> File environs/cloudinit/cloudinit.go (right):
> 
>
https://codereview.appspot.com/7102043/diff/5001/environs/cloudinit/cloudinit...
> environs/cloudinit/cloudinit.go:205: acfg := cfg.agentConfig(entityName)
> Why don't you replace these few lines with a call to addAgentInfo() above? It
> seems like unnecessary duplication.
> 
>
https://codereview.appspot.com/7102043/diff/5001/environs/cloudinit/cloudinit...
> File environs/cloudinit/cloudinit_test.go (right):
> 
>
https://codereview.appspot.com/7102043/diff/5001/environs/cloudinit/cloudinit...
> environs/cloudinit/cloudinit_test.go:279: cfg.StateInfo = &state.Info{Addrs:
> []string{"host:98765"}}
> Maybe adding a constant MONGODB_HOST = "host:98765" will make it better?
> 
>
https://codereview.appspot.com/7102043/diff/5001/environs/cloudinit/cloudinit...
> environs/cloudinit/cloudinit_test.go:285: Addrs:      []string{"host:98765"},
> ditto
> 
>
https://codereview.appspot.com/7102043/diff/5001/environs/cloudinit/cloudinit...
> environs/cloudinit/cloudinit_test.go:344: Addrs:  []string{"host:98765"},
> ditto

Just a couple of trivials; otherwise LGTM. Very nice.

[fwereade, 2013-01-11 18:01:26]

gaah, sorry

https://codereview.appspot.com/7102043/diff/5001/environs/cloudinit/cloudinit.go
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/7102043/diff/5001/environs/cloudinit/cloudinit...
environs/cloudinit/cloudinit.go:205: acfg := cfg.agentConfig(entityName)
On 2013/01/11 17:27:56, dimitern wrote:
> Why don't you replace these few lines with a call to addAgentInfo() above? It
> seems like unnecessary duplication.

+1

https://codereview.appspot.com/7102043/diff/5001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/7102043/diff/5001/state/state.go#newcode696
state/state.go:696: log.Printf("set password %q = %q", name, password)
badges on log messages?

[rog, 2013-01-11 18:18:37]

*** Submitted:

various: use agent package, lose agent flags

Unfortunate circular dependencies mean this
comes in one big lump.

R=dimitern, fwereade
CC=
https://codereview.appspot.com/7102043

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

https://codereview.appspot.com/7102043/diff/5001/cmd/jujud/deploy_test.go#new...
cmd/jujud/deploy_test.go:82: e0 := *expectInfo
On 2013/01/11 17:27:56, dimitern wrote:
> What's the purpose of this?

it means we don't care how expectInfo is changed after the call to this
function, because we're making a copy of it.

https://codereview.appspot.com/7102043/diff/5001/environs/cloudinit/cloudinit.go
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/7102043/diff/5001/environs/cloudinit/cloudinit...
environs/cloudinit/cloudinit.go:205: acfg := cfg.agentConfig(entityName)
On 2013/01/11 17:27:56, dimitern wrote:
> Why don't you replace these few lines with a call to addAgentInfo() above? It
> seems like unnecessary duplication.

good call.

https://codereview.appspot.com/7102043/diff/5001/environs/cloudinit/cloudinit...
File environs/cloudinit/cloudinit_test.go (right):

https://codereview.appspot.com/7102043/diff/5001/environs/cloudinit/cloudinit...
environs/cloudinit/cloudinit_test.go:279: cfg.StateInfo = &state.Info{Addrs:
[]string{"host:98765"}}
On 2013/01/11 17:27:56, dimitern wrote:
> Maybe adding a constant MONGODB_HOST = "host:98765" will make it better?

i'm not sure that helps - it's longer, and the exact value is irrelevant - they
could all contain different values and it wouldn't matter.

https://codereview.appspot.com/7102043/diff/5001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/7102043/diff/5001/state/state.go#newcode696
state/state.go:696: log.Printf("set password %q = %q", name, password)
On 2013/01/11 18:01:26, fwereade wrote:
> badges on log messages?

i thought i'd deleted that already!