Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
fix up error handling when no current controller #6306
Conversation
| + context, err := s.runListControllers(c) | ||
| + c.Assert(err, jc.ErrorIsNil) | ||
| + c.Assert(testing.Stdout(context), gc.Equals, "") | ||
| + c.Assert(testing.Stderr(context), gc.Equals, modelcmd.ErrNoControllersDefined.Error()) |
| -Please either create your own new controller using "juju bootstrap" or | ||
| -connect to another controller that you have been given access to using "juju register". | ||
| +Please either create a new controller using "juju bootstrap" or connect to | ||
| +another controller that you have been given access to using "juju register". | ||
| `) |
kat-co
Sep 22, 2016
Contributor
I'm pretty sure we shouldn't try and fix this here, but we should define error messages as simple strings with no newlines, and the view on the error should be controlled when its output.
natefinch
Sep 22, 2016
Contributor
I know .... I started to do that, but it was going to be too complicated of a change, when all that was really asked for was for the text to be tweaked.
| + if !errors.IsNotFound(err) { | ||
| + return err | ||
| + } | ||
| + controllers, err := store.AllControllers() |
natefinch
Sep 22, 2016
Contributor
Nope, it reads your controllers.yaml. Disk access, while not ideal, still fairly reliable.
| + if len(controllers) == 0 { | ||
| + return ErrNoControllersDefined | ||
| + } | ||
| + return ErrNoCurrentController | ||
| } |
kat-co
Sep 22, 2016
Contributor
I don't think we should be discarding the error passed in. Please either mask it or annotate it.
natefinch
Sep 22, 2016
Contributor
Looks like Wrap has the right behavior - change the message and the cause, but keep the stack
natefinch
Sep 22, 2016
Contributor
New code is here: 0934520#diff-3475f343e517b41c7eb532ace14c4655R298
natefinch
referenced this pull request
Sep 22, 2016
Merged
fix up error handling when no current controller #6307
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
natefinch commentedSep 22, 2016
fixes https://bugs.launchpad.net/juju/+bug/1618998
list-models should point out list-controllers command in error message
QA:
Edit ~/.local/share/juju/controllers.yaml to remove the value for current-controller
(it's at the bottom). Then run juju status and juju models. You should get nice
error messages that tell you how to get out of the situation. Also try changing
current-controller to a controller name that doesn't exist. It should behave the
same as not having a current controller.