Preliminary support for basic juju id commands:

Preliminary support for basic juju id commands:

juju add-user <username> <password>

if password is not supplied uses gopass to take the password from the command line
	
juju remove-user <username>

doesn't actually remove the user. But mark them as inactive

add-user and remove-user aren't registered yet as they are useless without
login. But the details of this are still being worked out.

This branch also contains fixes for lp:1285256.
The EnvCommandBase struct includes a function for ensuring the envname is not "".

This replaces a review at https://codereview.appspot.com/51450047/

https://code.launchpad.net/~mattyw/juju-core/user-add-remove-cli/+merge/210962

(do not edit description out of merge proposal)

[mattyw, 2014-03-14 05:17:51]

Please take a look.

[mattyw, 2014-03-14 11:34:35]

On 2014/03/14 05:17:51, mattyw wrote:
> Please take a look.

This follows on from the review here:  https://codereview.appspot.com/51450047/

[fwereade, 2014-03-18 01:11:39]

Sorry, I don't think we're quite done here yet.

I am very happy with the EnvCommandBase stuff, though, so I'd swiftly approve a
CL that just landed those changes while we argue about the UI/UX for user
management.

https://codereview.appspot.com/75600044/diff/20001/cmd/juju/addmachine.go
File cmd/juju/addmachine.go (right):

https://codereview.appspot.com/75600044/diff/20001/cmd/juju/addmachine.go#new...
cmd/juju/addmachine.go:60: envcmd.EnvCommandBase
This has been scratching at the back of my mind for a while... "Base" isn't
quite the right term. "EnvCommand" -- or, hey, why not "EnvironmentCommand"? --
might actually be a bit clearer, but then it's not really itself a *command* in
itself. Bah.

Nothing necessarily actionable here because I can't actually suggest anything
better, but I'd love it if we did come up with a better name.

https://codereview.appspot.com/75600044/diff/20001/cmd/juju/adduser.go
File cmd/juju/adduser.go (right):

https://codereview.appspot.com/75600044/diff/20001/cmd/juju/adduser.go#newcode59
cmd/juju/adduser.go:59: c.Password = args[1]
I'm still not really in love with allowing people to specify passwords
(especially in the absence of resetting post-distribution).

https://codereview.appspot.com/75600044/diff/20001/cmd/juju/adduser.go#newcode70
cmd/juju/adduser.go:70: return client.AddUser(c.User, c.Password)
Any possibility we could design this to output a .jenv (*without* all the
bootstrap-config stuff) on stdout? I think that the pass-jenvs-around model is
pretty tolerable, really.

https://codereview.appspot.com/75600044/diff/20001/cmd/juju/adduser_test.go
File cmd/juju/adduser_test.go (right):

