From 0144a76db50bcdcbed50ccca324e78d56d0b31f3 Mon Sep 17 00:00:00 2001 From: Michal Fojtik Date: Thu, 7 Apr 2016 12:25:18 +0200 Subject: [PATCH] Use cobra for parsing multiple env variables --- cmd/s2i/main.go | 16 ++------ docs/cli.md | 4 +- pkg/api/describe/describer.go | 9 ++--- pkg/api/types.go | 42 +++++++++++++++++++- pkg/api/types_test.go | 37 +++++++++++++++++ pkg/build/strategies/onbuild/onbuild_test.go | 5 ++- pkg/build/strategies/sti/sti.go | 6 +-- pkg/build/strategies/sti/sti_test.go | 12 +++--- pkg/cmd/util.go | 20 ---------- 9 files changed, 100 insertions(+), 51 deletions(-) diff --git a/cmd/s2i/main.go b/cmd/s2i/main.go index fe7eb2ef2..d51de05e3 100644 --- a/cmd/s2i/main.go +++ b/cmd/s2i/main.go @@ -112,22 +112,17 @@ $ s2i build . centos/ruby-22-centos7 hello-world-app } } - cfg.Environment = map[string]string{} if len(cfg.EnvironmentFile) > 0 { result, err := util.ReadEnvironmentFile(cfg.EnvironmentFile) if err != nil { glog.Warningf("Unable to read environment file %q: %v", cfg.EnvironmentFile, err) } else { - cfg.Environment = result + for name, value := range result { + cfg.Environment = append(cfg.Environment, api.EnvironmentSpec{Name: name, Value: value}) + } } } - envs, err := cmdutil.ParseEnvs(cmd, "env") - checkErr(err) - for k, v := range envs { - cfg.Environment[k] = v - } - if len(oldScriptsFlag) != 0 { glog.Warning("DEPRECATED: Flag --scripts is deprecated, use --scripts-url instead") cfg.ScriptsURL = oldScriptsFlag @@ -168,7 +163,7 @@ $ s2i build . centos/ruby-22-centos7 hello-world-app buildCmd.Flags().BoolVar(&(cfg.RunImage), "run", false, "Run resulting image as part of invocation of this command") buildCmd.Flags().BoolVar(&(cfg.DisableRecursive), "recursive", true, "Fetch all git submodules when cloning application repository") - buildCmd.Flags().StringP("env", "e", "", "Specify an environment var NAME=VALUE,NAME2=VALUE2,...") + buildCmd.Flags().VarP(&(cfg.Environment), "env", "e", "Specify an single environment variable in NAME=VALUE format") buildCmd.Flags().StringVarP(&(cfg.Ref), "ref", "r", "", "Specify a ref to check-out") buildCmd.Flags().StringVarP(&(cfg.AssembleUser), "assemble-user", "", "", "Specify the user to run assemble with") buildCmd.Flags().StringVarP(&(cfg.ContextDir), "context-dir", "", "", "Specify the sub-directory inside the repository with the application sources") @@ -299,9 +294,6 @@ func newCmdUsage(cfg *api.Config) *cobra.Command { cfg.Usage = true cfg.BuilderImage = args[0] - envs, err := cmdutil.ParseEnvs(cmd, "env") - checkErr(err) - cfg.Environment = envs if len(oldScriptsFlag) != 0 { glog.Warning("DEPRECATED: Flag --scripts is deprecated, use --scripts-url instead") diff --git a/docs/cli.md b/docs/cli.md index 8e26a8259..4c70b0e25 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -76,7 +76,7 @@ that image and add them to the tar streamed to the container into `/artifacts`. | `-d (--destination)` | Location where the scripts and sources will be placed prior doing build (see [S2I Scripts](https://github.com/openshift/source-to-image/blob/master/docs/builder_image.md#s2i-scripts)) | | `--dockercfg-path` | The path to the Docker configuration file | | `--incremental` | Try to perform an incremental build | -| `-e (--env)` | Environment variables to be passed to the builder eg. `NAME=VALUE,NAME2=VALUE2,...` | +| `-e (--env)` | Environment variable to be passed to the builder eg. `NAME=VALUE` | | `-E (--environment-file)` | Specify the path to the file with environment | | `--exclude` | Regular expression for selecting files from the source tree to exclude from the build, where the default excludes the '.git' directory (see https://golang.org/pkg/regexp for syntax, but note that \"\" will be interpreted as allow all files and exclude no files) | | `--force-pull` | Always pull the builder image, even if it is present locally (defaults to true) | @@ -216,7 +216,7 @@ $ s2i usage [flags] | Name | Description | |:-------------------------- |:--------------------------------------------------------| | `-d (--destination)` | Location where the scripts and sources will be placed prior invoking usage (see [S2I Scripts](https://github.com/openshift/source-to-image/blob/master/docs/builder_image.md#s2i-scripts))| -| `-e (--env)` | Environment variables passed to the builder eg. `NAME=VALUE,NAME2=VALUE2,...`) | +| `-e (--env)` | Environment variable passed to the builder eg. `NAME=VALUE`) | | `--force-pull` | Always pull the builder image, even if it is present locally | | `--save-temp-dir` | Save the working directory used for fetching scripts and sources | | `-s (--scripts-url)` | URL of S2I scripts (see [Scripts URL](https://github.com/openshift/source-to-image/blob/master/docs/builder_image.md#s2i-scripts))| diff --git a/pkg/api/describe/describer.go b/pkg/api/describe/describer.go index 991011dc3..d0f54b7ba 100644 --- a/pkg/api/describe/describer.go +++ b/pkg/api/describe/describer.go @@ -102,13 +102,10 @@ func describeBuilderImage(config *api.Config, image string, out io.Writer) { } } -func printEnv(out io.Writer, env map[string]string) { - if len(env) == 0 { - return - } +func printEnv(out io.Writer, env api.EnvironmentList) { result := []string{} - for k, v := range env { - result = append(result, fmt.Sprintf("%s=%s", k, v)) + for _, e := range env { + result = append(result, strings.Join([]string{e.Name, e.Value}, "=")) } fmt.Fprintf(out, "Environment:\t%s\n", strings.Join(result, ",")) } diff --git a/pkg/api/types.go b/pkg/api/types.go index 51f11df2d..15e4b391f 100644 --- a/pkg/api/types.go +++ b/pkg/api/types.go @@ -7,6 +7,7 @@ import ( "strings" docker "github.com/fsouza/go-dockerclient" + "github.com/golang/glog" "github.com/openshift/source-to-image/pkg/util/user" ) @@ -117,7 +118,7 @@ type Config struct { RemovePreviousImage bool // Environment is a map of environment variables to be passed to the image. - Environment map[string]string + Environment EnvironmentList // EnvironmentFile provides the path to a file with list of environment // variables. @@ -199,6 +200,15 @@ type Config struct { DisableImplicitBuild bool } +// EnvironmentSpec specifies a single environment variable. +type EnvironmentSpec struct { + Name string + Value string +} + +// EnvironmentList contains list of environment variables. +type EnvironmentList []EnvironmentSpec + type ProxyConfig struct { HTTPProxy *url.URL HTTPSProxy *url.URL @@ -424,3 +434,33 @@ func (il *InjectionList) String() string { func (il *InjectionList) Type() string { return "string" } + +// Set implements the Set() function of pflags.Value interface. +func (e *EnvironmentList) Set(value string) error { + parts := strings.SplitN(value, "=", 2) + if len(parts) != 2 || len(parts[0]) == 0 { + return fmt.Errorf("invalid environment format %q, must be NAME=VALUE", value) + } + if strings.Contains(parts[1], ",") && strings.Contains(parts[1], "=") { + glog.Warningf("DEPRECATED: Use multiple -e flags to specify multiple environment variables instead of comma (%q)", parts[1]) + } + *e = append(*e, EnvironmentSpec{ + Name: strings.TrimSpace(parts[0]), + Value: strings.TrimSpace(parts[1]), + }) + return nil +} + +// String implements the String() function of pflags.Value interface. +func (e *EnvironmentList) String() string { + result := []string{} + for _, i := range *e { + result = append(result, strings.Join([]string{i.Name, i.Value}, "=")) + } + return strings.Join(result, ",") +} + +// Type implements the Type() function of pflags.Value interface. +func (e *EnvironmentList) Type() string { + return "string" +} diff --git a/pkg/api/types_test.go b/pkg/api/types_test.go index ba801e515..d52c9b99c 100644 --- a/pkg/api/types_test.go +++ b/pkg/api/types_test.go @@ -43,3 +43,40 @@ func TestInjectionListSet(t *testing.T) { } } } + +func TestEnvironmentSet(t *testing.T) { + table := map[string][]EnvironmentSpec{ + "FOO=bar": {{Name: "FOO", Value: "bar"}}, + "FOO=": {{Name: "FOO", Value: ""}}, + "FOO": {}, + "=": {}, + "FOO=bar,": {{Name: "FOO", Value: "bar,"}}, + // Users should get a deprecation warning in this case + // TODO: Create fake glog interface to be able to verify this. + "FOO=bar,BAR=foo": {{Name: "FOO", Value: "bar,BAR=foo"}}, + } + + for v, expected := range table { + got := EnvironmentList{} + err := got.Set(v) + if len(expected) == 0 && err == nil { + t.Errorf("Expected error for env %q", v) + continue + } + if len(expected) != len(got) { + t.Errorf("got %d items, expected %d items in the list for %q", len(got), len(expected), v) + continue + } + for _, exp := range expected { + found := false + for _, g := range got { + if g.Name == exp.Name && g.Value == exp.Value { + found = true + } + } + if !found { + t.Errorf("Expected %+v environment found in %#v list", exp, got) + } + } + } +} diff --git a/pkg/build/strategies/onbuild/onbuild_test.go b/pkg/build/strategies/onbuild/onbuild_test.go index 8e4c46a05..72e61a4ba 100644 --- a/pkg/build/strategies/onbuild/onbuild_test.go +++ b/pkg/build/strategies/onbuild/onbuild_test.go @@ -63,7 +63,10 @@ func checkDockerfile(fs *test.FakeFileSystem, t *testing.T) { func TestCreateDockerfile(t *testing.T) { fakeRequest := &api.Config{ BuilderImage: "fake:onbuild", - Environment: map[string]string{"FOO": "BAR", "TEST": "A VALUE"}, + Environment: api.EnvironmentList{ + {Name: "FOO", Value: "BAR"}, + {Name: "TEST", Value: "A VALUE"}, + }, } b := newFakeOnBuild() fakeFs := &test.FakeFileSystem{ diff --git a/pkg/build/strategies/sti/sti.go b/pkg/build/strategies/sti/sti.go index 24824b2e2..ce3a71a99 100644 --- a/pkg/build/strategies/sti/sti.go +++ b/pkg/build/strategies/sti/sti.go @@ -603,10 +603,8 @@ func (b *STI) Execute(command string, user string, config *api.Config) error { } func (b *STI) generateConfigEnv() (configEnv []string) { - if len(b.config.Environment) > 0 { - for key, val := range b.config.Environment { - configEnv = append(configEnv, key+"="+val) - } + for _, e := range b.config.Environment { + configEnv = append(configEnv, strings.Join([]string{e.Name, e.Value}, "=")) } return } diff --git a/pkg/build/strategies/sti/sti_test.go b/pkg/build/strategies/sti/sti_test.go index 3726f4e94..db4aa1a59 100644 --- a/pkg/build/strategies/sti/sti_test.go +++ b/pkg/build/strategies/sti/sti_test.go @@ -735,14 +735,16 @@ func equalArrayContents(a []string, b []string) bool { func TestGenerateConfigEnv(t *testing.T) { rh := newFakeSTI(&FakeSTI{}) - testEnv := map[string]string{ - "Key1": "Value1", - "Key2": "Value2", - "Key3": "Value3", + testEnv := api.EnvironmentList{ + {Name: "Key1", Value: "Value1"}, + {Name: "Key2", Value: "Value2"}, + {Name: "Key3", Value: "Value3"}, + {Name: "Key4", Value: "Value=4"}, + {Name: "Key5", Value: "Value,5"}, } rh.config.Environment = testEnv result := rh.generateConfigEnv() - expected := []string{"Key1=Value1", "Key2=Value2", "Key3=Value3"} + expected := []string{"Key1=Value1", "Key2=Value2", "Key3=Value3", "Key4=Value=4", "Key5=Value,5"} if !equalArrayContents(result, expected) { t.Errorf("Unexpected result. Expected: %#v. Actual: %#v", expected, result) diff --git a/pkg/cmd/util.go b/pkg/cmd/util.go index 6519e95b7..212f729ec 100644 --- a/pkg/cmd/util.go +++ b/pkg/cmd/util.go @@ -1,10 +1,8 @@ package cmd import ( - "fmt" "os" "path/filepath" - "strings" "github.com/openshift/source-to-image/pkg/api" "github.com/spf13/cobra" @@ -33,21 +31,3 @@ func AddCommonFlags(c *cobra.Command, cfg *api.Config) { c.Flags().StringVarP(&(cfg.Destination), "destination", "d", "", "Specify a destination location for untar operation") } - -// ParseEnvs parses the command line environemnt variable definitions -func ParseEnvs(c *cobra.Command, name string) (map[string]string, error) { - env := c.Flags().Lookup(name) - if env == nil || len(env.Value.String()) == 0 { - return nil, nil - } - envs := make(map[string]string) - pairs := strings.Split(env.Value.String(), ",") - for _, pair := range pairs { - atoms := strings.Split(pair, "=") - if len(atoms) != 2 { - return nil, fmt.Errorf("malformed syntax for environment variable: %s", pair) - } - envs[atoms[0]] = atoms[1] - } - return envs, nil -}