From e1035d9880736f03f9a771449ec4c0b63ecf3c88 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. --- pkg/shp/cmd/build/create.go | 7 --- pkg/shp/cmd/buildrun/create.go | 7 --- pkg/shp/flags/build.go | 2 +- pkg/shp/flags/buildrun.go | 2 +- pkg/shp/flags/core_envvar_value.go | 61 ++++++++++++++++++++ pkg/shp/flags/core_envvvar_value_test.go | 35 ++++++++++++ pkg/shp/flags/csv.go | 39 +++++++++++++ pkg/shp/flags/csv_test.go | 71 ++++++++++++++++++++++++ pkg/shp/flags/flags.go | 8 +-- pkg/shp/util/env.go | 17 ------ pkg/shp/util/env_test.go | 54 ------------------ 11 files changed, 211 insertions(+), 92 deletions(-) create mode 100644 pkg/shp/flags/core_envvar_value.go create mode 100644 pkg/shp/flags/core_envvvar_value_test.go create mode 100644 pkg/shp/flags/csv.go create mode 100644 pkg/shp/flags/csv_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 4f0d1302e..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,12 +61,6 @@ func (c *CreateCommand) Run(params *params.Params, io *genericclioptions.IOStrea Spec: *c.buildSpec, } - envs, err := c.cmd.Flags().GetStringArray("env") - if err != nil { - return err - } - b.Spec.Env = append(b.Spec.Env, util.StringSliceToEnvVarSlice(envs)...) - 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 ee0b38985..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,12 +62,6 @@ func (c *CreateCommand) Run(params *params.Params, ioStreams *genericclioptions. Spec: *c.buildRunSpec, } - envs, err := c.cmd.Flags().GetStringArray("env") - if err != nil { - return err - } - br.Spec.Env = append(br.Spec.Env, util.StringSliceToEnvVarSlice(envs)...) - 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..1d51e1c4a --- /dev/null +++ b/pkg/shp/flags/core_envvar_value.go @@ -0,0 +1,61 @@ +package flags + +import ( + "fmt" + + corev1 "k8s.io/api/core/v1" +) + +// CoreEnvVarSliceValue implements pflag.Value interface, in order to store corev1.EnvVar key-value +// pairs, used on Shipwright's BuildSpec. +type CoreEnvVarSliceValue struct { + envs *[]corev1.EnvVar // pointer to the slice of EnvVar +} + +// String prints out the string representation of the slice of EnvVar objects. +func (c *CoreEnvVarSliceValue) String() string { + s := []string{} + for _, e := range *c.envs { + s = append(s, fmt.Sprintf("%s=%s", e.Name, e.Value)) + } + return fmt.Sprintf("[%s]", s) +} + +// setEnvVar populates the local slice of EnvVar, making sure the entry is not repeated. +func (c *CoreEnvVarSliceValue) setEnvVar(value string) error { + k, v, err := splitKV(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 +} + +// Set receives a string with key-value pairs comma separated. +func (c *CoreEnvVarSliceValue) Set(value string) error { + values, err := readAsCSV(value) + if err != nil { + return err + } + for _, v := range values { + if err = c.setEnvVar(v); err != nil { + return err + } + } + return nil +} + +// Type analogous to the pflag "stringSlice". +func (c *CoreEnvVarSliceValue) Type() string { + return "stringSlice" +} + +// NewCoreEnvVarSliceValue instantiate a CoreEnvVarSliceValue sharing the EnvVar pointer. +func NewCoreEnvVarSliceValue(envs *[]corev1.EnvVar) *CoreEnvVarSliceValue { + return &CoreEnvVarSliceValue{envs: envs} +} diff --git a/pkg/shp/flags/core_envvvar_value_test.go b/pkg/shp/flags/core_envvvar_value_test.go new file mode 100644 index 000000000..d84c6d963 --- /dev/null +++ b/pkg/shp/flags/core_envvvar_value_test.go @@ -0,0 +1,35 @@ +package flags + +import ( + "testing" + + "github.com/onsi/gomega" + o "github.com/onsi/gomega" + buildv1alpha1 "github.com/shipwright-io/build/pkg/apis/build/v1alpha1" + corev1 "k8s.io/api/core/v1" +) + +func TestCoreEnvVarSliceValue(t *testing.T) { + g := gomega.NewGomegaWithT(t) + + spec := &buildv1alpha1.BuildSpec{Env: []corev1.EnvVar{}} + c := NewCoreEnvVarSliceValue(&spec.Env) + + // expect error when key-value is not split by equal sign + err := c.Set("a") + g.Expect(err).NotTo(o.BeNil()) + + // setting two key-value pairs, error is not expected + err = c.Set("a=b,c=d") + g.Expect(err).To(o.BeNil()) + g.Expect(len(spec.Env)).To(o.Equal(2)) + g.Expect(spec.Env[0].Name).To(o.Equal("a")) + g.Expect(spec.Env[0].Value).To(o.Equal("b")) + + // on trying to insert a repeated value, it should error + err = c.Set("a=b") + g.Expect(err).NotTo(o.BeNil()) + + s := c.String() + g.Expect(s).To(o.Equal("[[a=b c=d]]")) +} diff --git a/pkg/shp/flags/csv.go b/pkg/shp/flags/csv.go new file mode 100644 index 000000000..609954c6b --- /dev/null +++ b/pkg/shp/flags/csv.go @@ -0,0 +1,39 @@ +package flags + +import ( + "bytes" + "encoding/csv" + "fmt" + "strings" +) + +// splitKV splits a key-value pair, and returns error for malformed pairs. +func splitKV(value string) (string, string, error) { + s := strings.Split(value, "=") + 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 +} + +// readAsCSV splits the value by comma into a slice of string. +func readAsCSV(value string) ([]string, error) { + if value == "" { + return []string{}, nil + } + + r := strings.NewReader(value) + cr := csv.NewReader(r) + return cr.Read() +} + +// writeAsCSV joins with comma the values informed. +func writeAsCSV(values []string) (string, error) { + b := &bytes.Buffer{} + w := csv.NewWriter(b) + if err := w.Write(values); err != nil { + return "", err + } + w.Flush() + return strings.TrimSuffix(b.String(), "\n"), nil +} diff --git a/pkg/shp/flags/csv_test.go b/pkg/shp/flags/csv_test.go new file mode 100644 index 000000000..84dc78e01 --- /dev/null +++ b/pkg/shp/flags/csv_test.go @@ -0,0 +1,71 @@ +package flags + +import ( + "testing" + + "github.com/onsi/gomega" + o "github.com/onsi/gomega" +) + +func Test_splitKV(t *testing.T) { + tests := []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, + }} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, got1, err := splitKV(tt.value) + if (err != nil) != tt.wantErr { + t.Errorf("splitKV() error = %v, wantErr %v", err, tt.wantErr) + return + } + if got != tt.k { + t.Errorf("splitKV() got = %v, want %v", got, tt.k) + } + if got1 != tt.v { + t.Errorf("splitKV() got1 = %v, want %v", got1, tt.v) + } + }) + } +} + +func Test_readAsCSV(t *testing.T) { + g := gomega.NewGomegaWithT(t) + + emptySlice, err := readAsCSV("") + g.Expect(err).To(o.BeNil()) + g.Expect(emptySlice).To(o.Equal([]string{})) + + slice, err := readAsCSV("a,b,c") + g.Expect(err).To(o.BeNil()) + g.Expect(slice).To(o.Equal([]string{"a", "b", "c"})) +} + +func Test_writeAsCSV(t *testing.T) { + g := gomega.NewGomegaWithT(t) + + s, err := writeAsCSV([]string{"a", "b", "c"}) + g.Expect(err).To(o.BeNil()) + g.Expect(s).To(o.Equal("a,b,c")) +} diff --git a/pkg/shp/flags/flags.go b/pkg/shp/flags/flags.go index a0c2f95ea..b11d54c69 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, +func envFlags(flags *pflag.FlagSet, envs *[]corev1.EnvVar) { + flags.VarP( + NewCoreEnvVarSliceValue(envs), "env", "e", - []string{}, "specify a key-value pair for an environment variable to set for the build container", ) } diff --git a/pkg/shp/util/env.go b/pkg/shp/util/env.go deleted file mode 100644 index 8b6c332e5..000000000 --- a/pkg/shp/util/env.go +++ /dev/null @@ -1,17 +0,0 @@ -package util - -import ( - "strings" - - corev1 "k8s.io/api/core/v1" -) - -func StringSliceToEnvVarSlice(envs []string) []corev1.EnvVar { - envVars := []corev1.EnvVar{} - - for _, e := range envs { - d := strings.Split(e, "=") - envVars = append(envVars, corev1.EnvVar{Name: d[0], Value: d[1]}) - } - return envVars -} diff --git a/pkg/shp/util/env_test.go b/pkg/shp/util/env_test.go deleted file mode 100644 index 30f16be3e..000000000 --- a/pkg/shp/util/env_test.go +++ /dev/null @@ -1,54 +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) - } - }) - } -}