https://codereview.appspot.com/75600044/diff/20001/cmd/juju/adduser_test.go#n...
cmd/juju/adduser_test.go:21: func (s *AddUserSuite) Testadduser(c *gc.C) {
TestAddUser, please

https://codereview.appspot.com/75600044/diff/20001/cmd/juju/adduser_test.go#n...
cmd/juju/adduser_test.go:27: c.Assert(err, gc.ErrorMatches, "Failed to create
user: user already exists")
we don't seem to have tests for the automatic password generation...

https://codereview.appspot.com/75600044/diff/20001/cmd/juju/removeuser_test.go
File cmd/juju/removeuser_test.go (right):

https://codereview.appspot.com/75600044/diff/20001/cmd/juju/removeuser_test.g...
cmd/juju/removeuser_test.go:19: func (s *RemoveUserSuite) Testremoveuser(c
*gc.C) {
TestRemoveUser

https://codereview.appspot.com/75600044/diff/20001/cmd/juju/removeuser_test.g...
cmd/juju/removeuser_test.go:24: c.Assert(err, gc.IsNil)
no tests for CheckEmpty etc (same applies to AddUser as well fwiw)

https://codereview.appspot.com/75600044/diff/20001/cmd/plugins/juju-metadata/...
File cmd/plugins/juju-metadata/validatetoolsmetadata.go (right):

https://codereview.appspot.com/75600044/diff/20001/cmd/plugins/juju-metadata/...
cmd/plugins/juju-metadata/validatetoolsmetadata.go:24: envcmd.EnvCommandBase
Hmm. Shouldn't these be using Ensure..?

https://codereview.appspot.com/75600044/diff/20001/errors/errors.go
File errors/errors.go (right):

https://codereview.appspot.com/75600044/diff/20001/errors/errors.go#newcode98
errors/errors.go:98: return fmt.Sprintf("rc: %v", e.Code)
If we're touching this can we please do something about the overwhelmingly
shitty UX of being told "rc: 2", which means literally nothing to most
semi-normal humans? just "subprocess exited with code 2" or something... it'd be
way better if we said *which* one but that's way out of scope.

https://codereview.appspot.com/75600044/diff/20001/errors/errors.go#newcode124
errors/errors.go:124: // return code from the cmd.Main function rather than the
default of 1 if
and this comment in particular makes me wonder whether it's right to have moved
it. I think it *is* but it's a conceptual boundary violation to mention cmd.Main
in this context -- can we maybe just give it a generic name like "ExitCodeError"
and keep the special handling details in cmd.Main?

Scope issues again, though, so you can TODO this with a bug tagged tech-debt if
you like.

https://codereview.appspot.com/75600044/diff/20001/juju/osenv/vars_windows_te...
File juju/osenv/vars_windows_test.go (right):

https://codereview.appspot.com/75600044/diff/20001/juju/osenv/vars_windows_te...
juju/osenv/vars_windows_test.go:9: "launchpad.net/juju-core/testing/testbase"
This is a bit surprising. Are you sure it's right?

https://codereview.appspot.com/75600044/diff/20001/state/user.go
File state/user.go (right):

https://codereview.appspot.com/75600044/diff/20001/state/user.go#newcode32
state/user.go:32: return nil, fmt.Errorf("password can't be empty")
why can admin have an empty password set? (and wasn't there a constant for
"admin"?)

[mattyw, 2014-03-18 03:45:21]

Updated the comments, stuff I haven't commented on is stuff I will do

https://codereview.appspot.com/75600044/diff/20001/cmd/juju/adduser.go
File cmd/juju/adduser.go (right):

https://codereview.appspot.com/75600044/diff/20001/cmd/juju/adduser.go#newcode59
cmd/juju/adduser.go:59: c.Password = args[1]
On 2014/03/18 01:11:39, fwereade wrote:
> I'm still not really in love with allowing people to specify passwords
> (especially in the absence of resetting post-distribution).

I'm not a huge fan of this either, it was added for two reasons:
1) Makes testing easier
2) Allows people to "script" adding users

Suggestions welcome

https://codereview.appspot.com/75600044/diff/20001/cmd/juju/adduser.go#newcode70
cmd/juju/adduser.go:70: return client.AddUser(c.User, c.Password)
On 2014/03/18 01:11:39, fwereade wrote:
> Any possibility we could design this to output a .jenv (*without* all the
> bootstrap-config stuff) on stdout? I think that the pass-jenvs-around model is
> pretty tolerable, really.

Let's discuss this

https://codereview.appspot.com/75600044/diff/20001/juju/osenv/vars_windows_te...
File juju/osenv/vars_windows_test.go (right):

https://codereview.appspot.com/75600044/diff/20001/juju/osenv/vars_windows_te...
juju/osenv/vars_windows_test.go:9: "launchpad.net/juju-core/testing/testbase"
On 2014/03/18 01:11:39, fwereade wrote:
> This is a bit surprising. Are you sure it's right?

I'll double check - I'm sure I was left with no choice at the time

https://codereview.appspot.com/75600044/diff/20001/state/user.go
File state/user.go (right):

https://codereview.appspot.com/75600044/diff/20001/state/user.go#newcode32
state/user.go:32: return nil, fmt.Errorf("password can't be empty")
On 2014/03/18 01:11:39, fwereade wrote:
> why can admin have an empty password set? (and wasn't there a constant for
> "admin"?)

Admin does have a constant- must have missed this one.

I'm sure there was a reason for this - either we discussed it or not having it
caused problems - I'll double check

[mattyw, 2014-03-26 15:01:28]

https://codereview.appspot.com/75600044/diff/20001/errors/errors.go
File errors/errors.go (right):

https://codereview.appspot.com/75600044/diff/20001/errors/errors.go#newcode98
errors/errors.go:98: return fmt.Sprintf("rc: %v", e.Code)
On 2014/03/18 01:11:39, fwereade wrote:
> If we're touching this can we please do something about the overwhelmingly
> shitty UX of being told "rc: 2", which means literally nothing to most
> semi-normal humans? just "subprocess exited with code 2" or something... it'd
be
> way better if we said *which* one but that's way out of scope.

Done.

https://codereview.appspot.com/75600044/diff/20001/juju/osenv/vars_windows_te...
File juju/osenv/vars_windows_test.go (right):

https://codereview.appspot.com/75600044/diff/20001/juju/osenv/vars_windows_te...
juju/osenv/vars_windows_test.go:9: "launchpad.net/juju-core/testing/testbase"
On 2014/03/18 03:45:22, mattyw wrote:
> On 2014/03/18 01:11:39, fwereade wrote:
> > This is a bit surprising. Are you sure it's right?
> 
> I'll double check - I'm sure I was left with no choice at the time

Done.

https://codereview.appspot.com/75600044/diff/20001/state/user.go
File state/user.go (right):

https://codereview.appspot.com/75600044/diff/20001/state/user.go#newcode32
state/user.go:32: return nil, fmt.Errorf("password can't be empty")
On 2014/03/18 03:45:22, mattyw wrote:
> On 2014/03/18 01:11:39, fwereade wrote:
> > why can admin have an empty password set? (and wasn't there a constant for
> > "admin"?)
> 
> Admin does have a constant- must have missed this one.
> 
> I'm sure there was a reason for this - either we discussed it or not having it
> caused problems - I'll double check
> 

Done.

[mattyw, 2014-03-27 10:20:17]

Please take a look.

https://codereview.appspot.com/75600044/diff/20001/cmd/juju/adduser_test.go
File cmd/juju/adduser_test.go (right):

https://codereview.appspot.com/75600044/diff/20001/cmd/juju/adduser_test.go#n...
cmd/juju/adduser_test.go:21: func (s *AddUserSuite) Testadduser(c *gc.C) {
On 2014/03/18 01:11:39, fwereade wrote:
> TestAddUser, please

Done.

https://codereview.appspot.com/75600044/diff/20001/cmd/juju/adduser_test.go#n...
cmd/juju/adduser_test.go:27: c.Assert(err, gc.ErrorMatches, "Failed to create
user: user already exists")
On 2014/03/18 01:11:39, fwereade wrote:
> we don't seem to have tests for the automatic password generation...
This isn't supported in the adduser stuff at the moment. Can I add this in a
separate branch?

https://codereview.appspot.com/75600044/diff/20001/cmd/juju/removeuser_test.go
File cmd/juju/removeuser_test.go (right):

https://codereview.appspot.com/75600044/diff/20001/cmd/juju/removeuser_test.g...
cmd/juju/removeuser_test.go:19: func (s *RemoveUserSuite) Testremoveuser(c
*gc.C) {
On 2014/03/18 01:11:39, fwereade wrote:
> TestRemoveUser

Done.

https://codereview.appspot.com/75600044/diff/20001/cmd/juju/removeuser_test.g...
cmd/juju/removeuser_test.go:24: c.Assert(err, gc.IsNil)
On 2014/03/18 01:11:39, fwereade wrote:
> no tests for CheckEmpty etc (same applies to AddUser as well fwiw)

Done.

https://codereview.appspot.com/75600044/diff/20001/cmd/plugins/juju-metadata/...
File cmd/plugins/juju-metadata/validatetoolsmetadata.go (right):

https://codereview.appspot.com/75600044/diff/20001/cmd/plugins/juju-metadata/...
cmd/plugins/juju-metadata/validatetoolsmetadata.go:24: envcmd.EnvCommandBase
On 2014/03/18 01:11:39, fwereade wrote:
> Hmm. Shouldn't these be using Ensure..?

Done.

https://codereview.appspot.com/75600044/diff/20001/errors/errors.go
File errors/errors.go (right):

https://codereview.appspot.com/75600044/diff/20001/errors/errors.go#newcode124
errors/errors.go:124: // return code from the cmd.Main function rather than the
default of 1 if
On 2014/03/18 01:11:39, fwereade wrote:
> and this comment in particular makes me wonder whether it's right to have
moved
> it. I think it *is* but it's a conceptual boundary violation to mention
cmd.Main
> in this context -- can we maybe just give it a generic name like
"ExitCodeError"
> and keep the special handling details in cmd.Main?
> 
> Scope issues again, though, so you can TODO this with a bug tagged tech-debt
if
> you like.

Done.

[fwereade, 2014-03-27 12:14:34]

TYVM for all the fixes -- a couple of bits need some work, but they'll hopefully
be changes that simplify/reduce scope rather than otherwise. Let me know what
you think.

https://codereview.appspot.com/75600044/diff/40001/cmd/juju/adduser.go
File cmd/juju/adduser.go (right):

https://codereview.appspot.com/75600044/diff/40001/cmd/juju/adduser.go#newcode61
cmd/juju/adduser.go:61: pass, err := terminal.ReadPassword(0)
I'm not sure we really want this -- it STM to qualify as actual logic, and thus
to belong in Run rather than Init. It also should be using stdin/out/err from
the cmd.Context passed into run. Furthermore, ReadPassword seems like overkill
when we're just going to be storing it in plaintext locally.

Let's just give them a default strong password in that case?

https://codereview.appspot.com/75600044/diff/40001/cmd/juju/adduser.go#newcode68
cmd/juju/adduser.go:68: c.Password = args[1]
I feel this lot could be easier to follow. If we drop the terminal stuff, how
about something like:

switch len(args) {
case 0:
    return fmt.Errorf("no username supplied")
case 1:
    c.Password = utils.RandomPassword()
case 2:
    c.Password = args[1]
default:
    return cmd.CheckEmpty(args[2:])
}
c.User = args[0]
return nil

..? YMMV, just a suggestion.

https://codereview.appspot.com/75600044/diff/40001/cmd/juju/adduser.go#newcode88
cmd/juju/adduser.go:88:
//info.SetAPICredentials(configstore.APICredentials{c.User, c.Password})
d

https://codereview.appspot.com/75600044/diff/40001/cmd/juju/adduser.go#newcode89
cmd/juju/adduser.go:89: //info.SetBootstrapConfig(map[string]interface{}{})
d

https://codereview.appspot.com/75600044/diff/40001/cmd/juju/adduser.go#newcode98
cmd/juju/adduser.go:98: err = ioutil.WriteFile(c.OutputFile, data, 0777)
You need to ctx.AbsPath() this so we can test relative paths work right.

https://codereview.appspot.com/75600044/diff/40001/environs/configstore/disk.go
File environs/configstore/disk.go (right):

https://codereview.appspot.com/75600044/diff/40001/environs/configstore/disk....
environs/configstore/disk.go:33: type EnvInfo struct {
I don't think we can just expose this type, it opens up all sorts of cans of
worms with Write and suchlike.

Would you extract the exported fields into a separate exported type, but keep
this one private, and embed the exported type therein? You'd need to tweak the
marshalling to read/write the embedded struct, not environInfo itself, but it
should otherwise be trivial. Make sense?

https://codereview.appspot.com/75600044/diff/40001/environs/configstore/disk....
environs/configstore/disk.go:168: func (info *EnvInfo) Write() error {
Yeah, this isn't going to be too happy if it's created without a .path. Keep
this type private, please.

[mattyw, 2014-03-27 17:12:08]

Please take a look.

https://codereview.appspot.com/75600044/diff/40001/cmd/juju/adduser.go
File cmd/juju/adduser.go (right):

https://codereview.appspot.com/75600044/diff/40001/cmd/juju/adduser.go#newcode61
cmd/juju/adduser.go:61: pass, err := terminal.ReadPassword(0)
On 2014/03/27 12:14:34, fwereade wrote:
> I'm not sure we really want this -- it STM to qualify as actual logic, and
thus
> to belong in Run rather than Init. It also should be using stdin/out/err from
> the cmd.Context passed into run. Furthermore, ReadPassword seems like overkill
> when we're just going to be storing it in plaintext locally.
> 
> Let's just give them a default strong password in that case?

Done.

https://codereview.appspot.com/75600044/diff/40001/cmd/juju/adduser.go#newcode68
cmd/juju/adduser.go:68: c.Password = args[1]
On 2014/03/27 12:14:34, fwereade wrote:
> I feel this lot could be easier to follow. If we drop the terminal stuff, how
> about something like:
> 
> switch len(args) {
> case 0:
>     return fmt.Errorf("no username supplied")
> case 1:
>     c.Password = utils.RandomPassword()
> case 2:
>     c.Password = args[1]
> default:
>     return cmd.CheckEmpty(args[2:])
> }
> c.User = args[0]
> return nil
> 
> ..? YMMV, just a suggestion.

This is a great idea - let'd do this!

https://codereview.appspot.com/75600044/diff/40001/cmd/juju/adduser.go#newcode68
cmd/juju/adduser.go:68: c.Password = args[1]
On 2014/03/27 12:14:34, fwereade wrote:
> I feel this lot could be easier to follow. If we drop the terminal stuff, how
> about something like:
> 
> switch len(args) {
> case 0:
>     return fmt.Errorf("no username supplied")
> case 1:
>     c.Password = utils.RandomPassword()
> case 2:
>     c.Password = args[1]
> default:
>     return cmd.CheckEmpty(args[2:])
> }
> c.User = args[0]
> return nil
> 
> ..? YMMV, just a suggestion.

Done.

https://codereview.appspot.com/75600044/diff/40001/cmd/juju/adduser.go#newcode88
cmd/juju/adduser.go:88:
//info.SetAPICredentials(configstore.APICredentials{c.User, c.Password})
On 2014/03/27 12:14:34, fwereade wrote:
> d

Done.

https://codereview.appspot.com/75600044/diff/40001/cmd/juju/adduser.go#newcode89
cmd/juju/adduser.go:89: //info.SetBootstrapConfig(map[string]interface{}{})
On 2014/03/27 12:14:34, fwereade wrote:
> d

Done.

https://codereview.appspot.com/75600044/diff/40001/cmd/juju/adduser.go#newcode98
cmd/juju/adduser.go:98: err = ioutil.WriteFile(c.OutputFile, data, 0777)
On 2014/03/27 12:14:34, fwereade wrote:
> You need to ctx.AbsPath() this so we can test relative paths work right.

Done.

[fwereade, 2014-03-27 17:27:23]

Couple more tweaks to the command, and a couple of tests (and the json
marshalling); should be the last round.

https://codereview.appspot.com/75600044/diff/60001/cmd/juju/adduser.go
File cmd/juju/adduser.go (right):

https://codereview.appspot.com/75600044/diff/60001/cmd/juju/adduser.go#newcode21
cmd/juju/adduser.go:21: when the environent is destroyed.
Mention that it writes to stdout by default?

https://codereview.appspot.com/75600044/diff/60001/cmd/juju/adduser.go#newcode64
cmd/juju/adduser.go:64: fmt.Println(c.Password)
This isn't using the context to print, which it should; but it can't, because
Init. And really, dammit, we shouldn't be erroring for reasons unrelated to
arg-parsing here at all. Sorry I didn't spot this earlier.

Let's just lose the printing, because it'll be written out anyway in a mo -- or,
?nicer?, leave c.Password empty, and do the RandomPassword in Run if c.Password
== "".

https://codereview.appspot.com/75600044/diff/60001/cmd/juju/adduser.go#newcode98
cmd/juju/adduser.go:98: return c.out.Write(ctx, outputInfo)
I think I'd still write this stuff out before calling AddUser -- a nonsense jenv
can be regenerated, but you can't recover the password if the process goes down
before we write it out.

https://codereview.appspot.com/75600044/diff/60001/cmd/juju/adduser_test.go
File cmd/juju/adduser_test.go (right):

https://codereview.appspot.com/75600044/diff/60001/cmd/juju/adduser_test.go#n...
cmd/juju/adduser_test.go:59: c.Assert(string(data), gc.DeepEquals,
string(expectedStr))
We should have some basic tests for --format to stdout and to -o. Don't
necessarily need to go overboard.

[mattyw, 2014-03-28 07:25:32]

Please take a look.

https://codereview.appspot.com/75600044/diff/60001/cmd/juju/adduser.go
File cmd/juju/adduser.go (right):

https://codereview.appspot.com/75600044/diff/60001/cmd/juju/adduser.go#newcode21
cmd/juju/adduser.go:21: when the environent is destroyed.
On 2014/03/27 17:27:23, fwereade wrote:
> Mention that it writes to stdout by default?

Done.

https://codereview.appspot.com/75600044/diff/60001/cmd/juju/adduser.go#newcode64
cmd/juju/adduser.go:64: fmt.Println(c.Password)
On 2014/03/27 17:27:23, fwereade wrote:
> This isn't using the context to print, which it should; but it can't, because
> Init. And really, dammit, we shouldn't be erroring for reasons unrelated to
> arg-parsing here at all. Sorry I didn't spot this earlier.
> 
> Let's just lose the printing, because it'll be written out anyway in a mo --
or,
> ?nicer?, leave c.Password empty, and do the RandomPassword in Run if
c.Password
> == "".

Done.

https://codereview.appspot.com/75600044/diff/60001/cmd/juju/adduser.go#newcode98
cmd/juju/adduser.go:98: return c.out.Write(ctx, outputInfo)
On 2014/03/27 17:27:23, fwereade wrote:
> I think I'd still write this stuff out before calling AddUser -- a nonsense
jenv
> can be regenerated, but you can't recover the password if the process goes
down
> before we write it out.

Done.

[fwereade, 2014-03-28 10:09:21]

LGTM with trivial fixes:

https://codereview.appspot.com/75600044/diff/80001/cmd/juju/adduser.go
File cmd/juju/adduser.go (right):

https://codereview.appspot.com/75600044/diff/80001/cmd/juju/adduser.go#newcode22
cmd/juju/adduser.go:22: An example jenv file will be output to allow you to use
the environment as that user.
A jenv file identifying the user and the environment will be written to stdout,
or to a path you specify with --output.

When add-environment lands, please add a note to this doc saying what you can do
with the jenv.

https://codereview.appspot.com/75600044/diff/80001/cmd/juju/adduser.go#newcode35
cmd/juju/adduser.go:35: generatePassword bool
I rather like exporting the fields that get set up by Init at least -- and this
field should really go next to Password.

https://codereview.appspot.com/75600044/diff/80001/cmd/juju/adduser_test.go
File cmd/juju/adduser_test.go (right):

https://codereview.appspot.com/75600044/diff/80001/cmd/juju/adduser_test.go#n...
cmd/juju/adduser_test.go:48: ca-cert: ""
because yaml is evil, please test the output by parsing it, rather than just
checking for equality.

https://codereview.appspot.com/75600044/diff/80001/environs/info/info.go
File environs/info/info.go (right):

https://codereview.appspot.com/75600044/diff/80001/environs/info/info.go#newc...
environs/info/info.go:6: // TODO add json tags
d

[mattyw, 2014-03-28 11:00:28]

Please take a look.

https://codereview.appspot.com/75600044/diff/80001/cmd/juju/adduser.go
File cmd/juju/adduser.go (right):

https://codereview.appspot.com/75600044/diff/80001/cmd/juju/adduser.go#newcode22
cmd/juju/adduser.go:22: An example jenv file will be output to allow you to use
the environment as that user.
On 2014/03/28 10:09:22, fwereade wrote:
> A jenv file identifying the user and the environment will be written to
stdout,
> or to a path you specify with --output.
> 
> When add-environment lands, please add a note to this doc saying what you can
do
> with the jenv.

Done.

https://codereview.appspot.com/75600044/diff/80001/cmd/juju/adduser.go#newcode35
cmd/juju/adduser.go:35: generatePassword bool
On 2014/03/28 10:09:22, fwereade wrote:
> I rather like exporting the fields that get set up by Init at least -- and
this
> field should really go next to Password.

Done.

https://codereview.appspot.com/75600044/diff/80001/cmd/juju/adduser_test.go
File cmd/juju/adduser_test.go (right):

https://codereview.appspot.com/75600044/diff/80001/cmd/juju/adduser_test.go#n...
cmd/juju/adduser_test.go:48: ca-cert: ""
On 2014/03/28 10:09:22, fwereade wrote:
> because yaml is evil, please test the output by parsing it, rather than just
> checking for equality.

Done.

https://codereview.appspot.com/75600044/diff/80001/environs/info/info.go
File environs/info/info.go (right):

https://codereview.appspot.com/75600044/diff/80001/environs/info/info.go#newc...
environs/info/info.go:6: // TODO add json tags
On 2014/03/28 10:09:22, fwereade wrote:
> d

Done.

[fwereade, 2014-03-28 11:26:50]

LGTM, thanks

[mattyw, 2014-03-28 12:34:37]

Please take a look.

[rog, 2014-03-31 14:21:12]

Looks great in general, with some comments and suggestions below.

https://codereview.appspot.com/75600044/diff/120001/cmd/envcmd/environmentcom...
File cmd/envcmd/environmentcommand.go (right):

https://codereview.appspot.com/75600044/diff/120001/cmd/envcmd/environmentcom...
cmd/envcmd/environmentcommand.go:67: func (c *EnvCommandBase) EnsureEnvNameSet()
error {
I suggest that Init would be a more appropriate name for this method (we want
every command that embeds it to call it)

// Init initializes the Command.
func (c *Command) Init() error

https://codereview.appspot.com/75600044/diff/120001/cmd/plugins/juju-restore/...
File cmd/plugins/juju-restore/restore.go (right):

https://codereview.appspot.com/75600044/diff/120001/cmd/plugins/juju-restore/...
cmd/plugins/juju-restore/restore.go:92: if c.showDescription {
this should call EnsureEnvNameSet, presumably?

https://codereview.appspot.com/75600044/diff/120001/environs/info/info.go
File environs/info/info.go (right):

https://codereview.appspot.com/75600044/diff/120001/environs/info/info.go#new...
environs/info/info.go:6: type EnvironInfo struct {
Why does this need its own package?

https://codereview.appspot.com/75600044/diff/120001/environs/manual/provision...
File environs/manual/provisioner_test.go (right):

https://codereview.appspot.com/75600044/diff/120001/environs/manual/provision...
environs/manual/provisioner_test.go:112: c.Assert(err, gc.ErrorMatches, "error
checking if provisioned: subprocess encountered error code 255")
I know it's not your doing, but I don't like seeing these error messages. An
error code tells us almost nothing. We really need to change things so we use
stderr when possible. No action required, just saying.

https://codereview.appspot.com/75600044/diff/120001/errors/errors.go
File errors/errors.go (right):

https://codereview.appspot.com/75600044/diff/120001/errors/errors.go#newcode101
errors/errors.go:101: func IsRcPassthroughError(err error) bool {
If reasonable, I think this really does belong to cmd, because it pertains
directly to functionality in that package.

https://codereview.appspot.com/75600044/diff/120001/errors/errors.go#newcode126
errors/errors.go:126: // TODO (mattyw) It's odd that we're talking about
cmd.Main in here.
indeed

https://codereview.appspot.com/75600044/diff/120001/juju/api.go
File juju/api.go (right):

https://codereview.appspot.com/75600044/diff/120001/juju/api.go#newcode104
juju/api.go:104: func NewUserManagerClient(envName string) (*usermanager.Client,
error) {
Rather than create a new function in juju for every single API client, I'd
prefer to just export NewAPIClient here, and for clients to use facade New
methods as appropriate.

So I'd delete NewKeyManagerClient and NewUserManagerClient, and add methods to
api.Client, KeyManager and UserManager.

That means a certain amount of shake up (the New functions need to change to
take an interface to avoid cyclic imports), but it's the same pattern that works
well for agents, and consistency is good.

Perhaps leave it like it is for now, and address that in a subsequent CL?

https://codereview.appspot.com/75600044/diff/120001/testing/environ.go
File testing/environ.go (right):

https://codereview.appspot.com/75600044/diff/120001/testing/environ.go#newcod...
testing/environ.go:243: func MakeSampleHome(c *gc.C) *FakeHome {
gosh, these functions could all do with doc comments. I wonder what the
difference is between a "fake home" and a "sample home".

https://codereview.appspot.com/75600044/diff/120001/testing/environ.go#newcod...
testing/environ.go:247: func MakeSampleHomeWithFiles(c *gc.C, files []TestFile)
*FakeHome {
Or perhaps change MakeSampleHome to be:

func MakeSampleHome(c *gc.C, files ...TestFile) *FakeHome

[mattyw, 2014-04-01 08:07:30]

https://codereview.appspot.com/75600044/diff/120001/cmd/envcmd/environmentcom...
File cmd/envcmd/environmentcommand.go (right):

https://codereview.appspot.com/75600044/diff/120001/cmd/envcmd/environmentcom...
cmd/envcmd/environmentcommand.go:67: func (c *EnvCommandBase) EnsureEnvNameSet()
error {
On 2014/03/31 14:21:12, rog wrote:
> I suggest that Init would be a more appropriate name for this method (we want
> every command that embeds it to call it)
> 
> // Init initializes the Command.
> func (c *Command) Init() error

Done.

https://codereview.appspot.com/75600044/diff/120001/cmd/plugins/juju-restore/...
File cmd/plugins/juju-restore/restore.go (right):

https://codereview.appspot.com/75600044/diff/120001/cmd/plugins/juju-restore/...
cmd/plugins/juju-restore/restore.go:92: if c.showDescription {
On 2014/03/31 14:21:12, rog wrote:
> this should call EnsureEnvNameSet, presumably?

Done.

https://codereview.appspot.com/75600044/diff/120001/environs/info/info.go
File environs/info/info.go (right):

https://codereview.appspot.com/75600044/diff/120001/environs/info/info.go#new...
environs/info/info.go:6: type EnvironInfo struct {
On 2014/03/31 14:21:12, rog wrote:
> Why does this need its own package?

Done.

https://codereview.appspot.com/75600044/diff/120001/errors/errors.go
File errors/errors.go (right):

https://codereview.appspot.com/75600044/diff/120001/errors/errors.go#newcode101
errors/errors.go:101: func IsRcPassthroughError(err error) bool {
On 2014/03/31 14:21:12, rog wrote:
> If reasonable, I think this really does belong to cmd, because it pertains
> directly to functionality in that package.

Done.

https://codereview.appspot.com/75600044/diff/120001/errors/errors.go#newcode126
errors/errors.go:126: // TODO (mattyw) It's odd that we're talking about
cmd.Main in here.
On 2014/03/31 14:21:12, rog wrote:
> indeed

Done.

https://codereview.appspot.com/75600044/diff/120001/juju/api.go
File juju/api.go (right):

https://codereview.appspot.com/75600044/diff/120001/juju/api.go#newcode104
juju/api.go:104: func NewUserManagerClient(envName string) (*usermanager.Client,
error) {
On 2014/03/31 14:21:12, rog wrote:
> Rather than create a new function in juju for every single API client, I'd
> prefer to just export NewAPIClient here, and for clients to use facade New
> methods as appropriate.
> 
> So I'd delete NewKeyManagerClient and NewUserManagerClient, and add methods to
> api.Client, KeyManager and UserManager.
> 

https://bugs.launchpad.net/juju-core/+bug/1300637
> That means a certain amount of shake up (the New functions need to change to
> take an interface to avoid cyclic imports), but it's the same pattern that
works
> well for agents, and consistency is good.
> 
> Perhaps leave it like it is for now, and address that in a subsequent CL?

https://codereview.appspot.com/75600044/diff/120001/testing/environ.go
File testing/environ.go (right):

https://codereview.appspot.com/75600044/diff/120001/testing/environ.go#newcod...
testing/environ.go:247: func MakeSampleHomeWithFiles(c *gc.C, files []TestFile)
*FakeHome {
On 2014/03/31 14:21:12, rog wrote:
> Or perhaps change MakeSampleHome to be:
> 
> func MakeSampleHome(c *gc.C, files ...TestFile) *FakeHome

Done.

[mattyw, 2014-04-01 08:27:35]

Please take a look.

[fwereade, 2014-04-01 09:40:52]

I'm a little bit irritated at the changes to EnsureEnvNameSet, and the
EnvironInfoData type, but neither decision is irrevocable and I really cannot be
bothered to fight them; let's just land this.

LGTM contingent on the other tweaks below, ping me if anything's not clear.

https://codereview.appspot.com/75600044/diff/120001/cmd/envcmd/environmentcom...
File cmd/envcmd/environmentcommand.go (right):

https://codereview.appspot.com/75600044/diff/120001/cmd/envcmd/environmentcom...
cmd/envcmd/environmentcommand.go:67: func (c *EnvCommandBase) EnsureEnvNameSet()
error {
On 2014/04/01 08:07:31, mattyw wrote:
> On 2014/03/31 14:21:12, rog wrote:
> > I suggest that Init would be a more appropriate name for this method (we
want
> > every command that embeds it to call it)
> > 
> > // Init initializes the Command.
> > func (c *Command) Init() error
> 
> Done.

I suggest that it would not, because it's aping the cmd.Command interface but
not actually satisfying it; and it has different responsibilities, which might
be most relevant at Init time but are actually to do with, uh, ensuring an
environment is set...

https://codereview.appspot.com/75600044/diff/120001/environs/info/info.go
File environs/info/info.go (right):

https://codereview.appspot.com/75600044/diff/120001/environs/info/info.go#new...
environs/info/info.go:6: type EnvironInfo struct {
On 2014/04/01 08:07:31, mattyw wrote:
> On 2014/03/31 14:21:12, rog wrote:
> > Why does this need its own package?
> 
> Done.

What's the benefit of removing a small and focused package in favour of jamming
its contents into a tangentially-related one? at the cost of a sensible type
name?

https://codereview.appspot.com/75600044/diff/120001/environs/manual/provision...
File environs/manual/provisioner_test.go (right):

https://codereview.appspot.com/75600044/diff/120001/environs/manual/provision...
environs/manual/provisioner_test.go:112: c.Assert(err, gc.ErrorMatches, "error
checking if provisioned: subprocess encountered error code 255")
On 2014/03/31 14:21:12, rog wrote:
> I know it's not your doing, but I don't like seeing these error messages. An
> error code tells us almost nothing. We really need to change things so we use
> stderr when possible. No action required, just saying.

Progress not perfection. This beats the hell out of "rc:255" :).

https://codereview.appspot.com/75600044/diff/140001/cmd/juju/adduser.go
File cmd/juju/adduser.go (right):

https://codereview.appspot.com/75600044/diff/140001/cmd/juju/adduser.go#newco...
cmd/juju/adduser.go:98: c.out.Write(ctx, outputInfo)
doesn't this return an error?

https://codereview.appspot.com/75600044/diff/140001/cmd/juju/environment.go
File cmd/juju/environment.go (right):

https://codereview.appspot.com/75600044/diff/140001/cmd/juju/environment.go#n...
cmd/juju/environment.go:219: func (c *UnsetEnvironmentCommand) Init(args
[]string) (err error) {
This needs EnsureEnvNameSet as well

https://codereview.appspot.com/75600044/diff/140001/juju/api.go
File juju/api.go (right):

https://codereview.appspot.com/75600044/diff/140001/juju/api.go#newcode136
juju/api.go:136: envs, err := environs.ReadEnvirons("")
Shouldn't this code have been deleted? We should never have an empty envName to
deal with now...

[mattyw, 2014-04-01 14:31:01]

https://codereview.appspot.com/75600044/diff/140001/cmd/juju/adduser.go
File cmd/juju/adduser.go (right):

https://codereview.appspot.com/75600044/diff/140001/cmd/juju/adduser.go#newco...
cmd/juju/adduser.go:98: c.out.Write(ctx, outputInfo)
On 2014/04/01 09:40:54, fwereade wrote:
> doesn't this return an error?

Done.

https://codereview.appspot.com/75600044/diff/140001/cmd/juju/environment.go
File cmd/juju/environment.go (right):

https://codereview.appspot.com/75600044/diff/140001/cmd/juju/environment.go#n...
cmd/juju/environment.go:219: func (c *UnsetEnvironmentCommand) Init(args
[]string) (err error) {
On 2014/04/01 09:40:54, fwereade wrote:
> This needs EnsureEnvNameSet as well

Done.

https://codereview.appspot.com/75600044/diff/140001/juju/api.go
File juju/api.go (right):

https://codereview.appspot.com/75600044/diff/140001/juju/api.go#newcode136
juju/api.go:136: envs, err := environs.ReadEnvirons("")
On 2014/04/01 09:40:54, fwereade wrote:
> Shouldn't this code have been deleted? We should never have an empty envName
to
> deal with now...
https://bugs.launchpad.net/juju-core/+bug/1300823

[mattyw, 2014-04-01 14:37:05]

Please take a look.