environs/cloudinit: use --initial-password

environs/cloudinit: use --initial-password



https://code.launchpad.net/~rogpeppe/juju-core/105-cloudinit-initial-password/+merge/128222

Requires: https://code.launchpad.net/~rogpeppe/juju-core/107-jujud-bootstrap-initial-password/+merge/128210

(do not edit description out of merge proposal)

[rog, 2012-10-05 11:04:48]

Please take a look.

[TheMue, 2012-10-05 11:15:50]

LGTM

[niemeyer, 2012-10-05 15:43:41]

One minor:

https://codereview.appspot.com/6612054/diff/2001/environs/cloudinit/cloudinit.go
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/6612054/diff/2001/environs/cloudinit/cloudinit...
environs/cloudinit/cloudinit.go:41: // The entity name will be ignored.
Rather than ignoring it, we can make pretty good use of it.

Let's validate that when we're running bootstrap-state we've got an empty
EntityName, and when we're not running it, we've got one that matches the entity
name of the machine being started. If those fail, we can error out and complain
that the provider is mis-implemented.

[rog, 2012-10-05 16:40:12]

https://codereview.appspot.com/6612054/diff/2001/environs/cloudinit/cloudinit.go
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/6612054/diff/2001/environs/cloudinit/cloudinit...
environs/cloudinit/cloudinit.go:41: // The entity name will be ignored.
On 2012/10/05 15:43:41, niemeyer wrote:
> Rather than ignoring it, we can make pretty good use of it.
> 
> Let's validate that when we're running bootstrap-state we've got an empty
> EntityName, and when we're not running it, we've got one that matches the
entity
> name of the machine being started. If those fail, we can error out and
complain
> that the provider is mis-implemented.

ok. it's slightly strange that when running bootstrap-state, the password is
actually for the admin *and* the machine entity, but seems reasonable. do you
think it's ok to assume the machine entity name format is well defined, or
should we pass in a Machine here instead of a machine id?

[niemeyer, 2012-10-05 17:13:03]

https://codereview.appspot.com/6612054/diff/2001/environs/cloudinit/cloudinit.go
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/6612054/diff/2001/environs/cloudinit/cloudinit...
environs/cloudinit/cloudinit.go:41: // The entity name will be ignored.
On 2012/10/05 16:40:12, rog wrote:
> On 2012/10/05 15:43:41, niemeyer wrote:
> > Rather than ignoring it, we can make pretty good use of it.
> > 
> > Let's validate that when we're running bootstrap-state we've got an empty
> > EntityName, and when we're not running it, we've got one that matches the
> entity
> > name of the machine being started. If those fail, we can error out and
> complain
> > that the provider is mis-implemented.
> 
> ok. it's slightly strange that when running bootstrap-state, the password is
> actually for the admin *and* the machine entity, but seems reasonable. do you
> think it's ok to assume the machine entity name format is well defined, or
> should we pass in a Machine here instead of a machine id?

if cfg.EntityName != "machine-" + strconv.Itoa(cfg.MachineId) {
    return fmt.Errorf("invalid machine configuration: need authentication for
machine %d", cfg.MachineId)
}

Let's not spend too much time on something that trivial.

[rog, 2012-10-05 17:16:53]

Please take a look.

[rog, 2012-10-05 17:34:13]

Please take a look.

[rog, 2012-10-05 17:37:55]

https://codereview.appspot.com/6612054/diff/2001/environs/cloudinit/cloudinit.go
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/6612054/diff/2001/environs/cloudinit/cloudinit...
environs/cloudinit/cloudinit.go:41: // The entity name will be ignored.
On 2012/10/05 17:13:04, niemeyer wrote:
> On 2012/10/05 16:40:12, rog wrote:
> > On 2012/10/05 15:43:41, niemeyer wrote:
> > > Rather than ignoring it, we can make pretty good use of it.
> > > 
> > > Let's validate that when we're running bootstrap-state we've got an empty
> > > EntityName, and when we're not running it, we've got one that matches the
> > entity
> > > name of the machine being started. If those fail, we can error out and
> > complain
> > > that the provider is mis-implemented.
> > 
> > ok. it's slightly strange that when running bootstrap-state, the password is
> > actually for the admin *and* the machine entity, but seems reasonable. do
you
> > think it's ok to assume the machine entity name format is well defined, or
> > should we pass in a Machine here instead of a machine id?
> 
> if cfg.EntityName != "machine-" + strconv.Itoa(cfg.MachineId) {
>     return fmt.Errorf("invalid machine configuration: need authentication for
> machine %d", cfg.MachineId)
> }
> 
> Let's not spend too much time on something that trivial.

Done.

[niemeyer, 2012-10-05 17:39:18]

LGTM

https://codereview.appspot.com/6612054/diff/7002/environs/jujutest/test.go
File environs/jujutest/test.go (right):

https://codereview.appspot.com/6612054/diff/7002/environs/jujutest/test.go#ne...
environs/jujutest/test.go:18: EntityName: fmt.Sprintf("machine-%d", machineId),
Password: "unimportant",

[rog, 2012-10-08 14:30:53]

Please take a look.

[rog, 2012-10-08 15:26:23]

Please take a look.

https://codereview.appspot.com/6612054/diff/7002/environs/jujutest/test.go
File environs/jujutest/test.go (right):

https://codereview.appspot.com/6612054/diff/7002/environs/jujutest/test.go#ne...
environs/jujutest/test.go:18: EntityName: fmt.Sprintf("machine-%d", machineId),
On 2012/10/05 17:39:18, niemeyer wrote:
> Password: "unimportant",

Done. But I've moved this function into juju/testing, because it's useful
elsewhere now.

[niemeyer, 2012-10-08 16:17:09]

LGTM

https://codereview.appspot.com/6612054/diff/20005/juju/testing/conn.go
File juju/testing/conn.go (right):

https://codereview.appspot.com/6612054/diff/20005/juju/testing/conn.go#newcode42
juju/testing/conn.go:42: // machine id of the machine to be started
Please reindent.

[rog, 2012-10-08 16:17:24]

Please take a look.

[rog, 2012-10-08 16:19:34]

*** Submitted:

environs/cloudinit: use --initial-password

R=TheMue, niemeyer
CC=
https://codereview.appspot.com/6612054