Add a display name option for new users

Add a display name option for new users

This ended up being more work than I hoped as I refactored the apiserver
parts, many tests, and made sure there was backwards compatability with
older client apis.

Also changed how the CLI command was tested so it didn't need a
JujuConnSuite.

https://code.launchpad.net/~thumper/juju-core/user-display-name/+merge/221823

(do not edit description out of merge proposal)

[thumper, 2014-06-03 04:31:15]

Please take a look.

[menn0, 2014-06-03 05:17:26]

LGTM - only minor comments and praise added in-line. The user add tests look
much better.

One thing: can this actually land without schema migration support?

https://codereview.appspot.com/102970043/diff/1/cmd/juju/user_add.go
File cmd/juju/user_add.go (left):

https://codereview.appspot.com/102970043/diff/1/cmd/juju/user_add.go#oldcode44
cmd/juju/user_add.go:44: Args:    "<username> <password>",
Sorry for missing this when I implemented --password

https://codereview.appspot.com/102970043/diff/1/cmd/juju/user_add.go
File cmd/juju/user_add.go (right):

https://codereview.appspot.com/102970043/diff/1/cmd/juju/user_add.go#newcode109
cmd/juju/user_add.go:109: return err
errors.Trace() here too?

https://codereview.appspot.com/102970043/diff/1/state/api/usermanager/client.go
File state/api/usermanager/client.go (left):

https://codereview.appspot.com/102970043/diff/1/state/api/usermanager/client....
state/api/usermanager/client.go:37: p := params.EntityPasswords{Changes:
[]params.EntityPassword{u}}
These names were terrible! Glad you've fixed it.

https://codereview.appspot.com/102970043/diff/1/state/api/usermanager/client.go
File state/api/usermanager/client.go (right):

https://codereview.appspot.com/102970043/diff/1/state/api/usermanager/client....
state/api/usermanager/client.go:37: Changes: []params.ModifyUser{{Username:
username, DisplayName: displayName, Password: password}},
This is much more readable.

https://codereview.appspot.com/102970043/diff/1/state/api/usermanager/client_...
File state/api/usermanager/client_test.go (right):

https://codereview.appspot.com/102970043/diff/1/state/api/usermanager/client_...
state/api/usermanager/client_test.go:42: err := s.APIState.Call("UserManager",
"", "AddUser", userArgs, results)
Maybe add a comment here about why s.APIState.Call("UserManager",...) is used
here instead of s.usermanager.AddUser()? I understand why but had to think about
it for a bit.

https://codereview.appspot.com/102970043/diff/1/state/apiserver/charms_test.go
File state/apiserver/charms_test.go (left):

https://codereview.appspot.com/102970043/diff/1/state/apiserver/charms_test.g...
state/apiserver/charms_test.go:39: password, err := utils.RandomPassword()
Play devil's advocate: this change means the user has a fixed password instead
of a random one. Are you sure that doesn't negatively impact any existing tests?
I can't think of why it would but it is a test functionality change.

https://codereview.appspot.com/102970043/diff/1/state/user_test.go
File state/user_test.go (right):

https://codereview.appspot.com/102970043/diff/1/state/user_test.go#newcode45
state/user_test.go:45: func (s *UserSuite) makeUser(c *gc.C) *state.User {
addSomeUser might be a better name, indicating that a test/throwaway user is
being created. It also hints at the username being used.

[waigani, 2014-06-03 21:29:13]

On 2014/06/03 05:17:26, menn0 wrote:
> LGTM - only minor comments and praise added in-line. The user add tests look
> much better.
> 
> One thing: can this actually land without schema migration support?
> 
> https://codereview.appspot.com/102970043/diff/1/cmd/juju/user_add.go
> File cmd/juju/user_add.go (left):
> 
> https://codereview.appspot.com/102970043/diff/1/cmd/juju/user_add.go#oldcode44
> cmd/juju/user_add.go:44: Args:    "<username> <password>",
> Sorry for missing this when I implemented --password
> 
> https://codereview.appspot.com/102970043/diff/1/cmd/juju/user_add.go
> File cmd/juju/user_add.go (right):
> 
>
https://codereview.appspot.com/102970043/diff/1/cmd/juju/user_add.go#newcode109
> cmd/juju/user_add.go:109: return err
> errors.Trace() here too?
> 
>
https://codereview.appspot.com/102970043/diff/1/state/api/usermanager/client.go
> File state/api/usermanager/client.go (left):
> 
>
https://codereview.appspot.com/102970043/diff/1/state/api/usermanager/client....
> state/api/usermanager/client.go:37: p := params.EntityPasswords{Changes:
> []params.EntityPassword{u}}
> These names were terrible! Glad you've fixed it.
> 
>
https://codereview.appspot.com/102970043/diff/1/state/api/usermanager/client.go
> File state/api/usermanager/client.go (right):
> 
>
https://codereview.appspot.com/102970043/diff/1/state/api/usermanager/client....
> state/api/usermanager/client.go:37: Changes: []params.ModifyUser{{Username:
> username, DisplayName: displayName, Password: password}},
> This is much more readable.
> 
>
https://codereview.appspot.com/102970043/diff/1/state/api/usermanager/client_...
> File state/api/usermanager/client_test.go (right):
> 
>
https://codereview.appspot.com/102970043/diff/1/state/api/usermanager/client_...
> state/api/usermanager/client_test.go:42: err := s.APIState.Call("UserManager",
> "", "AddUser", userArgs, results)
> Maybe add a comment here about why s.APIState.Call("UserManager",...) is used
> here instead of s.usermanager.AddUser()? I understand why but had to think
about
> it for a bit.
> 
> https://codereview.appspot.com/102970043/diff/1/state/apiserver/charms_test.go
> File state/apiserver/charms_test.go (left):
> 
>
https://codereview.appspot.com/102970043/diff/1/state/apiserver/charms_test.g...
> state/apiserver/charms_test.go:39: password, err := utils.RandomPassword()
> Play devil's advocate: this change means the user has a fixed password instead
> of a random one. Are you sure that doesn't negatively impact any existing
tests?
> I can't think of why it would but it is a test functionality change.
> 
> https://codereview.appspot.com/102970043/diff/1/state/user_test.go
> File state/user_test.go (right):
> 
> https://codereview.appspot.com/102970043/diff/1/state/user_test.go#newcode45
> state/user_test.go:45: func (s *UserSuite) makeUser(c *gc.C) *state.User {
> addSomeUser might be a better name, indicating that a test/throwaway user is
> being created. It also hints at the username being used.

LGTM. Just one suggestion inline. The bulk of the changes look to be updating
AddUser to take an extra arg. As to schema migration support, I think adding
fields is fine - its when we change or remove them that things get tricky.  It's
good to see juju/errors package being used.

At what point will Tag be able to be dropped? Will it be the next release of
Juju that break backwards compatibility or will the API have its own versioning?

[waigani, 2014-06-03 21:29:27]

https://codereview.appspot.com/102970043/diff/1/state/user_test.go
File state/user_test.go (right):

https://codereview.appspot.com/102970043/diff/1/state/user_test.go#newcode75
state/user_test.go:75: user := s.makeUser(c)
It seems you are making a user for every test. Why not makeUser in SetUpTest?

[thumper, 2014-06-04 04:01:23]

New branch is moving to github.

https://codereview.appspot.com/102970043/diff/1/state/api/usermanager/client_...
File state/api/usermanager/client_test.go (right):

https://codereview.appspot.com/102970043/diff/1/state/api/usermanager/client_...
state/api/usermanager/client_test.go:42: err := s.APIState.Call("UserManager",
"", "AddUser", userArgs, results)
On 2014/06/03 05:17:26, menn0 wrote:
> Maybe add a comment here about why s.APIState.Call("UserManager",...) is used
> here instead of s.usermanager.AddUser()? I understand why but had to think
about
> it for a bit.

Done.

https://codereview.appspot.com/102970043/diff/1/state/apiserver/charms_test.go
File state/apiserver/charms_test.go (left):

https://codereview.appspot.com/102970043/diff/1/state/apiserver/charms_test.g...
state/apiserver/charms_test.go:39: password, err := utils.RandomPassword()
On 2014/06/03 05:17:26, menn0 wrote:
> Play devil's advocate: this change means the user has a fixed password instead
> of a random one. Are you sure that doesn't negatively impact any existing
tests?
> I can't think of why it would but it is a test functionality change.

Sure, but the tests all still pass :-)

I'm considering a way to change the way we create test fixture objects.  A
cunning plan.

https://codereview.appspot.com/102970043/diff/1/state/user_test.go
File state/user_test.go (right):

https://codereview.appspot.com/102970043/diff/1/state/user_test.go#newcode75
state/user_test.go:75: user := s.makeUser(c)
On 2014/06/03 21:29:26, waigani wrote:
> It seems you are making a user for every test. Why not makeUser in SetUpTest?

Not quite every test :-)