Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
cmd/juju/user: refactor login command #7136
Conversation
| - defer api.Close() | ||
| - mm := modelmanager.NewClient(api) | ||
| + listModels = func(c api.Connection, userName string) ([]apibase.UserModel, error) { | ||
| + mm := modelmanager.NewClient(c) |
| - if ctl.ControllerUUID == controllerDetails.ControllerUUID { | ||
| - // TODO(rogpeppe) lp#1614010 Succeed but override the account details in this case? | ||
| - return errors.Errorf("controller is already registered as %q", name) | ||
| +var errAlreadyLoggedIn = errors.Errorf(`already logged in |
mhilton
Mar 22, 2017
Member
It seems to me that there's something icky about having this sort of formatting embedded in an error.
nskaggs
Mar 22, 2017
Owner
Might it be possible to be clearer to users who are used to the 'controller already registered as' to be more explicit that the user is already logged into the controller in question, and it's available to them? Perhaps just adding the controller name / location to this message?
rogpeppe
Mar 23, 2017
Owner
The error that's printed already mentions the controller name. I made it mention the user name too.
wallyworld
approved these changes
Mar 23, 2017
Looks good. I've left some suggestions for better error messages. I fell that end users will not understand the wording of what's there now.
| - fset.BoolVar(&c.forceUser, "u", false, "log into the current controller as a local user") | ||
| - fset.BoolVar(&c.forceUser, "user", false, "") | ||
| + fset.StringVar(&c.username, "u", "", "log in as this local user") | ||
| + fset.StringVar(&c.username, "user", "", "") |
wallyworld
Mar 23, 2017
Owner
probably should have a description here too, making the distinction that this flag is used to specify a local user
rogpeppe
Mar 23, 2017
Owner
As they both refer to the same variable, they're treated as aliases and the same description is used for both (taken from the shortest flag as per the gnuflag docs).
| + } else if controllerDetails == nil { | ||
| + // No controller found and no domain specified - we | ||
| + // have no idea where we should be logging in. | ||
| + return errors.Errorf("domain not specified and controller does not exist") |
wallyworld
Mar 23, 2017
Owner
the end user has no idea what a domain is.
can we use the term "host name" or something?
".. and controller does not exist" is also not clear.
what about "... and there is no current controller registered" or something like that
rogpeppe
Mar 23, 2017
Owner
Unfortunately, "host name" isn't accurate either, as it can refer to one of the publicly defined controller aliases. "no current controller registered" isn't quite right either, as when the -c flag is specified we might not be talking about the current controller. I'll try to think of something else.
| + // The domain we're trying to log into doesn't match the | ||
| + // existing controller. | ||
| + return errors.Errorf(` | ||
| +controller at %q does not match existing controller. |
wallyworld
Mar 23, 2017
Owner
An end user would not really grok this message I don't think.
How about something like
Connection details for controller %q have changed.
You can remove the outdated controller details using
"juju unregister %s".
To login to a different controller, use the -c flag to specify another name"
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
$$merge$$ |
|
Build failed: Tests failed |
|
$$merge$$ Maybe unrelated failure on windows: FAIL: :243: UpgradeCharmStoreResourceSuite.SetUpTest |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$merge$$ and this time because: [LOG] 0:00.151 DEBUG juju.mongo dialled mongodb server at "127.0.0.1:43363" |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
rogpeppe commentedMar 22, 2017
We integrate the two kinds of login, so local user login
is orthogonal to whether you're logging into a public controller.
Logging into a controller is now idempotent - no message
will be printed unless the account or controller have changed.
The -u (--user) flag now takes the username as its argument,
so the argument to juju login is always now the controller domain.
Also add a few tests for existing untested behaviour and integrate
the two login test suites (but left them in two separate files for now
so that the diff looks better).
To QA, try "juju login" with as many different combinations as possible.