From 1a1d2d85361821ac3de6e070abbea72f2c2896e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ota=CC=81vio=20Fernandes?= Date: Fri, 29 Oct 2021 15:33:11 +0200 Subject: [PATCH] Refactoring corev1.EnvVar Flag Implementing pflag.Value interface to represent corev1.EnvVar used in both Build and BuildRun. Co-authored-by: Adam Kaplan --- pkg/shp/cmd/build/create.go | 11 ---- pkg/shp/cmd/buildrun/create.go | 11 ---- pkg/shp/flags/build.go | 2 +- pkg/shp/flags/buildrun.go | 2 +- pkg/shp/flags/core_envvar_value.go | 50 ++++++++++++++++ pkg/shp/flags/core_envvar_value_test.go | 50 ++++++++++++++++ pkg/shp/flags/flags.go | 10 ++-- pkg/shp/flags/key_value.go | 29 +++++++++ pkg/shp/flags/key_value_test.go | 54 +++++++++++++++++ pkg/shp/util/env.go | 22 ------- pkg/shp/util/env_test.go | 78 ------------------------- 11 files changed, 189 insertions(+), 130 deletions(-) create mode 100644 pkg/shp/flags/core_envvar_value.go create mode 100644 pkg/shp/flags/core_envvar_value_test.go create mode 100644 pkg/shp/flags/key_value.go create mode 100644 pkg/shp/flags/key_value_test.go delete mode 100644 pkg/shp/util/env.go delete mode 100644 pkg/shp/util/env_test.go diff --git a/pkg/shp/cmd/build/create.go b/pkg/shp/cmd/build/create.go index 09194693e..4dbeffea2 100644 --- a/pkg/shp/cmd/build/create.go +++ b/pkg/shp/cmd/build/create.go @@ -12,7 +12,6 @@ import ( "github.com/shipwright-io/cli/pkg/shp/cmd/runner" "github.com/shipwright-io/cli/pkg/shp/flags" "github.com/shipwright-io/cli/pkg/shp/params" - "github.com/shipwright-io/cli/pkg/shp/util" ) // CreateCommand contains data input from user @@ -62,16 +61,6 @@ func (c *CreateCommand) Run(params *params.Params, io *genericclioptions.IOStrea Spec: *c.buildSpec, } - envs, err := c.cmd.Flags().GetStringArray(flags.EnvFlag) - if err != nil { - return err - } - parsedEnvs, err := util.StringSliceToEnvVarSlice(envs) - if err != nil { - return err - } - b.Spec.Env = append(b.Spec.Env, parsedEnvs...) - flags.SanitizeBuildSpec(&b.Spec) clientset, err := params.ShipwrightClientSet() diff --git a/pkg/shp/cmd/buildrun/create.go b/pkg/shp/cmd/buildrun/create.go index ba76b6339..e513139e3 100644 --- a/pkg/shp/cmd/buildrun/create.go +++ b/pkg/shp/cmd/buildrun/create.go @@ -12,7 +12,6 @@ import ( "github.com/shipwright-io/cli/pkg/shp/cmd/runner" "github.com/shipwright-io/cli/pkg/shp/flags" "github.com/shipwright-io/cli/pkg/shp/params" - "github.com/shipwright-io/cli/pkg/shp/util" ) // CreateCommand reprents the build's create subcommand. @@ -63,16 +62,6 @@ func (c *CreateCommand) Run(params *params.Params, ioStreams *genericclioptions. Spec: *c.buildRunSpec, } - envs, err := c.cmd.Flags().GetStringArray(flags.EnvFlag) - if err != nil { - return err - } - parsedEnvs, err := util.StringSliceToEnvVarSlice(envs) - if err != nil { - return err - } - br.Spec.Env = append(br.Spec.Env, parsedEnvs...) - flags.SanitizeBuildRunSpec(&br.Spec) clientset, err := params.ShipwrightClientSet() diff --git a/pkg/shp/flags/build.go b/pkg/shp/flags/build.go index 3c6b59ddd..be2b5381e 100644 --- a/pkg/shp/flags/build.go +++ b/pkg/shp/flags/build.go @@ -38,7 +38,7 @@ func BuildSpecFromFlags(flags *pflag.FlagSet) *buildv1alpha1.BuildSpec { imageFlags(flags, "builder", spec.Builder) imageFlags(flags, "output", &spec.Output) timeoutFlags(flags, spec.Timeout) - envFlags(flags, spec.Env) + envFlags(flags, &spec.Env) return spec } diff --git a/pkg/shp/flags/buildrun.go b/pkg/shp/flags/buildrun.go index 91ececc8e..82826035a 100644 --- a/pkg/shp/flags/buildrun.go +++ b/pkg/shp/flags/buildrun.go @@ -27,7 +27,7 @@ func BuildRunSpecFromFlags(flags *pflag.FlagSet) *buildv1alpha1.BuildRunSpec { serviceAccountFlags(flags, spec.ServiceAccount) timeoutFlags(flags, spec.Timeout) imageFlags(flags, "output", spec.Output) - envFlags(flags, spec.Env) + envFlags(flags, &spec.Env) return spec } diff --git a/pkg/shp/flags/core_envvar_value.go b/pkg/shp/flags/core_envvar_value.go new file mode 100644 index 000000000..673acadd9 --- /dev/null +++ b/pkg/shp/flags/core_envvar_value.go @@ -0,0 +1,50 @@ +package flags + +import ( + "fmt" + + corev1 "k8s.io/api/core/v1" +) + +// CoreEnvVarArrayValue implements pflag.Value interface, in order to store corev1.EnvVar key-value +// pairs used on Shipwright's BuildSpec. +type CoreEnvVarArrayValue struct { + envs *[]corev1.EnvVar // pointer to the slice of EnvVar +} + +// String prints out the string representation of the slice of EnvVar objects. +func (c *CoreEnvVarArrayValue) String() string { + slice := []string{} + for _, e := range *c.envs { + slice = append(slice, fmt.Sprintf("%s=%s", e.Name, e.Value)) + } + csv, _ := writeAsCSV(slice) + return fmt.Sprintf("[%s]", csv) +} + +// Set receives a key-value entry separated by equal sign ("="). +func (c *CoreEnvVarArrayValue) Set(value string) error { + k, v, err := splitKeyValue(value) + if err != nil { + return err + } + for _, e := range *c.envs { + if k == e.Name { + return fmt.Errorf("environment variable '%s' is already set!", k) + } + } + *c.envs = append(*c.envs, corev1.EnvVar{Name: k, Value: v}) + return nil +} + +// Type analogous to the pflag "stringArray" type, where each flag entry will be tranlated to a +// single array (slice) entry, therefore the comma (",") is accepted as part of the value, as any +// other special character. +func (c *CoreEnvVarArrayValue) Type() string { + return "stringArray" +} + +// NewCoreEnvVarArrayValue instantiate a CoreEnvVarSliceValue sharing the EnvVar pointer. +func NewCoreEnvVarArrayValue(envs *[]corev1.EnvVar) *CoreEnvVarArrayValue { + return &CoreEnvVarArrayValue{envs: envs} +} diff --git a/pkg/shp/flags/core_envvar_value_test.go b/pkg/shp/flags/core_envvar_value_test.go new file mode 100644 index 000000000..37eff033f --- /dev/null +++ b/pkg/shp/flags/core_envvar_value_test.go @@ -0,0 +1,50 @@ +package flags + +import ( + "testing" + + buildv1alpha1 "github.com/shipwright-io/build/pkg/apis/build/v1alpha1" + corev1 "k8s.io/api/core/v1" + + o "github.com/onsi/gomega" +) + +func TestCoreEnvVarSliceValue(t *testing.T) { + g := o.NewGomegaWithT(t) + + spec := &buildv1alpha1.BuildSpec{Env: []corev1.EnvVar{}} + c := NewCoreEnvVarArrayValue(&spec.Env) + + // expect error when key-value is not split by equal sign + err := c.Set("a") + g.Expect(err).NotTo(o.BeNil()) + + // setting a simple key-value entry + err = c.Set("a=b") + g.Expect(err).To(o.BeNil()) + g.Expect(len(spec.Env)).To(o.Equal(1)) + g.Expect(spec.Env[0].Name).To(o.Equal("a")) + g.Expect(spec.Env[0].Value).To(o.Equal("b")) + + // setting a key-value entry with special characters + err = c.Set("b=c,d,e=f") + g.Expect(err).To(o.BeNil()) + g.Expect(len(spec.Env)).To(o.Equal(2)) + g.Expect(spec.Env[1].Name).To(o.Equal("b")) + g.Expect(spec.Env[1].Value).To(o.Equal("c,d,e=f")) + + // setting a key-value entry with space on it + err = c.Set("c=d e") + g.Expect(err).To(o.BeNil()) + g.Expect(len(spec.Env)).To(o.Equal(3)) + g.Expect(spec.Env[2].Name).To(o.Equal("c")) + g.Expect(spec.Env[2].Value).To(o.Equal("d e")) + + // on trying to insert a repeated value, it should error + err = c.Set("a=b") + g.Expect(err).NotTo(o.BeNil()) + + // making sure the string representation produced is as expected + s := c.String() + g.Expect(s).To(o.Equal("[a=b,\"b=c,d,e=f\",c=d e]")) +} diff --git a/pkg/shp/flags/flags.go b/pkg/shp/flags/flags.go index 2553f1130..1ba4c3a65 100644 --- a/pkg/shp/flags/flags.go +++ b/pkg/shp/flags/flags.go @@ -166,13 +166,11 @@ func serviceAccountFlags(flags *pflag.FlagSet, sa *buildv1alpha1.ServiceAccount) } // envFlags registers flags for adding corev1.EnvVars. -func envFlags(flags *pflag.FlagSet, envs []corev1.EnvVar) { - var e []string - flags.StringArrayVarP( - &e, - EnvFlag, +func envFlags(flags *pflag.FlagSet, envs *[]corev1.EnvVar) { + flags.VarP( + NewCoreEnvVarArrayValue(envs), + "env", "e", - []string{}, "specify a key-value pair for an environment variable to set for the build container", ) } diff --git a/pkg/shp/flags/key_value.go b/pkg/shp/flags/key_value.go new file mode 100644 index 000000000..82f6e8190 --- /dev/null +++ b/pkg/shp/flags/key_value.go @@ -0,0 +1,29 @@ +package flags + +import ( + "bytes" + "encoding/csv" + "fmt" + "strings" +) + +// splitKeyValue splits a key-value string with the format "key=value". Returns the key, value, and +// an error if the string is not formatted correctly. +func splitKeyValue(value string) (string, string, error) { + s := strings.SplitN(value, "=", 2) + if len(s) != 2 || s[0] == "" { + return "", "", fmt.Errorf("informed value '%s' is not in key=value format!", value) + } + return s[0], s[1], nil +} + +// writeAsCSV returns the informed slice of strings as a single string split by comma (","). +func writeAsCSV(slice []string) (string, error) { + b := &bytes.Buffer{} + w := csv.NewWriter(b) + if err := w.Write(slice); err != nil { + return "", err + } + w.Flush() + return strings.TrimSuffix(b.String(), "\n"), nil +} diff --git a/pkg/shp/flags/key_value_test.go b/pkg/shp/flags/key_value_test.go new file mode 100644 index 000000000..5bd99831e --- /dev/null +++ b/pkg/shp/flags/key_value_test.go @@ -0,0 +1,54 @@ +package flags + +import ( + "testing" +) + +func TestSplitKeyValue(t *testing.T) { + testCases := []struct { + name string + value string + k string + v string + wantErr bool + }{{ + "error when no value is informed", + "k", + "", + "", + true, + }, { + "error when only value is informed", + "=v", + "", + "", + true, + }, { + "success on spliting 'key=value'", + "k=v", + "k", + "v", + false, + }, { + "success on splitting 'key=value=value' (double equal sign)", + "k=v=v", + "k", + "v=v", + false, + }} + for _, tt := range testCases { + t.Run(tt.name, func(t *testing.T) { + k, v, err := splitKeyValue(tt.value) + if (err != nil) != tt.wantErr { + t.Errorf("splitKeyValue() error='%v', wantErr '%v'", err, tt.wantErr) + return + } + if k != tt.k { + t.Errorf("splitKeyValue() key='%v', want '%v'", k, tt.k) + } + if v != tt.v { + t.Errorf("splitKeyValue() value='%v', want '%v'", v, tt.v) + } + }) + } +} diff --git a/pkg/shp/util/env.go b/pkg/shp/util/env.go deleted file mode 100644 index f91c36cc8..000000000 --- a/pkg/shp/util/env.go +++ /dev/null @@ -1,22 +0,0 @@ -package util - -import ( - "fmt" - "strings" - - corev1 "k8s.io/api/core/v1" -) - -func StringSliceToEnvVarSlice(envs []string) ([]corev1.EnvVar, error) { - envVars := []corev1.EnvVar{} - - for _, l := range envs { - parts := strings.SplitN(l, "=", 2) - if len(parts) == 1 { - return envVars, fmt.Errorf("failed to parse key-value pair %q, not enough parts", l) - } - envVars = append(envVars, corev1.EnvVar{Name: parts[0], Value: parts[1]}) - } - - return envVars, nil -} diff --git a/pkg/shp/util/env_test.go b/pkg/shp/util/env_test.go deleted file mode 100644 index a486b71eb..000000000 --- a/pkg/shp/util/env_test.go +++ /dev/null @@ -1,78 +0,0 @@ -package util - -import ( - "reflect" - "testing" - - corev1 "k8s.io/api/core/v1" -) - -func TestStringSliceToEnvVar(t *testing.T) { - type args struct { - envs []string - } - tests := []struct { - name string - args args - want []corev1.EnvVar - }{ - { - name: "no envs", - args: args{ - envs: []string{}, - }, - want: []corev1.EnvVar{}, - }, - { - name: "one env", - args: args{ - envs: []string{"my-name=my-value"}, - }, - want: []corev1.EnvVar{ - {Name: "my-name", Value: "my-value"}, - }, - }, - { - name: "multiple envs", - args: args{ - envs: []string{"name-one=value-one", "name-two=value-two", "name-three=value-three"}, - }, - want: []corev1.EnvVar{ - {Name: "name-one", Value: "value-one"}, - {Name: "name-two", Value: "value-two"}, - {Name: "name-three", Value: "value-three"}, - }, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got, _ := StringSliceToEnvVarSlice(tt.args.envs); !reflect.DeepEqual(got, tt.want) { - t.Errorf("StringSliceToEnvVar() = %#v, want %#v", got, tt.want) - } - }) - } -} - -func TestStringSliceToEnvVar_ErrorCases(t *testing.T) { - type args struct { - envs []string - } - tests := []struct { - name string - args args - }{ - { - name: "value part missing", - args: args{ - envs: []string{"abc"}, - }, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if _, err := StringSliceToEnvVarSlice(tt.args.envs); err == nil { - t.Errorf("StringSliceToEnvVarSlice() = want error but got nil") - } - }) - } -}