Code review comment for lp:~thumper/juju-core/user-display-name

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.go#oldcode37
> 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.go#newcode37
> 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_test.go
> File state/api/usermanager/client_test.go (right):

https://codereview.appspot.com/102970043/diff/1/state/api/usermanager/client_test.go#newcode42
> 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.go#oldcode39
> 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?

https://codereview.appspot.com/102970043/