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.