various: set passwords in state at bootstrap time.

various: set passwords in state at bootstrap time.



https://code.launchpad.net/~rogpeppe/juju-core/113-use-passwords/+merge/128752

Requires: https://code.launchpad.net/~rogpeppe/juju-core/114-testing-logged-in-session/+merge/128718

(do not edit description out of merge proposal)

[rog, 2012-10-09 16:46:13]

Please take a look.

[niemeyer, 2012-10-09 22:35:45]

Very nice. LGTM, with simple stuff.

https://codereview.appspot.com/6632049/diff/5001/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/6632049/diff/5001/environs/dummy/environs.go#n...
environs/dummy/environs.go:404: return fmt.Errorf("cannot initialize state: %v",
err)
This error message should be within Initialize, as usual for the state package.

https://codereview.appspot.com/6632049/diff/5001/environs/dummy/environs.go#n...
environs/dummy/environs.go:408: return fmt.Errorf("cannot set admin password:
%v", err)
return err

We have exactly the same error message within SetAdminPassword itself, and in
better shape in fact given that in some case it does set the password despite
the error.

https://codereview.appspot.com/6632049/diff/5001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6632049/diff/5001/environs/ec2/ec2.go#newcode241
environs/ec2/ec2.go:241: Password: password,
FWIW, single line is rather pleasant in those cases:

info := &state.Info{Password: password}

https://codereview.appspot.com/6632049/diff/5001/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/6632049/diff/5001/environs/interface.go#newcod...
environs/interface.go:131: // and should be changed at the first opportunity.
// ... (see trivial.PasswordHash).
// When first connecting to the environment via the juju package, the
// password hash will be automatically replaced by the real password.

https://codereview.appspot.com/6632049/diff/5001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/6632049/diff/5001/juju/conn.go#newcode30
juju/conn.go:30: // perhaps we are the first connection and the
s/we are/this was/

https://codereview.appspot.com/6632049/diff/5001/juju/conn.go#newcode39
juju/conn.go:39: }
This turned out rather nice.

https://codereview.appspot.com/6632049/diff/5001/juju/conn_test.go
File juju/conn_test.go (right):

https://codereview.appspot.com/6632049/diff/5001/juju/conn_test.go#newcode135
juju/conn_test.go:135: func (cs *ConnSuite) TestConnWithPassword(c *C) {
Awesome test.

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

https://codereview.appspot.com/6632049/diff/5001/state/state.go#newcode548
state/state.go:548: return fmt.Errorf("cannot add admin user: %v", err)
The original message was correct. The method caller didn't ask to add a user,
and it will be called even when the user already exists.

https://codereview.appspot.com/6632049/diff/5001/state/state.go#newcode555
state/state.go:555: return fmt.Errorf("cannot remove admin user: %v", err)
And it sounds like this should actually say:

"cannot disable admin password: %v"

[rog, 2012-10-11 10:51:13]

Please take a look.

https://codereview.appspot.com/6632049/diff/5001/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/6632049/diff/5001/environs/dummy/environs.go#n...
environs/dummy/environs.go:404: return fmt.Errorf("cannot initialize state: %v",
err)
On 2012/10/09 22:35:45, niemeyer wrote:
> This error message should be within Initialize, as usual for the state
package.

If I do that, then Initialize and Open won't be able to return a bare
state.ErrUnauthorized. I could change that error to be an error type that
includes another error if you like. Or Initialize could be different and include
the prefix, where Open doesn't.

https://codereview.appspot.com/6632049/diff/5001/environs/dummy/environs.go#n...
environs/dummy/environs.go:408: return fmt.Errorf("cannot set admin password:
%v", err)
On 2012/10/09 22:35:45, niemeyer wrote:
> return err
> 
> We have exactly the same error message within SetAdminPassword itself, and in
> better shape in fact given that in some case it does set the password despite
> the error.

Done.

https://codereview.appspot.com/6632049/diff/5001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6632049/diff/5001/environs/ec2/ec2.go#newcode241
environs/ec2/ec2.go:241: Password: password,
On 2012/10/09 22:35:45, niemeyer wrote:
> FWIW, single line is rather pleasant in those cases:
> 
> info := &state.Info{Password: password}

Done.

https://codereview.appspot.com/6632049/diff/5001/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/6632049/diff/5001/environs/interface.go#newcod...
environs/interface.go:131: // and should be changed at the first opportunity.
On 2012/10/09 22:35:45, niemeyer wrote:
> // ... (see trivial.PasswordHash).
> // When first connecting to the environment via the juju package, the
> // password hash will be automatically replaced by the real password.

Done.

https://codereview.appspot.com/6632049/diff/5001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/6632049/diff/5001/juju/conn.go#newcode30
juju/conn.go:30: // perhaps we are the first connection and the
On 2012/10/09 22:35:45, niemeyer wrote:
> s/we are/this was/

Done.

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

https://codereview.appspot.com/6632049/diff/5001/state/state.go#newcode548
state/state.go:548: return fmt.Errorf("cannot add admin user: %v", err)
On 2012/10/09 22:35:45, niemeyer wrote:
> The original message was correct. The method caller didn't ask to add a user,
> and it will be called even when the user already exists.

Done.

https://codereview.appspot.com/6632049/diff/5001/state/state.go#newcode555
state/state.go:555: return fmt.Errorf("cannot remove admin user: %v", err)
On 2012/10/09 22:35:45, niemeyer wrote:
> And it sounds like this should actually say:
> 
> "cannot disable admin password: %v"

Done.

[rog, 2012-10-11 11:00:22]

*** Submitted:

various: set passwords in state at bootstrap time.

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

https://codereview.appspot.com/6632049/diff/5001/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/6632049/diff/5001/environs/dummy/environs.go#n...
environs/dummy/environs.go:404: return fmt.Errorf("cannot initialize state: %v",
err)
On 2012/10/11 10:51:13, rog wrote:
> On 2012/10/09 22:35:45, niemeyer wrote:
> > This error message should be within Initialize, as usual for the state
> package.
> 
> If I do that, then Initialize and Open won't be able to return a bare
> state.ErrUnauthorized. I could change that error to be an error type that
> includes another error if you like. Or Initialize could be different and
include
> the prefix, where Open doesn't.

I've reverted to panicing as before to get this CL submitted.