[flagutil] Cleanup `flag` references (#11381)

This:
- Corrects an initial migration of the `DualFormat*` functions to
  _actually_ install the flag on the passed in flagset, rather than the
  global one.
- Updates `StringListVar` to fully-implement `pflag.Value`, mimicking
  pflag's `StringSlice` implementation for this method.
- Removes the compile-time type-check on `StringSetFlag` implementing
  `flag.Value` (which is a subset of `pflag.Value` anyway), as well as in
  the test code.

Closes #11296.

Signed-off-by: Andrew Mason <andrew@planetscale.com>

go/flags/endtoend/vttablet.txt

@@ -435,9 +435,9 @@ Usage of vttablet:
       --twopc_coordinator_address string                                 address of the (VTGate) process(es) that will be used to notify of abandoned transactions.
       --twopc_enable                                                     if the flag is on, 2pc is enabled. Other 2pc flags must be supplied.
       --tx-throttler-config string                                       Synonym to -tx_throttler_config (default "target_replication_lag_sec: 2\nmax_replication_lag_sec: 10\ninitial_rate: 100\nmax_increase: 1\nemergency_decrease: 0.5\nmin_duration_between_increases_sec: 40\nmax_duration_between_increases_sec: 62\nmin_duration_between_decreases_sec: 20\nspread_backlog_across_sec: 20\nage_bad_rate_after_sec: 180\nbad_rate_increase: 0.1\nmax_rate_approach_threshold: 0.9\n")
-      --tx-throttler-healthcheck-cells StringList                        Synonym to -tx_throttler_healthcheck_cells
+      --tx-throttler-healthcheck-cells strings                           Synonym to -tx_throttler_healthcheck_cells
       --tx_throttler_config string                                       The configuration of the transaction throttler as a text formatted throttlerdata.Configuration protocol buffer message (default "target_replication_lag_sec: 2\nmax_replication_lag_sec: 10\ninitial_rate: 100\nmax_increase: 1\nemergency_decrease: 0.5\nmin_duration_between_increases_sec: 40\nmax_duration_between_increases_sec: 62\nmin_duration_between_decreases_sec: 20\nspread_backlog_across_sec: 20\nage_bad_rate_after_sec: 180\nbad_rate_increase: 0.1\nmax_rate_approach_threshold: 0.9\n")
-      --tx_throttler_healthcheck_cells StringList                        A comma-separated list of cells. Only tabletservers running in these cells will be monitored for replication lag by the transaction throttler.
+      --tx_throttler_healthcheck_cells strings                           A comma-separated list of cells. Only tabletservers running in these cells will be monitored for replication lag by the transaction throttler.
       --unhealthy_threshold duration                                     replication lag after which a replica is considered unhealthy (default 2h0m0s)
       --use_super_read_only                                              Set super_read_only flag when performing planned failover.
   -v, --v Level                                                          log level for V logs

go/flagutil/flagutil.go

@@ -20,7 +20,6 @@ package flagutil
 
 import (
 	"errors"
-	"flag"
 	"fmt"
 	"sort"
 	"strconv"
@@ -86,14 +85,15 @@ func (value StringListValue) String() string {
 		parts[i] = strings.Replace(strings.Replace(v, "\\", "\\\\", -1), ",", `\,`, -1)
 	}
 	return strings.Join(parts, ",")
-
 }
 
+func (value StringListValue) Type() string { return "strings" }
+
 // StringListVar defines a []string flag with the specified name, value and usage
 // string. The argument 'p' points to a []string in which to store the value of the flag.
-func StringListVar(p *[]string, name string, defaultValue []string, usage string) {
+func StringListVar(fs *pflag.FlagSet, p *[]string, name string, defaultValue []string, usage string) {
 	*p = defaultValue
-	flag.Var((*StringListValue)(p), name, usage)
+	fs.Var((*StringListValue)(p), name, usage)
 }
 
 // StringMapValue is a map[string]string flag. It accepts a
@@ -143,9 +143,9 @@ func DualFormatStringListVar(fs *pflag.FlagSet, p *[]string, name string, value
 	dashes := strings.Replace(name, "_", "-", -1)
 	underscores := strings.Replace(name, "-", "_", -1)
 
-	StringListVar(p, underscores, value, usage)
+	StringListVar(fs, p, underscores, value, usage)
 	if dashes != underscores {
-		StringListVar(p, dashes, *p, fmt.Sprintf("Synonym to -%s", underscores))
+		StringListVar(fs, p, dashes, *p, fmt.Sprintf("Synonym to -%s", underscores))
 	}
 }
 
@@ -154,9 +154,9 @@ func DualFormatStringVar(fs *pflag.FlagSet, p *string, name string, value string
 	dashes := strings.Replace(name, "_", "-", -1)
 	underscores := strings.Replace(name, "-", "_", -1)
 
-	flag.StringVar(p, underscores, value, usage)
+	fs.StringVar(p, underscores, value, usage)
 	if dashes != underscores {
-		flag.StringVar(p, dashes, *p, fmt.Sprintf("Synonym to -%s", underscores))
+		fs.StringVar(p, dashes, *p, fmt.Sprintf("Synonym to -%s", underscores))
 	}
 }
 
@@ -165,9 +165,9 @@ func DualFormatInt64Var(fs *pflag.FlagSet, p *int64, name string, value int64, u
 	dashes := strings.Replace(name, "_", "-", -1)
 	underscores := strings.Replace(name, "-", "_", -1)
 
-	flag.Int64Var(p, underscores, value, usage)
+	fs.Int64Var(p, underscores, value, usage)
 	if dashes != underscores {
-		flag.Int64Var(p, dashes, *p, fmt.Sprintf("Synonym to -%s", underscores))
+		fs.Int64Var(p, dashes, *p, fmt.Sprintf("Synonym to -%s", underscores))
 	}
 }
 
@@ -176,9 +176,9 @@ func DualFormatIntVar(fs *pflag.FlagSet, p *int, name string, value int, usage s
 	dashes := strings.Replace(name, "_", "-", -1)
 	underscores := strings.Replace(name, "-", "_", -1)
 
-	flag.IntVar(p, underscores, value, usage)
+	fs.IntVar(p, underscores, value, usage)
 	if dashes != underscores {
-		flag.IntVar(p, dashes, *p, fmt.Sprintf("Synonym to -%s", underscores))
+		fs.IntVar(p, dashes, *p, fmt.Sprintf("Synonym to -%s", underscores))
 	}
 }
 
@@ -187,9 +187,9 @@ func DualFormatBoolVar(fs *pflag.FlagSet, p *bool, name string, value bool, usag
 	dashes := strings.Replace(name, "_", "-", -1)
 	underscores := strings.Replace(name, "-", "_", -1)
 
-	flag.BoolVar(p, underscores, value, usage)
+	fs.BoolVar(p, underscores, value, usage)
 	if dashes != underscores {
-		flag.BoolVar(p, dashes, *p, fmt.Sprintf("Synonym to -%s", underscores))
+		fs.BoolVar(p, dashes, *p, fmt.Sprintf("Synonym to -%s", underscores))
 	}
 }
 

go/flagutil/flagutil_test.go

@@ -17,18 +17,18 @@ limitations under the License.
 package flagutil
 
 import (
-	"flag"
 	"strings"
 	"testing"
 	"time"
 
+	"github.com/spf13/pflag"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
 func TestStringList(t *testing.T) {
 	p := StringListValue([]string{})
-	var _ flag.Value = &p
+	var _ pflag.Value = &p
 	wanted := map[string]string{
 		"0ala,ma,kota":   "0ala.ma.kota",
 		`1ala\,ma,kota`:  "1ala,ma.kota",
@@ -52,7 +52,7 @@ func TestStringList(t *testing.T) {
 // TestEmptyStringList verifies that an empty parameter results in an empty list
 func TestEmptyStringList(t *testing.T) {
 	var p StringListValue
-	var _ flag.Value = &p
+	var _ pflag.Value = &p
 	if err := p.Set(""); err != nil {
 		t.Fatalf("p.Set(\"\"): %v", err)
 	}
@@ -69,7 +69,7 @@ type pair struct {
 
 func TestStringMap(t *testing.T) {
 	v := StringMapValue(nil)
-	var _ flag.Value = &v
+	var _ pflag.Value = &v
 	wanted := []pair{
 		{
 			in:  "tag1:value1,tag2:value2",

go/flagutil/sets.go

@@ -17,17 +17,13 @@ limitations under the License.
 package flagutil
 
 import (
-	"flag"
 	"strings"
 
 	"github.com/spf13/pflag"
 	"k8s.io/apimachinery/pkg/util/sets"
 )
 
-var (
-	_ flag.Value  = (*StringSetFlag)(nil)
-	_ pflag.Value = (*StringSetFlag)(nil)
-)
+var _ pflag.Value = (*StringSetFlag)(nil)
 
 // StringSetFlag can be used to collect multiple instances of a flag into a set
 // of values.