Centralize command-line parsing #2717
Conversation
|
Running this locally I don't actually see any of the command-specific options, just the generic ones. |
|
Oops, I had added a check just before committing for hiding the 'Command Options' section if it was empty (for force-leave) and NFlag() wasn't the right thing to use for that. |
command/configtest.go
Outdated
| f.Var((*agent.AppendSliceValue)(&c.configFiles), "config-file", | ||
| "Path to a JSON file to read configuration from. This can be specified multiple times.") | ||
| f.Var((*agent.AppendSliceValue)(&c.configFiles), "config-dir", | ||
| "Path to a directory to read configuration files from. This will read every file ending in "+ |
command/lock.go
Outdated
| "implementation switches from a lock to a semaphore when the value is "+ | ||
| "greater than 1. The default value is 1.") | ||
| f.IntVar(&c.monitorRetry, "monitor-retry", defaultMonitorRetry, | ||
| "Number of times to retry Consul returns a 500 error while monitoring "+ |
command/lock.go
Outdated
| if limit == 1 { | ||
| *lu, err = c.setupLock(client, prefix, name, oneshot, wait, retry) | ||
| if c.limit == 1 { | ||
| *lu, err = c.setupLock(client, prefix, c.name, oneshot, c.timeout, c.monitorRetry) |
There was a problem hiding this comment.
These ended up a little weird passing member variables to member functions. I'd probably make these all local variables.
command/meta.go
Outdated
| FlagSetHTTP FlagSetFlags = iota << 1 | ||
| ) | ||
|
|
||
| type Meta struct { |
There was a problem hiding this comment.
Let's add a comment about what Meta does - it's bad enough we have a class called Meta :-D
There was a problem hiding this comment.
should we rename it to CommandMeta or something similar? I agree the name's not great
There was a problem hiding this comment.
It's in the command package, so it's already command.Meta to the outside - I couldn't think of a better name for it. CommonFlags or StandardFlags might be better, do either of those seem good to you?
command/meta.go
Outdated
| panic("flags have not been parsed") | ||
| } | ||
|
|
||
| return api.NewClient(&api.Config{ |
There was a problem hiding this comment.
I don't think this will respect the environment variables such as for the token. I think we need to start with an api.DefaultConfig() and then optionally poke in the command line variables if they are present. See #2610.
command/meta.go
Outdated
| return skip | ||
| } | ||
|
|
||
| // wrapAtLength wraps the given text at the maxLineLength, taxing into account |
|
Let's update the website docs for each command as we convert. That'll get them synced up and serve as a good sanity check. |
command/base/command.go
Outdated
| c.httpAddr.Merge(&config.Address) | ||
| c.token.Merge(&config.Token) | ||
| c.datacenter.Merge(&config.Datacenter) | ||
| c.Ui.Info(fmt.Sprintf("client http addr: %s", config.Address)) |
There was a problem hiding this comment.
This'll add clutter to the command output - let's drop this print.
There was a problem hiding this comment.
oops, this was a debug print that got left in
command/base/config_util.go
Outdated
| "time" | ||
|
|
||
| "github.com/mitchellh/mapstructure" | ||
| "os" |
There was a problem hiding this comment.
These are messed up - should move them up.
command/base/command.go
Outdated
| type FlagSetFlags uint | ||
|
|
||
| const ( | ||
| FlagSetNone FlagSetFlags = iota << 1 |
There was a problem hiding this comment.
These should be 1 << iota.
This picks up from #2412 and converts the
configtestandforce-leavecommands to use the Meta struct.