Merge pull request #1167 from bcwaldon/move-flag-code

Move flag-related code into new pkg package

main.go

@@ -15,6 +15,7 @@ import (
 
 	"github.com/coreos/etcd/etcdserver"
 	"github.com/coreos/etcd/etcdserver/etcdhttp"
+	"github.com/coreos/etcd/pkg"
 	"github.com/coreos/etcd/proxy"
 	"github.com/coreos/etcd/raft"
 	"github.com/coreos/etcd/snap"
@@ -91,15 +92,15 @@ func init() {
 	flag.StringVar(&peerTLSInfo.KeyFile, "peer-key-file", "", "Path to the peer server TLS key file.")
 
 	for _, f := range deprecated {
-		flag.Var(&deprecatedFlag{f}, f, "No longer supported.")
+		flag.Var(&pkg.DeprecatedFlag{f}, f, "")
 	}
 }
 
 func main() {
-	flag.Usage = usageWithIgnoredFlagsFunc(flag.CommandLine, deprecated)
+	flag.Usage = pkg.UsageWithIgnoredFlagsFunc(flag.CommandLine, deprecated)
 	flag.Parse()
 
-	setFlagsFromEnv()
+	pkg.SetFlagsFromEnv(flag.CommandLine)
 
 	if string(*proxyFlag) == proxyFlagValueOff {
 		startEtcd()
@@ -332,61 +333,3 @@ func (pf *ProxyFlag) Set(s string) error {
 func (pf *ProxyFlag) String() string {
 	return string(*pf)
 }
-
-// setFlagsFromEnv parses all registered flags in the global flagset,
-// and if they are not already set it attempts to set their values from
-// environment variables. Environment variables take the name of the flag but
-// are UPPERCASE, have the prefix "ETCD_", and any dashes are replaced by
-// underscores - for example: some-flag => ETCD_SOME_FLAG
-func setFlagsFromEnv() {
-	alreadySet := make(map[string]bool)
-	flag.Visit(func(f *flag.Flag) {
-		alreadySet[f.Name] = true
-	})
-	flag.VisitAll(func(f *flag.Flag) {
-		if !alreadySet[f.Name] {
-			key := "ETCD_" + strings.ToUpper(strings.Replace(f.Name, "-", "_", -1))
-			val := os.Getenv(key)
-			if val != "" {
-				flag.Set(f.Name, val)
-			}
-		}
-
-	})
-}
-
-type deprecatedFlag struct {
-	name string
-}
-
-// IsBoolFlag is defined to allow the flag to be defined without an argument
-func (df *deprecatedFlag) IsBoolFlag() bool {
-	return true
-}
-
-func (df *deprecatedFlag) Set(s string) error {
-	log.Printf("WARNING: flag \"-%s\" is no longer supported.", df.name)
-	return nil
-}
-
-func (df *deprecatedFlag) String() string {
-	return ""
-}
-
-func usageWithIgnoredFlagsFunc(fs *flag.FlagSet, ignore []string) func() {
-	iMap := make(map[string]struct{}, len(ignore))
-	for _, name := range ignore {
-		iMap[name] = struct{}{}
-	}
-
-	return func() {
-		fs.VisitAll(func(f *flag.Flag) {
-			if _, ok := iMap[f.Name]; ok {
-				return
-			}
-
-			format := "  -%s=%s: %s\n"
-			fmt.Fprintf(os.Stderr, format, f.Name, f.DefValue, f.Usage)
-		})
-	}
-}

main_test.go

@@ -1,46 +1,8 @@
 package main
 
-import "os"
+import (
-import "flag"
+	"testing"
-import "testing"
+)
-
-func TestSetFlagsFromEnv(t *testing.T) {
-	os.Clearenv()
-	// flags should be settable using env vars
-	os.Setenv("ETCD_DATA_DIR", "/foo/bar")
-	// and command-line flags
-	if err := flag.Set("peer-bind-addr", "1.2.3.4"); err != nil {
-		t.Fatal(err)
-	}
-	// command-line flags take precedence over env vars
-	os.Setenv("ETCD_ID", "woof")
-	if err := flag.Set("id", "quack"); err != nil {
-		t.Fatal(err)
-	}
-
-	// first verify that flags are as expected before reading the env
-	for f, want := range map[string]string{
-		"data-dir":       "",
-		"peer-bind-addr": "1.2.3.4",
-		"id":             "quack",
-	} {
-		if got := flag.Lookup(f).Value.String(); got != want {
-			t.Fatalf("flag %q=%q, want %q", f, got, want)
-		}
-	}
-
-	// now read the env and verify flags were updated as expected
-	setFlagsFromEnv()
-	for f, want := range map[string]string{
-		"data-dir":       "/foo/bar",
-		"peer-bind-addr": "1.2.3.4",
-		"id":             "quack",
-	} {
-		if got := flag.Lookup(f).Value.String(); got != want {
-			t.Errorf("flag %q=%q, want %q", f, got, want)
-		}
-	}
-}
 
 func TestProxyFlagSet(t *testing.T) {
 	tests := []struct {

pkg/flag.go

@@ -0,0 +1,66 @@
+package pkg
+
+import (
+	"flag"
+	"fmt"
+	"log"
+	"os"
+	"strings"
+)
+
+type DeprecatedFlag struct {
+	Name string
+}
+
+// IsBoolFlag is defined to allow the flag to be defined without an argument
+func (df *DeprecatedFlag) IsBoolFlag() bool {
+	return true
+}
+
+func (df *DeprecatedFlag) Set(s string) error {
+	log.Printf("WARNING: flag \"-%s\" is no longer supported.", df.Name)
+	return nil
+}
+
+func (df *DeprecatedFlag) String() string {
+	return ""
+}
+
+func UsageWithIgnoredFlagsFunc(fs *flag.FlagSet, ignore []string) func() {
+	iMap := make(map[string]struct{}, len(ignore))
+	for _, name := range ignore {
+		iMap[name] = struct{}{}
+	}
+
+	return func() {
+		fs.VisitAll(func(f *flag.Flag) {
+			if _, ok := iMap[f.Name]; ok {
+				return
+			}
+
+			format := "  -%s=%s: %s\n"
+			fmt.Fprintf(os.Stderr, format, f.Name, f.DefValue, f.Usage)
+		})
+	}
+}
+
+// SetFlagsFromEnv parses all registered flags in the given flagset,
+// and if they are not already set it attempts to set their values from
+// environment variables. Environment variables take the name of the flag but
+// are UPPERCASE, have the prefix "ETCD_", and any dashes are replaced by
+// underscores - for example: some-flag => ETCD_SOME_FLAG
+func SetFlagsFromEnv(fs *flag.FlagSet) {
+	alreadySet := make(map[string]bool)
+	fs.Visit(func(f *flag.Flag) {
+		alreadySet[f.Name] = true
+	})
+	fs.VisitAll(func(f *flag.Flag) {
+		if !alreadySet[f.Name] {
+			key := "ETCD_" + strings.ToUpper(strings.Replace(f.Name, "-", "_", -1))
+			val := os.Getenv(key)
+			if val != "" {
+				fs.Set(f.Name, val)
+			}
+		}
+	})
+}

pkg/flag_test.go

@@ -0,0 +1,51 @@
+package pkg
+
+import (
+	"flag"
+	"os"
+	"testing"
+)
+
+func TestSetFlagsFromEnv(t *testing.T) {
+	fs := flag.NewFlagSet("testing", flag.ExitOnError)
+	fs.String("a", "", "")
+	fs.String("b", "", "")
+	fs.String("c", "", "")
+	fs.Parse([]string{})
+
+	os.Clearenv()
+	// flags should be settable using env vars
+	os.Setenv("ETCD_A", "foo")
+	// and command-line flags
+	if err := fs.Set("b", "bar"); err != nil {
+		t.Fatal(err)
+	}
+	// command-line flags take precedence over env vars
+	os.Setenv("ETCD_C", "woof")
+	if err := fs.Set("c", "quack"); err != nil {
+		t.Fatal(err)
+	}
+
+	// first verify that flags are as expected before reading the env
+	for f, want := range map[string]string{
+		"a": "",
+		"b": "bar",
+		"c": "quack",
+	} {
+		if got := fs.Lookup(f).Value.String(); got != want {
+			t.Fatalf("flag %q=%q, want %q", f, got, want)
+		}
+	}
+
+	// now read the env and verify flags were updated as expected
+	SetFlagsFromEnv(fs)
+	for f, want := range map[string]string{
+		"a": "foo",
+		"b": "bar",
+		"c": "quack",
+	} {
+		if got := fs.Lookup(f).Value.String(); got != want {
+			t.Errorf("flag %q=%q, want %q", f, got, want)
+		}
+	}
+}

test

@@ -15,7 +15,7 @@ COVER=${COVER:-"-cover"}
 source ./build
 
 # Hack: gofmt ./ will recursively check the .git directory. So use *.go for gofmt.
-TESTABLE_AND_FORMATTABLE="client etcdserver etcdserver/etcdhttp etcdserver/etcdserverpb functional proxy raft snap store wait wal transport"
+TESTABLE_AND_FORMATTABLE="client etcdserver etcdserver/etcdhttp etcdserver/etcdserverpb functional pkg proxy raft snap store transport wait wal"
 TESTABLE="$TESTABLE_AND_FORMATTABLE ./"
 FORMATTABLE="$TESTABLE_AND_FORMATTABLE *.go"