Merge pull request #1643 from jonboulle/fix_flags

pkg: fix SetFlagsFromEnv behaviour

etcdmain/etcd.go

@@ -161,9 +161,11 @@ func Main() {
 		os.Exit(0)
 	}
 
-	flags.SetFlagsFromEnv(fs)
+	err := flags.SetFlagsFromEnv(fs)
+	if err != nil {
+		log.Fatalf("etcd: %v", err)
+	}
 
-	var err error
 	shouldProxy := proxyFlag.String() != proxyFlagOff
 	if !shouldProxy {
 		err = startEtcd()

pkg/flags/flag.go

@@ -85,7 +85,8 @@ func UsageWithIgnoredFlagsFunc(fs *flag.FlagSet, ignore []string) func() {
 // 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) {
+func SetFlagsFromEnv(fs *flag.FlagSet) error {
+	var err error
 	alreadySet := make(map[string]bool)
 	fs.Visit(func(f *flag.Flag) {
 		alreadySet[f.Name] = true
@@ -95,13 +96,13 @@ func SetFlagsFromEnv(fs *flag.FlagSet) {
 			key := "ETCD_" + strings.ToUpper(strings.Replace(f.Name, "-", "_", -1))
 			val := os.Getenv(key)
 			if val != "" {
-				if err := fs.Set(f.Name, val); err != nil {
-					// Should never happen
-					log.Panicf("error setting flag from env: %v", err)
+				if serr := fs.Set(f.Name, val); serr != nil {
+					err = fmt.Errorf("invalid value %q for %s: %v", val, key, serr)
 				}
 			}
 		}
 	})
+	return err
 }
 
 // URLsFromFlags decides what URLs should be using two different flags

pkg/flags/flag_test.go

@@ -58,7 +58,10 @@ func TestSetFlagsFromEnv(t *testing.T) {
 	}
 
 	// now read the env and verify flags were updated as expected
-	SetFlagsFromEnv(fs)
+	err := SetFlagsFromEnv(fs)
+	if err != nil {
+		t.Errorf("err=%v, want nil", err)
+	}
 	for f, want := range map[string]string{
 		"a": "foo",
 		"b": "bar",
@@ -70,6 +73,16 @@ func TestSetFlagsFromEnv(t *testing.T) {
 	}
 }
 
+func TestSetFlagsFromEnvBad(t *testing.T) {
+	// now verify that an error is propagated
+	fs := flag.NewFlagSet("testing", flag.ExitOnError)
+	fs.Int("x", 0, "")
+	os.Setenv("ETCD_X", "not_a_number")
+	if err := SetFlagsFromEnv(fs); err == nil {
+		t.Errorf("err=nil, want != nil")
+	}
+}
+
 func TestURLsFromFlags(t *testing.T) {
 	tests := []struct {
 		args     []string