config: refactor commands to print help for flags (#3536)

This patch refactors the commands that use the mitchellh/cli library to
populate the command line flag set in both the Run() and the Help()
method. Earlier versions of the mitchellh/cli library relied on the
Run() method to populuate the flagset for generating the usage screen.
This has changed in later versions and was previously solved with a
small monkey patch to the library to restore the old behavior.

However, this makes upgrading the library difficult since the patch has
to be restored every time.

This patch addresses this by moving the command line flags into an
initFlags() method where appropriate and also moving all variables for
the flags from the Run() method into the command itself.

Fixes #3536

command/agent.go

@@ -49,11 +49,12 @@ type AgentCommand struct {
 // readConfig is responsible for setup of our configuration using
 // the command line and any file configs
 func (cmd *AgentCommand) readConfig() *config.RuntimeConfig {
+	cmd.InitFlagSet()
+
 	var flags config.Flags
-	fs := cmd.BaseCommand.NewFlagSet(cmd)
-	config.AddFlags(fs, &flags)
+	config.AddFlags(cmd.FlagSet, &flags)
 
-	if err := cmd.BaseCommand.Parse(cmd.args); err != nil {
+	if err := cmd.FlagSet.Parse(cmd.args); err != nil {
 		if !strings.Contains(err.Error(), "help requested") {
 			cmd.UI.Error(fmt.Sprintf("error parsing flags: %v", err))
 		}
@@ -480,15 +481,15 @@ func (cmd *AgentCommand) Synopsis() string {
 }
 
 func (cmd *AgentCommand) Help() string {
-	helpText := `
+	cmd.InitFlagSet()
+	config.AddFlags(cmd.FlagSet, &config.Flags{})
+	return cmd.HelpCommand(`
 Usage: consul agent [options]
 
   Starts the Consul agent and runs until an interrupt is received. The
   agent represents a single node in a cluster.
 
- ` + cmd.BaseCommand.Help()
-
-	return strings.TrimSpace(helpText)
+ `)
 }
 
 func printJSON(name string, v interface{}) {

command/base.go

@@ -35,7 +35,8 @@ type BaseCommand struct {
 
 	HideNormalFlagsHelp bool
 
-	flagSet *flag.FlagSet
+	FlagSet *flag.FlagSet
+	hidden  *flag.FlagSet
 
 	// These are the options which correspond to the HTTP API options
 	httpAddr      configutil.StringValue
@@ -56,7 +57,7 @@ func (c *BaseCommand) HTTPClient() (*api.Client, error) {
 	if !c.hasClientHTTP() && !c.hasServerHTTP() {
 		panic("no http flags defined")
 	}
-	if !c.flagSet.Parsed() {
+	if !c.FlagSet.Parsed() {
 		panic("flags have not been parsed")
 	}
 
@@ -141,19 +142,19 @@ func (c *BaseCommand) httpFlagsServer() *flag.FlagSet {
 
 // NewFlagSet creates a new flag set for the given command. It automatically
 // generates help output and adds the appropriate API flags.
-func (c *BaseCommand) NewFlagSet(command cli.Command) *flag.FlagSet {
-	c.flagSet = flag.NewFlagSet("", flag.ContinueOnError)
-	c.flagSet.Usage = func() { c.UI.Error(command.Help()) }
+func (c *BaseCommand) InitFlagSet() {
+	c.hidden = flag.NewFlagSet("", flag.ContinueOnError)
+	c.FlagSet = flag.NewFlagSet("", flag.ContinueOnError)
 
 	if c.hasClientHTTP() {
 		c.httpFlagsClient().VisitAll(func(f *flag.Flag) {
-			c.flagSet.Var(f.Value, f.Name, f.DefValue)
+			c.FlagSet.Var(f.Value, f.Name, f.Usage)
 		})
 	}
 
 	if c.hasServerHTTP() {
 		c.httpFlagsServer().VisitAll(func(f *flag.Flag) {
-			c.flagSet.Var(f.Value, f.Name, f.DefValue)
+			c.FlagSet.Var(f.Value, f.Name, f.Usage)
 		})
 	}
 
@@ -164,24 +165,11 @@ func (c *BaseCommand) NewFlagSet(command cli.Command) *flag.FlagSet {
 			c.UI.Error(errScanner.Text())
 		}
 	}()
-	c.flagSet.SetOutput(errW)
-
-	return c.flagSet
-}
-
-// Parse is used to parse the underlying flag set.
-func (c *BaseCommand) Parse(args []string) error {
-	return c.flagSet.Parse(args)
+	c.FlagSet.SetOutput(errW)
 }
 
-// Help returns the help for this flagSet.
-func (c *BaseCommand) Help() string {
-	// Some commands with subcommands (kv/snapshot) call this without initializing
-	// any flags first, so exit early to avoid a panic
-	if c.flagSet == nil {
-		return ""
-	}
-	return c.helpFlagsFor(c.flagSet)
+func (c *BaseCommand) HelpCommand(msg string) string {
+	return strings.TrimSpace(msg + c.helpFlagsFor())
 }
 
 // hasClientHTTP returns true if this meta command contains client HTTP flags.
@@ -198,7 +186,11 @@ func (c *BaseCommand) hasServerHTTP() bool {
 // help output. This function is sad because there's no "merging" of command
 // line flags. We explicitly pull out our "common" options into another section
 // by doing string comparisons :(.
-func (c *BaseCommand) helpFlagsFor(f *flag.FlagSet) string {
+func (c *BaseCommand) helpFlagsFor() string {
+	if c.FlagSet == nil {
+		panic("FlagSet not initialized. Did you forget to call InitFlagSet()?")
+	}
+
 	httpFlagsClient := c.httpFlagsClient()
 	httpFlagsServer := c.httpFlagsServer()
 
@@ -220,7 +212,7 @@ func (c *BaseCommand) helpFlagsFor(f *flag.FlagSet) string {
 
 	if !c.HideNormalFlagsHelp {
 		firstCommand := true
-		f.VisitAll(func(f *flag.Flag) {
+		c.FlagSet.VisitAll(func(f *flag.Flag) {
 			// Skip HTTP flags as they will be grouped separately
 			if flagContains(httpFlagsClient, f) || flagContains(httpFlagsServer, f) {
 				return

command/catalog_command.go

@@ -1,8 +1,6 @@
 package command
 
 import (
-	"strings"
-
 	"github.com/mitchellh/cli"
 )
 
@@ -17,7 +15,8 @@ func (c *CatalogCommand) Run(args []string) int {
 }
 
 func (c *CatalogCommand) Help() string {
-	helpText := `
+	c.InitFlagSet()
+	return c.HelpCommand(`
 Usage: consul catalog <subcommand> [options] [args]
 
   This command has subcommands for interacting with Consul's catalog. The
@@ -41,8 +40,7 @@ Usage: consul catalog <subcommand> [options] [args]
 
   For more examples, ask for subcommand help or view the documentation.
 
-`
-	return strings.TrimSpace(helpText)
+`)
 }
 
 func (c *CatalogCommand) Synopsis() string {

command/catalog_list_datacenters.go

@@ -2,7 +2,6 @@ package command
 
 import (
 	"fmt"
-	"strings"
 
 	"github.com/mitchellh/cli"
 )
@@ -14,7 +13,8 @@ type CatalogListDatacentersCommand struct {
 }
 
 func (c *CatalogListDatacentersCommand) Help() string {
-	helpText := `
+	c.InitFlagSet()
+	return c.HelpCommand(`
 Usage: consul catalog datacenters [options]
 
   Retrieves the list of all known datacenters. This datacenters are sorted in
@@ -27,25 +27,22 @@ Usage: consul catalog datacenters [options]
 
   For a full list of options and examples, please see the Consul documentation.
 
-` + c.BaseCommand.Help()
-
-	return strings.TrimSpace(helpText)
+`)
 }
 
 func (c *CatalogListDatacentersCommand) Run(args []string) int {
-	f := c.BaseCommand.NewFlagSet(c)
-
-	if err := c.BaseCommand.Parse(args); err != nil {
+	c.InitFlagSet()
+	if err := c.FlagSet.Parse(args); err != nil {
 		return 1
 	}
 
-	if l := len(f.Args()); l > 0 {
+	if l := len(c.FlagSet.Args()); l > 0 {
 		c.UI.Error(fmt.Sprintf("Too many arguments (expected 0, got %d)", l))
 		return 1
 	}
 
 	// Create and test the HTTP client
-	client, err := c.BaseCommand.HTTPClient()
+	client, err := c.HTTPClient()
 	if err != nil {
 		c.UI.Error(fmt.Sprintf("Error connecting to Consul agent: %s", err))
 		return 1

command/catalog_list_nodes.go

@@ -16,10 +16,31 @@ var _ cli.Command = (*CatalogListNodesCommand)(nil)
 // nodes in the catalog.
 type CatalogListNodesCommand struct {
 	BaseCommand
+
+	// flags
+	detailed bool
+	near     string
+	nodeMeta map[string]string
+	service  string
+}
+
+func (c *CatalogListNodesCommand) initFlags() {
+	c.InitFlagSet()
+	c.FlagSet.BoolVar(&c.detailed, "detailed", false, "Output detailed information about "+
+		"the nodes including their addresses and metadata.")
+	c.FlagSet.StringVar(&c.near, "near", "", "Node name to sort the node list in ascending "+
+		"order based on estimated round-trip time from that node. "+
+		"Passing \"_agent\" will use this agent's node for sorting.")
+	c.FlagSet.Var((*configutil.FlagMapValue)(&c.nodeMeta), "node-meta", "Metadata to "+
+		"filter nodes with the given `key=value` pairs. This flag may be "+
+		"specified multiple times to filter on multiple sources of metadata.")
+	c.FlagSet.StringVar(&c.service, "service", "", "Service `id or name` to filter nodes. "+
+		"Only nodes which are providing the given service will be returned.")
 }
 
 func (c *CatalogListNodesCommand) Help() string {
-	helpText := `
+	c.initFlags()
+	return c.HelpCommand(`
 Usage: consul catalog nodes [options]
 
   Retrieves the list nodes registered in a given datacenter. By default, the
@@ -48,50 +69,32 @@ Usage: consul catalog nodes [options]
 
   For a full list of options and examples, please see the Consul documentation.
 
-` + c.BaseCommand.Help()
-
-	return strings.TrimSpace(helpText)
+`)
 }
 
 func (c *CatalogListNodesCommand) Run(args []string) int {
-	f := c.BaseCommand.NewFlagSet(c)
-
-	detailed := f.Bool("detailed", false, "Output detailed information about "+
-		"the nodes including their addresses and metadata.")
-
-	near := f.String("near", "", "Node name to sort the node list in ascending "+
-		"order based on estimated round-trip time from that node. "+
-		"Passing \"_agent\" will use this agent's node for sorting.")
-
-	nodeMeta := make(map[string]string)
-	f.Var((*configutil.FlagMapValue)(&nodeMeta), "node-meta", "Metadata to "+
-		"filter nodes with the given `key=value` pairs. This flag may be "+
-		"specified multiple times to filter on multiple sources of metadata.")
-
-	service := f.String("service", "", "Service `id or name` to filter nodes. "+
-		"Only nodes which are providing the given service will be returned.")
-
-	if err := c.BaseCommand.Parse(args); err != nil {
+	c.initFlags()
+	if err := c.FlagSet.Parse(args); err != nil {
 		return 1
 	}
 
-	if l := len(f.Args()); l > 0 {
+	if l := len(c.FlagSet.Args()); l > 0 {
 		c.UI.Error(fmt.Sprintf("Too many arguments (expected 0, got %d)", l))
 		return 1
 	}
 
 	// Create and test the HTTP client
-	client, err := c.BaseCommand.HTTPClient()
+	client, err := c.HTTPClient()
 	if err != nil {
 		c.UI.Error(fmt.Sprintf("Error connecting to Consul agent: %s", err))
 		return 1
 	}
 
 	var nodes []*api.Node
-	if *service != "" {
-		services, _, err := client.Catalog().Service(*service, "", &api.QueryOptions{
-			Near:     *near,
-			NodeMeta: nodeMeta,
+	if c.service != "" {
+		services, _, err := client.Catalog().Service(c.service, "", &api.QueryOptions{
+			Near:     c.near,
+			NodeMeta: c.nodeMeta,
 		})
 		if err != nil {
 			c.UI.Error(fmt.Sprintf("Error listing nodes for service: %s", err))
@@ -113,8 +116,8 @@ func (c *CatalogListNodesCommand) Run(args []string) int {
 		}
 	} else {
 		nodes, _, err = client.Catalog().Nodes(&api.QueryOptions{
-			Near:     *near,
-			NodeMeta: nodeMeta,
+			Near:     c.near,
+			NodeMeta: c.nodeMeta,
 		})
 		if err != nil {
 			c.UI.Error(fmt.Sprintf("Error listing nodes: %s", err))
@@ -128,7 +131,7 @@ func (c *CatalogListNodesCommand) Run(args []string) int {
 		return 0
 	}
 
-	output, err := printNodes(nodes, *detailed)
+	output, err := printNodes(nodes, c.detailed)
 	if err != nil {
 		c.UI.Error(fmt.Sprintf("Error printing nodes: %s", err))
 		return 1