cmd/juju: user detail

cmd/juju: user detail

Add a new subcommand to `juju user` which
prints out the details of the current 
user.

Add a function on the apiserver, 
CurrentUserInfo. Add a function on the api
client which calls the server side function
print out the result via the cmd.Output
formatter, allowing json and yaml outputs.

https://code.launchpad.net/~waigani/juju-core/user-detail/+merge/221314

(do not edit description out of merge proposal)

[waigani, 2014-05-29 05:04:42]

Please take a look.

[thumper, 2014-05-29 05:15:01]

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

https://codereview.appspot.com/98700043/diff/1/cmd/juju/user_detail.go#newcode20
cmd/juju/user_detail.go:20: user-name: admin
How about using examples that don't use admin?

https://codereview.appspot.com/98700043/diff/1/cmd/juju/user_detail.go#newcode28
cmd/juju/user_detail.go:28: user-name: admin
I think we should have a big TODO here somewhere that says: as we add
information to the user in the database, we will show it here.

https://codereview.appspot.com/98700043/diff/1/cmd/juju/user_detail.go#newcode47
cmd/juju/user_detail.go:47: c.out.AddFlags(f, "smart", cmd.DefaultFormatters)
I think we should have the default formatter not output "smart" yaml, but
instead be a simple output that just says the name:

$ juju user detail
admin

$ juju user detail --format json
{"user-name": "admin"}

https://codereview.appspot.com/98700043/diff/1/cmd/juju/user_detail.go#newcode60
cmd/juju/user_detail.go:60: findResult, err := client.CurrentUserInfo()
This isn't handling the case where we have a user specified on the command line.

We should be saying:
  client.UserInfo(username)

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

https://codereview.appspot.com/98700043/diff/1/state/api/client.go#newcode861
state/api/client.go:861: func (c *Client) CurrentUserInfo() (result
params.CurrentUserInfoResults, err error) {
As mentioned elsewhere, this is the wrong name...

[fwereade, 2014-05-29 07:29:17]

This does not LGTM as it stands. Along with the issues with the API itself,
there are no tests for anything api-related. I would strongly suggest writing a
CL that *just* adds the API, and to worry about the CLI once that's solid.

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

https://codereview.appspot.com/98700043/diff/1/cmd/juju/user_detail.go#newcode14
cmd/juju/user_detail.go:14: const userDetailCommandDoc = `
FWIW, it might be worthwhile separating the intended future docs from the
in-command docs -- as it is, while it doesn't yet handle username, this is just
inaccurate.

https://codereview.appspot.com/98700043/diff/1/cmd/juju/user_detail.go#newcode28
cmd/juju/user_detail.go:28: user-name: admin
On 2014/05/29 05:15:01, thumper wrote:
> I think we should have a big TODO here somewhere that says: as we add
> information to the user in the database, we will show it here.

This does feed into how we want to write the smart formatter. Just outputting
"admin" (or "thumper") is going to become more unhelpful as we get more fields,
especially as those fields' meaning becomes harder to infer from content alone.

https://codereview.appspot.com/98700043/diff/1/cmd/juju/user_detail.go#newcode47
cmd/juju/user_detail.go:47: c.out.AddFlags(f, "smart", cmd.DefaultFormatters)
On 2014/05/29 05:15:01, thumper wrote:
> I think we should have the default formatter not output "smart" yaml, but
> instead be a simple output that just says the name:
> 
> $ juju user detail
> admin
> 
> $ juju user detail --format json
> {"user-name": "admin"}
> 

FWIW, I don't think "smart" format has to be entirely consistent across
commands. My conception of it has always been "generate nicely human-readable
output (with an option on easy handling in bash)" -- if you want
easily-parseable output, that's what json/yaml are for.

Writing an output-type-specific "smart" formatter for each command seems like a
pretty low-friction way to go, tbh; and indeed *sometimes* the default smart
formatter will be perfectly adequate (eg, it would be pretty nice for just
printing out lists of api endpoints one-per-line)

https://codereview.appspot.com/98700043/diff/1/cmd/juju/user_detail.go#newcode60
cmd/juju/user_detail.go:60: findResult, err := client.CurrentUserInfo()
On 2014/05/29 05:15:01, thumper wrote:
> This isn't handling the case where we have a user specified on the command
line.
> 
> We should be saying:
>   client.UserInfo(username)
> 

...and we should be figuring out the current user to begin with, and asking for
UserInfo by name in all cases anyway.

You're writing bulk API calls, right? I don't see how CurrentUserInfo is
amenable to that... if you want a magic value that means "the logged-in user" I
could *maybe* live with that, but really, whatever makes the connection ought to
know on whose behalf it's made the connection in the first place, and should ask
explicitly for that user's info.

(maybe you handle this elsewhere, I haven't read the whole CL yet, but I'm not
even convinced that the cost/benefit of CurrentUserInfo really helps much even
as a pure client-side convenience method)

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

https://codereview.appspot.com/98700043/diff/1/state/api/client.go#newcode862
state/api/client.go:862: err = c.call("CurrentUserInfo", nil, &result)
And non-bulk calls are not acceptable anyway. I have a major grump going on at
the moment because someone implemented StateServers() (or something like that as
non-bulk, zero-args) and thereby makes it impossible to report different
addresses for different clients, or to ask for those potentially-differing
addresses for more than one entity at once.

https://codereview.appspot.com/98700043/diff/1/state/api/params/internal.go
File state/api/params/internal.go (right):

https://codereview.appspot.com/98700043/diff/1/state/api/params/internal.go#n...
state/api/params/internal.go:609: ExcludeNetworks []string
GAAH. I was going to look at this to check I got the json-marshalling info right
for the comment below, and I find it's using its own conventions. These things
should be marshalled as "include-networks" etc.

Not your fault, just ranting generally.

https://codereview.appspot.com/98700043/diff/1/state/api/params/internal.go#n...
state/api/params/internal.go:625: Info  map[string]interface{}
Please use an actual type here, and do the whole bulk thing.

type UserInfo struct {
    Name string `json:"name,omitempty"`
}

type UserInfoResult struct {
    Result *UserInfo `json:"result,omitempty"`
    Error *Error     `json:"error,omitempty"`
}

type UserInfoResults struct {
    Results []UserInfoResult `json:"results,omitempty"`
}

And please include marshalling information, with explicit names, not just the
ehh-probably-right ",omitempty" style -- without it, we can't even tweak field
names for sanity in the code.

https://codereview.appspot.com/98700043/diff/1/state/apiserver/client/client.go
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/98700043/diff/1/state/apiserver/client/client....
state/apiserver/client/client.go:1105: currentEnv :=
envcmd.ReadCurrentEnvironment()
This is completely wrong. envcmd is about figuring out the current environment
on a *client* machine -- on a state server, there will be no current environment
to read, no configstore, no user credentials, no nothing.

Pass the usernames you want in the args, look them up, return their info. It
seems stupid now while the username is the only info that comes back, and that
therefore doesn't even need you to connect -- but this is just the scaffolding.

https://codereview.appspot.com/98700043/diff/1/state/apiserver/client/client....
state/apiserver/client/client.go:1130: // TODO(waigani) add:
To reiterate: make a struct, and define the marshalling in the struct.