From 6c1f51aa95e7966c49a5211ffabe337bf4d2f3fb Mon Sep 17 00:00:00 2001 From: Jesse Szwedko Date: Sun, 31 Jul 2016 14:14:46 -0700 Subject: [PATCH] Fix context.(Global)IsSet to respect environment variables This appeared to be the least messy approach to hack in support for IsSet also checking environment variables to see if a particular cli.Flag was set without making backwards incompatible changes to the interface. I intend to fix this more properly in v2, probably by adding another method to the cli.Flag interface to push the responsibility down as it occurred to me that it was really the `Flag`s themselves that offer support for configuration via the environment as opposed to the `context` or other supporting structures. This opens the door for the anything implementing the `Flag` interface to have additional sources of input while still supporting `context.IsSet`. --- context.go | 79 ++++++++++++++++++++++++++++++++++++++----------- context_test.go | 60 +++++++++++++++++++++++++++++++++++++ 2 files changed, 121 insertions(+), 18 deletions(-) diff --git a/context.go b/context.go index 14ad3f7..5ea4f8f 100644 --- a/context.go +++ b/context.go @@ -3,6 +3,8 @@ package cli import ( "errors" "flag" + "os" + "reflect" "strings" ) @@ -11,12 +13,11 @@ import ( // can be used to retrieve context-specific Args and // parsed command-line options. type Context struct { - App *App - Command Command - flagSet *flag.FlagSet - setFlags map[string]bool - globalSetFlags map[string]bool - parentContext *Context + App *App + Command Command + flagSet *flag.FlagSet + setFlags map[string]bool + parentContext *Context } // NewContext creates a new context. For use in when invoking an App or Command action. @@ -43,28 +44,70 @@ func (c *Context) GlobalSet(name, value string) error { func (c *Context) IsSet(name string) bool { if c.setFlags == nil { c.setFlags = make(map[string]bool) + c.flagSet.Visit(func(f *flag.Flag) { c.setFlags[f.Name] = true }) + + c.flagSet.VisitAll(func(f *flag.Flag) { + if _, ok := c.setFlags[f.Name]; ok { + return + } + c.setFlags[f.Name] = false + }) + + // XXX hack to support IsSet for flags with EnvVar + // + // There isn't an easy way to do this with the current implementation since + // whether a flag was set via an environment variable is very difficult to + // determine here. Instead, we intend to introduce a backwards incompatible + // change in version 2 to add `IsSet` to the Flag interface to push the + // responsibility closer to where the information required to determine + // whether a flag is set by non-standard means such as environment + // variables is avaliable. + // + // See https://github.com/urfave/cli/issues/294 for additional discussion + flags := c.Command.Flags + if c.Command.Name == "" { // cannot == Command{} since it contains slice types + if c.App != nil { + flags = c.App.Flags + } + } + for _, f := range flags { + eachName(f.GetName(), func(name string) { + if isSet, ok := c.setFlags[name]; isSet || !ok { + return + } + + envVars := reflect.ValueOf(f).FieldByName("EnvVar").String() + + eachName(envVars, func(envVar string) { + envVar = strings.TrimSpace(envVar) + if envVal := os.Getenv(envVar); envVal != "" { + c.setFlags[name] = true + return + } + }) + }) + } } - return c.setFlags[name] == true + + return c.setFlags[name] } // GlobalIsSet determines if the global flag was actually set func (c *Context) GlobalIsSet(name string) bool { - if c.globalSetFlags == nil { - c.globalSetFlags = make(map[string]bool) - ctx := c - if ctx.parentContext != nil { - ctx = ctx.parentContext - } - for ; ctx != nil && c.globalSetFlags[name] == false; ctx = ctx.parentContext { - ctx.flagSet.Visit(func(f *flag.Flag) { - c.globalSetFlags[f.Name] = true - }) + ctx := c + if ctx.parentContext != nil { + ctx = ctx.parentContext + } + + for ; ctx != nil; ctx = ctx.parentContext { + if ctx.IsSet(name) { + return true } } - return c.globalSetFlags[name] + return false } // FlagNames returns a slice of flag names used in this context. diff --git a/context_test.go b/context_test.go index 5c68fdd..0cf84d1 100644 --- a/context_test.go +++ b/context_test.go @@ -2,6 +2,7 @@ package cli import ( "flag" + "os" "testing" "time" ) @@ -180,6 +181,33 @@ func TestContext_IsSet(t *testing.T) { expect(t, c.IsSet("myflagGlobal"), false) } +// XXX Corresponds to hack in context.IsSet for flags with EnvVar field +// Should be moved to `flag_test` in v2 +func TestContext_IsSet_fromEnv(t *testing.T) { + var timeoutIsSet, tIsSet, noEnvVarIsSet, nIsSet bool + + os.Clearenv() + os.Setenv("APP_TIMEOUT_SECONDS", "15.5") + a := App{ + Flags: []Flag{ + Float64Flag{Name: "timeout, t", EnvVar: "APP_TIMEOUT_SECONDS"}, + Float64Flag{Name: "no-env-var, n"}, + }, + Action: func(ctx *Context) error { + timeoutIsSet = ctx.IsSet("timeout") + tIsSet = ctx.IsSet("t") + noEnvVarIsSet = ctx.IsSet("no-env-var") + nIsSet = ctx.IsSet("n") + return nil + }, + } + a.Run([]string{"run"}) + expect(t, timeoutIsSet, true) + expect(t, tIsSet, true) + expect(t, noEnvVarIsSet, false) + expect(t, nIsSet, false) +} + func TestContext_GlobalIsSet(t *testing.T) { set := flag.NewFlagSet("test", 0) set.Bool("myflag", false, "doc") @@ -199,6 +227,38 @@ func TestContext_GlobalIsSet(t *testing.T) { expect(t, c.GlobalIsSet("bogusGlobal"), false) } +// XXX Corresponds to hack in context.IsSet for flags with EnvVar field +// Should be moved to `flag_test` in v2 +func TestContext_GlobalIsSet_fromEnv(t *testing.T) { + var timeoutIsSet, tIsSet, noEnvVarIsSet, nIsSet bool + + os.Clearenv() + os.Setenv("APP_TIMEOUT_SECONDS", "15.5") + a := App{ + Flags: []Flag{ + Float64Flag{Name: "timeout, t", EnvVar: "APP_TIMEOUT_SECONDS"}, + Float64Flag{Name: "no-env-var, n"}, + }, + Commands: []Command{ + { + Name: "hello", + Action: func(ctx *Context) error { + timeoutIsSet = ctx.GlobalIsSet("timeout") + tIsSet = ctx.GlobalIsSet("t") + noEnvVarIsSet = ctx.GlobalIsSet("no-env-var") + nIsSet = ctx.GlobalIsSet("n") + return nil + }, + }, + }, + } + a.Run([]string{"run", "hello"}) + expect(t, timeoutIsSet, true) + expect(t, tIsSet, true) + expect(t, noEnvVarIsSet, false) + expect(t, nIsSet, false) +} + func TestContext_NumFlags(t *testing.T) { set := flag.NewFlagSet("test", 0) set.Bool("myflag", false, "doc")