Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Improve error text for unauthorized add commands #6377
Conversation
| + command = "complete this operation" | ||
| + } | ||
| + permMsg := fmt.Sprintf(permMsg, command) | ||
| + return errors.Errorf("%v\n\n%s\n%s\n", err, permMsg, grantMsg) |
howbazaar
Oct 5, 2016
Owner
You should probably wrap the error to keep the stack.
Also, should you check that err is not nil?
newErr := errors.Errorf("%v\n\n%s\n%s\n", err, permMsg, grantMsg)
return errors.Wrap(err, newErr)
kat-co
suggested changes
Oct 5, 2016
So I talked this over with @howbazaar some more. I think this might be a better way of doing things.
Rather than trying to shove a message we want printed out onto stderr in an error type, let's instead create a function that takes in io.Writer (potentially ansiterm.Writer) and writes out both the friendly message and the underlying error to the user.
Then return errors.Wrap(err, cmd.ErrSilent) so that the command returns nothing further.
| + stdout.Write([]byte( | ||
| + fmt.Sprintf("\n%s\n%s\n\n", permMsg, grantMsg)), | ||
| + ) | ||
| + return err |
macgreagoir
Oct 6, 2016
•
Edited 1 time
-
macgreagoir
Oct 6, 2016
Contributor
-
Oct 6, 2016
Returning errors.Wrap(err, cmd.ErrSilent) here shows a potentially confusing error , unless the user redirects stderr [to /dev/null]. It seemed appropriate to just return the err, with the friendlier message written to stdout. I stand to be corrected :-)
macgreagoir
Oct 7, 2016
Contributor
...and I've reworked it slightly again, leaving the caller to return err, and letting the new function just print the nicer message.
macgreagoir
added some commits
Oct 4, 2016
| + if command == "" { | ||
| + command = "complete this operation" | ||
| + } | ||
| + fmt.Fprintf(writer, "\n%s\n%s\n\n", fmt.Sprintf(perm, command), grant) |
kat-co
Oct 11, 2016
Contributor
Nitpick that shouldn't block landing:
This reads a little weird. I think it would be more common to see this as a series of Fprintfs.
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
macgreagoir commentedOct 4, 2016
The following commands now output more user-friendly responses,
mentioning the use of
juju grant, when the API returns an"unauthorized access" error.
Fixes lp:1612046
QA steps:
juju add-user fooandjuju grant foo read defaultsudo adduser fooandsudo su - foojuju register <as output by juju register>juju switch <controller>:admin@local/defaultjuju grant.