cmd/juju/user: set controller name properly #7092

Merged
merged 1 commit into from Mar 14, 2017

Conversation

Projects
None yet
5 participants
Owner

rogpeppe commented Mar 13, 2017

Because the controller wasn't being set, the ListControllers
call was failing. The tests still passed because the ListControllers
call was entirely mocked.

Also make a backwardly incompatible change so that the old
"juju login $user" behaviour is no longer the default - to log in as
a user, the "--user" (or "-u") flag is required.

loginDoc needs to be updated to reflect this change

cmd/juju/user/login.go
@@ -128,6 +129,7 @@ func (c *loginCommand) Init(args []string) error {
// Run implements Command.Run.
func (c *loginCommand) Run(ctx *cmd.Context) error {
+ logger.Infof("in loginCommand.Run")
@mjs

mjs Mar 13, 2017

Contributor

not necessary?

@rogpeppe

rogpeppe Mar 14, 2017

Owner

debugging remnant, deleted

@@ -76,7 +76,7 @@ func (s *LoginCommandSuite) TestInitError(c *gc.C) {
}
func (s *LoginCommandSuite) TestLogin(c *gc.C) {
- stdout, stderr, code := runLogin(c, "current-user\nsekrit\n")
+ stdout, stderr, code := runLogin(c, "current-user\nsekrit\n", "-u")
@mjs

mjs Mar 13, 2017

Contributor

Just checking, are there still some tests which check user logins without -u?

@rogpeppe

rogpeppe Mar 14, 2017

Owner

You can't do a user login without -u any more.

@mjs

mjs Mar 14, 2017

Contributor

ok, no problems

Contributor

mjs commented Mar 13, 2017

Looks good overall.

@juju/docs: Possible minor docs impact here.

LGTM

cmd/juju/user/login.go
- if controllerHost == "" {
- if !c.forceHost {
- return errNotControllerLogin
+ logger.Infof("controllerLogin")
@mhilton

mhilton Mar 14, 2017

Member

Info looks high for this message, left over debugging?

@mjs

mjs Mar 14, 2017

Contributor

agreed that this probably shouldn't be here

mjs approved these changes Mar 14, 2017

cmd/juju/user/login.go
+(either a known public controller or the host name of a public
+controller). In this case, the -c flag can be used to choose a name for
+the new controller. The -u flag causes it to log into a controller as a
+local user instead - the -c flag names a current controller in this case.
@mjs

mjs Mar 14, 2017

Contributor

I think this might be clearer for the above paragraph:

By default, the juju login command logs the user into a public controller. If the controller is already known it can be specified by name. Alternatively, the host name of a public controller can be specified. The -c flag can be used to specify a name for the controller when a host name is provided.

If the -u flag is provided, the juju login command will attempt to log into a controller as a local user. In this case, the -c flag names the controller to log in to.

This still assumes a fair bit of knowledge (e.g. the "public controller" and "local user" concepts) but I think it's clearer.

@rogpeppe

rogpeppe Mar 14, 2017

Owner

Better, thanks. Done.

cmd/juju/user/login.go
- if controllerHost == "" {
- if !c.forceHost {
- return errNotControllerLogin
+ logger.Infof("controllerLogin")
@mhilton

mhilton Mar 14, 2017

Member

Info looks high for this message, left over debugging?

@mjs

mjs Mar 14, 2017

Contributor

agreed that this probably shouldn't be here

@@ -76,7 +76,7 @@ func (s *LoginCommandSuite) TestInitError(c *gc.C) {
}
func (s *LoginCommandSuite) TestLogin(c *gc.C) {
- stdout, stderr, code := runLogin(c, "current-user\nsekrit\n")
+ stdout, stderr, code := runLogin(c, "current-user\nsekrit\n", "-u")
@mjs

mjs Mar 13, 2017

Contributor

Just checking, are there still some tests which check user logins without -u?

@rogpeppe

rogpeppe Mar 14, 2017

Owner

You can't do a user login without -u any more.

@mjs

mjs Mar 14, 2017

Contributor

ok, no problems

Owner

rogpeppe commented Mar 14, 2017

$$merge$$

Contributor

jujubot commented Mar 14, 2017

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

Contributor

jujubot commented Mar 14, 2017

Build failed: Tests failed
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju/10456

Owner

rogpeppe commented Mar 14, 2017

$$merge$$

Contributor

jujubot commented Mar 14, 2017

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

@jujubot jujubot merged commit e699e58 into juju:user-domain Mar 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment