pkg/flags: clean up, add "SelectiveStringsValue"

Signed-off-by: Gyuho Lee <gyuhox@gmail.com>

etcdmain/config.go

@@ -86,9 +86,9 @@ type config struct {
 // configFlags has the set of flags used for command line parsing a Config
 type configFlags struct {
 	flagSet      *flag.FlagSet
-	clusterState *flags.StringsFlag
-	fallback     *flags.StringsFlag
-	proxy        *flags.StringsFlag
+	clusterState *flags.SelectiveStringValue
+	fallback     *flags.SelectiveStringValue
+	proxy        *flags.SelectiveStringValue
 }
 
 func newConfig() *config {
@@ -105,15 +105,15 @@ func newConfig() *config {
 	}
 	cfg.cf = configFlags{
 		flagSet: flag.NewFlagSet("etcd", flag.ContinueOnError),
-		clusterState: flags.NewStringsFlag(
+		clusterState: flags.NewSelectiveStringValue(
 			embed.ClusterStateFlagNew,
 			embed.ClusterStateFlagExisting,
 		),
-		fallback: flags.NewStringsFlag(
+		fallback: flags.NewSelectiveStringValue(
 			fallbackFlagProxy,
 			fallbackFlagExit,
 		),
-		proxy: flags.NewStringsFlag(
+		proxy: flags.NewSelectiveStringValue(
 			proxyFlagOff,
 			proxyFlagReadonly,
 			proxyFlagOn,
@@ -151,7 +151,7 @@ func newConfig() *config {
 	fs.Var(flags.NewURLsValue(embed.DefaultInitialAdvertisePeerURLs), "initial-advertise-peer-urls", "List of this member's peer URLs to advertise to the rest of the cluster.")
 	fs.Var(flags.NewURLsValue(embed.DefaultAdvertiseClientURLs), "advertise-client-urls", "List of this member's client URLs to advertise to the public.")
 	fs.StringVar(&cfg.ec.Durl, "discovery", cfg.ec.Durl, "Discovery URL used to bootstrap the cluster.")
-	fs.Var(cfg.cf.fallback, "discovery-fallback", fmt.Sprintf("Valid values include %s", strings.Join(cfg.cf.fallback.Values, ", ")))
+	fs.Var(cfg.cf.fallback, "discovery-fallback", fmt.Sprintf("Valid values include %q", cfg.cf.fallback.Valids()))
 
 	fs.StringVar(&cfg.ec.Dproxy, "discovery-proxy", cfg.ec.Dproxy, "HTTP proxy to use for traffic to discovery service.")
 	fs.StringVar(&cfg.ec.DNSCluster, "discovery-srv", cfg.ec.DNSCluster, "DNS domain used to bootstrap initial cluster.")
@@ -165,7 +165,7 @@ func newConfig() *config {
 	fs.StringVar(&cfg.ec.ExperimentalEnableV2V3, "experimental-enable-v2v3", cfg.ec.ExperimentalEnableV2V3, "v3 prefix for serving emulated v2 state.")
 
 	// proxy
-	fs.Var(cfg.cf.proxy, "proxy", fmt.Sprintf("Valid values include %s", strings.Join(cfg.cf.proxy.Values, ", ")))
+	fs.Var(cfg.cf.proxy, "proxy", fmt.Sprintf("Valid values include %q", cfg.cf.proxy.Valids()))
 
 	fs.UintVar(&cfg.cp.ProxyFailureWaitMs, "proxy-failure-wait", cfg.cp.ProxyFailureWaitMs, "Time (in milliseconds) an endpoint will be held in a failed state.")
 	fs.UintVar(&cfg.cp.ProxyRefreshIntervalMs, "proxy-refresh-interval", cfg.cp.ProxyRefreshIntervalMs, "Time (in milliseconds) of the endpoints refresh interval.")
@@ -189,7 +189,7 @@ func newConfig() *config {
 	fs.BoolVar(&cfg.ec.PeerAutoTLS, "peer-auto-tls", false, "Peer TLS using generated certificates")
 	fs.StringVar(&cfg.ec.PeerTLSInfo.CRLFile, "peer-crl-file", "", "Path to the peer certificate revocation list file.")
 	fs.StringVar(&cfg.ec.PeerTLSInfo.AllowedCN, "peer-cert-allowed-cn", "", "Allowed CN for inter peer authentication.")
-	fs.Var(flags.NewStringSlice(""), "host-whitelist", "Comma-separated acceptable hostnames from HTTP client requests, if server is not secure (empty means allow all).")
+	fs.Var(flags.NewStringsValue(""), "host-whitelist", "Comma-separated acceptable hostnames from HTTP client requests, if server is not secure (empty means allow all).")
 
 	// logging
 	fs.BoolVar(&cfg.ec.Debug, "debug", false, "Enable debug-level logging for etcd.")
@@ -268,7 +268,7 @@ func (cfg *config) configFromCmdLine() error {
 	cfg.ec.APUrls = flags.URLsFromFlag(cfg.cf.flagSet, "initial-advertise-peer-urls")
 	cfg.ec.LCUrls = flags.URLsFromFlag(cfg.cf.flagSet, "listen-client-urls")
 	cfg.ec.ACUrls = flags.URLsFromFlag(cfg.cf.flagSet, "advertise-client-urls")
-	cfg.ec.HostWhitelist = flags.StringSliceFromFlag(cfg.cf.flagSet, "host-whitelist")
+	cfg.ec.HostWhitelist = flags.StringsFromFlag(cfg.cf.flagSet, "host-whitelist")
 	cfg.ec.ListenMetricsUrls = flags.URLsFromFlag(cfg.cf.flagSet, "listen-metrics-urls")
 
 	cfg.ec.ClusterState = cfg.cf.clusterState.String()

pkg/flags/flag.go

@@ -18,7 +18,6 @@ package flags
 import (
 	"flag"
 	"fmt"
-	"net/url"
 	"os"
 	"strings"
 
@@ -26,44 +25,7 @@ import (
 	"github.com/spf13/pflag"
 )
 
-var (
-	plog = capnslog.NewPackageLogger("github.com/coreos/etcd", "pkg/flags")
-)
-
-// DeprecatedFlag encapsulates a flag that may have been previously valid but
-// is now deprecated. If a DeprecatedFlag is set, an error occurs.
-type DeprecatedFlag struct {
-	Name string
-}
-
-func (f *DeprecatedFlag) Set(_ string) error {
-	return fmt.Errorf(`flag "-%s" is no longer supported.`, f.Name)
-}
-
-func (f *DeprecatedFlag) String() string {
-	return ""
-}
-
-// IgnoredFlag encapsulates a flag that may have been previously valid but is
-// now ignored. If an IgnoredFlag is set, a warning is printed and
-// operation continues.
-type IgnoredFlag struct {
-	Name string
-}
-
-// IsBoolFlag is defined to allow the flag to be defined without an argument
-func (f *IgnoredFlag) IsBoolFlag() bool {
-	return true
-}
-
-func (f *IgnoredFlag) Set(s string) error {
-	plog.Warningf(`flag "-%s" is no longer supported - ignoring.`, f.Name)
-	return nil
-}
-
-func (f *IgnoredFlag) String() string {
-	return ""
-}
+var plog = capnslog.NewPackageLogger("github.com/coreos/etcd", "pkg/flags")
 
 // SetFlagsFromEnv parses all registered flags in the given flagset,
 // and if they are not already set it attempts to set their values from
@@ -148,16 +110,6 @@ func setFlagFromEnv(fs flagSetter, prefix, fname string, usedEnvKey, alreadySet
 	return nil
 }
 
-// URLsFromFlag returns a slices from url got from the flag.
-func URLsFromFlag(fs *flag.FlagSet, urlsFlagName string) []url.URL {
-	return []url.URL(*fs.Lookup(urlsFlagName).Value.(*URLsValue))
-}
-
-// StringSliceFromFlag returns a string slice from the flag.
-func StringSliceFromFlag(fs *flag.FlagSet, flagName string) []string {
-	return []string(*fs.Lookup(flagName).Value.(*StringSlice))
-}
-
 func IsSet(fs *flag.FlagSet, name string) bool {
 	set := false
 	fs.Visit(func(f *flag.Flag) {

pkg/flags/string_slice_test.go → pkg/flags/ignored.go

@@ -14,24 +14,23 @@
 
 package flags
 
-import (
-	"reflect"
-	"testing"
-)
+// IgnoredFlag encapsulates a flag that may have been previously valid but is
+// now ignored. If an IgnoredFlag is set, a warning is printed and
+// operation continues.
+type IgnoredFlag struct {
+	Name string
+}
+
+// IsBoolFlag is defined to allow the flag to be defined without an argument
+func (f *IgnoredFlag) IsBoolFlag() bool {
+	return true
+}
+
+func (f *IgnoredFlag) Set(s string) error {
+	plog.Warningf(`flag "-%s" is no longer supported - ignoring.`, f.Name)
+	return nil
+}
 
-func TestStringSlice(t *testing.T) {
-	tests := []struct {
-		s   string
-		exp []string
-	}{
-		{s: "a,b,c", exp: []string{"a", "b", "c"}},
-		{s: "a, b,c", exp: []string{"a", " b", "c"}},
-		{s: "", exp: []string{}},
-	}
-	for i := range tests {
-		ss := []string(*NewStringSlice(tests[i].s))
-		if !reflect.DeepEqual(tests[i].exp, ss) {
-			t.Fatalf("#%d: expected %q, got %q", i, tests[i].exp, ss)
-		}
-	}
+func (f *IgnoredFlag) String() string {
+	return ""
 }

pkg/flags/selective_string.go

@@ -0,0 +1,114 @@
+// Copyright 2018 The etcd Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package flags
+
+import (
+	"errors"
+	"fmt"
+	"sort"
+	"strings"
+)
+
+// SelectiveStringValue implements the flag.Value interface.
+type SelectiveStringValue struct {
+	v      string
+	valids map[string]struct{}
+}
+
+// Set verifies the argument to be a valid member of the allowed values
+// before setting the underlying flag value.
+func (ss *SelectiveStringValue) Set(s string) error {
+	if _, ok := ss.valids[s]; ok {
+		ss.v = s
+		return nil
+	}
+	return errors.New("invalid value")
+}
+
+// String returns the set value (if any) of the SelectiveStringValue
+func (ss *SelectiveStringValue) String() string {
+	return ss.v
+}
+
+// Valids returns the list of valid strings.
+func (ss *SelectiveStringValue) Valids() []string {
+	s := make([]string, 0, len(ss.valids))
+	for k := range ss.valids {
+		s = append(s, k)
+	}
+	sort.Strings(s)
+	return s
+}
+
+// NewSelectiveStringValue creates a new string flag
+// for which any one of the given strings is a valid value,
+// and any other value is an error.
+//
+// valids[0] will be default value. Caller must be sure
+// len(valids) != 0 or it will panic.
+func NewSelectiveStringValue(valids ...string) *SelectiveStringValue {
+	vm := make(map[string]struct{})
+	for _, v := range valids {
+		vm[v] = struct{}{}
+	}
+	return &SelectiveStringValue{valids: vm, v: valids[0]}
+}
+
+// SelectiveStringsValue implements the flag.Value interface.
+type SelectiveStringsValue struct {
+	vs     []string
+	valids map[string]struct{}
+}
+
+// Set verifies the argument to be a valid member of the allowed values
+// before setting the underlying flag value.
+func (ss *SelectiveStringsValue) Set(s string) error {
+	vs := strings.Split(s, ",")
+	for i := range vs {
+		if _, ok := ss.valids[vs[i]]; ok {
+			ss.vs = append(ss.vs, vs[i])
+		} else {
+			return fmt.Errorf("invalid value %q", vs[i])
+		}
+	}
+	sort.Strings(ss.vs)
+	return nil
+}
+
+// String returns the set value (if any) of the SelectiveStringsValue.
+func (ss *SelectiveStringsValue) String() string {
+	return strings.Join(ss.vs, ",")
+}
+
+// Valids returns the list of valid strings.
+func (ss *SelectiveStringsValue) Valids() []string {
+	s := make([]string, 0, len(ss.valids))
+	for k := range ss.valids {
+		s = append(s, k)
+	}
+	sort.Strings(s)
+	return s
+}
+
+// NewSelectiveStringsValue creates a new string slice flag
+// for which any one of the given strings is a valid value,
+// and any other value is an error.
+func NewSelectiveStringsValue(valids ...string) *SelectiveStringsValue {
+	vm := make(map[string]struct{})
+	for _, v := range valids {
+		vm[v] = struct{}{}
+	}
+	return &SelectiveStringsValue{valids: vm, vs: []string{}}
+}

pkg/flags/selective_string_test.go

@@ -0,0 +1,70 @@
+// Copyright 2018 The etcd Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package flags
+
+import (
+	"testing"
+)
+
+func TestSelectiveStringValue(t *testing.T) {
+	tests := []struct {
+		vals []string
+
+		val  string
+		pass bool
+	}{
+		// known values
+		{[]string{"abc", "def"}, "abc", true},
+		{[]string{"on", "off", "false"}, "on", true},
+
+		// unrecognized values
+		{[]string{"abc", "def"}, "ghi", false},
+		{[]string{"on", "off"}, "", false},
+	}
+	for i, tt := range tests {
+		sf := NewSelectiveStringValue(tt.vals...)
+		if sf.v != tt.vals[0] {
+			t.Errorf("#%d: want default val=%v,but got %v", i, tt.vals[0], sf.v)
+		}
+		err := sf.Set(tt.val)
+		if tt.pass != (err == nil) {
+			t.Errorf("#%d: want pass=%t, but got err=%v", i, tt.pass, err)
+		}
+	}
+}
+
+func TestSelectiveStringsValue(t *testing.T) {
+	tests := []struct {
+		vals []string
+
+		val  string
+		pass bool
+	}{
+		{[]string{"abc", "def"}, "abc", true},
+		{[]string{"abc", "def"}, "abc,def", true},
+		{[]string{"abc", "def"}, "abc, def", false},
+		{[]string{"on", "off", "false"}, "on,false", true},
+		{[]string{"abc", "def"}, "ghi", false},
+		{[]string{"on", "off"}, "", false},
+		{[]string{"a", "b", "c", "d", "e"}, "a,c,e", true},
+	}
+	for i, tt := range tests {
+		sf := NewSelectiveStringsValue(tt.vals...)
+		err := sf.Set(tt.val)
+		if tt.pass != (err == nil) {
+			t.Errorf("#%d: want pass=%t, but got err=%v", i, tt.pass, err)
+		}
+	}
+}

pkg/flags/string_slice.go

@@ -1,44 +0,0 @@
-// Copyright 2018 The etcd Authors
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//     http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package flags
-
-import (
-	"sort"
-	"strings"
-)
-
-// StringSlice wraps "sort.StringSlice".
-type StringSlice sort.StringSlice
-
-// Set parses a command line set of strings, separated by comma.
-func (ss *StringSlice) Set(s string) error {
-	*ss = strings.Split(s, ",")
-	return nil
-}
-
-func (ss *StringSlice) String() string { return strings.Join(*ss, ",") }
-
-// NewStringSlice implements string slice as flag.Value interface.
-// Given value is to be separated by comma.
-func NewStringSlice(s string) (ss *StringSlice) {
-	if s == "" {
-		return &StringSlice{}
-	}
-	ss = new(StringSlice)
-	if err := ss.Set(s); err != nil {
-		plog.Panicf("new StringSlice should never fail: %v", err)
-	}
-	return ss
-}

pkg/flags/strings.go

@@ -1,4 +1,4 @@
-// Copyright 2015 The etcd Authors
+// Copyright 2018 The etcd Authors
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -14,36 +14,39 @@
 
 package flags
 
-import "errors"
+import (
+	"flag"
+	"sort"
+	"strings"
+)
 
-// NewStringsFlag creates a new string flag for which any one of the given
-// strings is a valid value, and any other value is an error.
-//
-// valids[0] will be default value. Caller must be sure len(valids)!=0 or
-// it will panic.
-func NewStringsFlag(valids ...string) *StringsFlag {
-	return &StringsFlag{Values: valids, val: valids[0]}
-}
+// StringsValue wraps "sort.StringSlice".
+type StringsValue sort.StringSlice
 
-// StringsFlag implements the flag.Value interface.
-type StringsFlag struct {
-	Values []string
-	val    string
+// Set parses a command line set of strings, separated by comma.
+// Implements "flag.Value" interface.
+func (ss *StringsValue) Set(s string) error {
+	*ss = strings.Split(s, ",")
+	return nil
 }
 
-// Set verifies the argument to be a valid member of the allowed values
-// before setting the underlying flag value.
-func (ss *StringsFlag) Set(s string) error {
-	for _, v := range ss.Values {
-		if s == v {
-			ss.val = s
-			return nil
-		}
+// String implements "flag.Value" interface.
+func (ss *StringsValue) String() string { return strings.Join(*ss, ",") }
+
+// NewStringsValue implements string slice as "flag.Value" interface.
+// Given value is to be separated by comma.
+func NewStringsValue(s string) (ss *StringsValue) {
+	if s == "" {
+		return &StringsValue{}
+	}
+	ss = new(StringsValue)
+	if err := ss.Set(s); err != nil {
+		plog.Panicf("new StringsValue should never fail: %v", err)
 	}
-	return errors.New("invalid value")
+	return ss
 }
 
-// String returns the set value (if any) of the StringsFlag
-func (ss *StringsFlag) String() string {
-	return ss.val
+// StringsFromFlag returns a string slice from the flag.
+func StringsFromFlag(fs *flag.FlagSet, flagName string) []string {
+	return []string(*fs.Lookup(flagName).Value.(*StringsValue))
 }

pkg/flags/strings_test.go

@@ -1,4 +1,4 @@
-// Copyright 2015 The etcd Authors
+// Copyright 2018 The etcd Authors
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -15,34 +15,23 @@
 package flags
 
 import (
+	"reflect"
 	"testing"
 )
 
-func TestStringsSet(t *testing.T) {
+func TestStringsValue(t *testing.T) {
 	tests := []struct {
-		vals []string
-
-		val  string
-		pass bool
+		s   string
+		exp []string
 	}{
-		// known values
-		{[]string{"abc", "def"}, "abc", true},
-		{[]string{"on", "off", "false"}, "on", true},
-
-		// unrecognized values
-		{[]string{"abc", "def"}, "ghi", false},
-		{[]string{"on", "off"}, "", false},
+		{s: "a,b,c", exp: []string{"a", "b", "c"}},
+		{s: "a, b,c", exp: []string{"a", " b", "c"}},
+		{s: "", exp: []string{}},
 	}
-
-	for i, tt := range tests {
-		sf := NewStringsFlag(tt.vals...)
-		if sf.val != tt.vals[0] {
-			t.Errorf("#%d: want default val=%v,but got %v", i, tt.vals[0], sf.val)
-		}
-
-		err := sf.Set(tt.val)
-		if tt.pass != (err == nil) {
-			t.Errorf("#%d: want pass=%t, but got err=%v", i, tt.pass, err)
+	for i := range tests {
+		ss := []string(*NewStringsValue(tests[i].s))
+		if !reflect.DeepEqual(tests[i].exp, ss) {
+			t.Fatalf("#%d: expected %q, got %q", i, tests[i].exp, ss)
 		}
 	}
 }

pkg/flags/urls.go

@@ -15,6 +15,8 @@
 package flags
 
 import (
+	"flag"
+	"net/url"
 	"strings"
 
 	"github.com/coreos/etcd/pkg/types"
@@ -25,6 +27,7 @@ type URLsValue types.URLs
 
 // Set parses a command line set of URLs formatted like:
 // http://127.0.0.1:2380,http://10.1.1.2:80
+// Implements "flag.Value" interface.
 func (us *URLsValue) Set(s string) error {
 	ss, err := types.NewURLs(strings.Split(s, ","))
 	if err != nil {
@@ -34,6 +37,7 @@ func (us *URLsValue) Set(s string) error {
 	return nil
 }
 
+// String implements "flag.Value" interface.
 func (us *URLsValue) String() string {
 	all := make([]string, len(*us))
 	for i, u := range *us {
@@ -54,3 +58,8 @@ func NewURLsValue(s string) *URLsValue {
 	}
 	return v
 }
+
+// URLsFromFlag returns a slices from url got from the flag.
+func URLsFromFlag(fs *flag.FlagSet, urlsFlagName string) []url.URL {
+	return []url.URL(*fs.Lookup(urlsFlagName).Value.(*URLsValue))
+}