juju help has confusing and inconsistent defaults

Bug #1637814 reported by James Troup
16
This bug affects 3 people
Affects Status Importance Assigned to Milestone
juju
Triaged
High
Unassigned

Bug Description

The defaults output in juju help is both inconsistent and confusing.
See below in the help for juju run (this is with Juju 2.0.0):

 * Why is application default the empty string and model ""?

 * What does a default of 'false' mean for a --no-browser-login option
   - does it mean that the default is to use a browser login
   (i.e. double negative)? Or does it means that
   -B/--no-browser-login is the default (single negative)?

| $ juju help run
| Usage: juju run [options] <commands>
|
| Summary:
| Run the commands on the remote targets specified.
|
| Options:
| -B, --no-browser-login (= false)
| Do not use web browser for authentication
| --all (= false)
| Run the commands on all the machines
| --application (= )
| One or more application names
| --format (= default)
| Specify output format (default|json|yaml)
| -m, --model (= "")
| Model to operate in. Accepts [<controller name>:]<model name>
| --machine (= )
| One or more machine ids
| -o, --output (= "")
| Specify an output file
| --timeout (= 5m0s)
| How long to wait before the remote command is considered to have failed
| --unit (= )
| One or more unit ids
|
| Details:
| Run the commands on the specified targets. Only admin users of a model
| are able to use this command.
|
| Targets are specified using either machine ids, application names or unit
| names. At least one target specifier is needed.
|
| Multiple values can be set for --machine, --application, and --unit by using
| comma separated values.
|
| If the target is a machine, the command is run as the "ubuntu" user on
| the remote machine.
|
| If the target is an application, the command is run on all units for that
| application. For example, if there was an application "mysql" and that application
| had two units, "mysql/0" and "mysql/1", then
| --application mysql
| is equivalent to
| --unit mysql/0,mysql/1
|
| Commands run for applications or units are executed in a 'hook context' for
| the unit.
|
| --all is provided as a simple way to run the command on all the machines
| in the model. If you specify --all you cannot provide additional
| targets.
|
| Since juju run creates actions, you can query for the status of commands
| started with juju run by calling "juju show-action-status --name juju-run".
| $

Changed in juju:
status: New → Triaged
importance: Undecided → High
tags: added: usability
tags: added: helptext
Revision history for this message
Anastasia (anastasia-macmood) wrote :

It is a bit confusing at a quick glance but there is an explanation :)

I am thinking about how to make it more obvious but it really is not... The default for 'application', 'machine' and 'unit' is (= ) as opposed to (= "") like 'model' is because for this command, the arguments are not actually strings but a collection of strings, slices specifically.

I mean beyond being technically correct and doing something like (= []) - which is cumbersome as well, I do not know how to make it more explicit and less confusing... Suggestions are welcome :)

And, yes, no-browser-login is set to 'false' by default so the user will have a browser login unless this option is provided/set to true. If you have a suggested wording change, we are happy to change the wording.

Changed in juju:
status: Triaged → Incomplete
Revision history for this message
Launchpad Janitor (janitor) wrote :

[Expired for juju because there has been no activity for 60 days.]

Changed in juju:
status: Incomplete → Expired
Revision history for this message
Paul Gear (paulgear) wrote :

Getting rid of the double-negative option name would be a good start. And having firm guidelines that such options should be avoided rigorously. (Double negatives in English can be particularly confusing for non-native English speakers, since in some languages the 2nd negative intensifies rather than negates the 1st negative.)

Changed in juju:
status: Expired → New
Revision history for this message
John A Meinel (jameinel) wrote :

As for the "why", it is because application and machine are lists vs model being a single string. And the way it formats its value is:
// Implements gnuflag.Value String.
func (v *StringsValue) String() string {
 return strings.Join(*v, ",")
}

And when len(v) is empty, you get nothing out. And the code that handles rendering the help template uses:
  format := "%s (= %s)\n %s\n"
  if _, ok := fs[0].Value.(*stringValue); ok {
   // put quotes on the value
   format = "%s (= %q)\n %s\n"
  }
  fmt.Fprintf(f.out(), format, line.Bytes(), fs[0].DefValue, fs[0].Usage)
 }

Which says that only if the exact content can be cast to a pointer-to-string does it format using quoted form (thus an empty string shows up as "", rather than literal nothing.)

We could probably change StringsValue to return a quoted form always:
func (v *StringsValue) String() string {
 return strconv.Quote(strings.Join(*v, ","))
}

I think that is better than '[]' because *users* don't supply them as a list, they supply "app1,app2", so we should show that it is a string that we expect.

--no-browser-login is certainly an interesting case where gnuflag/flag doesn't provide a clear way to indicate that you would want two flags "--browser-login" *and* "--no-browser-login" and a way to provide what setting is default for that boolean. (The 'go' way of doing it is to provide a single boolean and the user negates it with '--foo' means '--foo=true' and '--foo=false' is the way of turning off that option rather than '--no-foo', but that doesn't match a lot of other CLIs that people are used to.)

Since *Juju* doesn't follow the flag conventions of booleans and =false to disable them, I think having the (= false) is more harmful/noise than helpful. Though we don't actually have a way to target the same boolean (so all our booleans except for exceptional circumstances are default false so that you can just supply the option without =true for it to work as people expect.)

Changed in juju:
status: New → Triaged
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers