Refactoring corev1.EnvVar Flag

Implementing pflag.Value interface to represent corev1.EnvVar used in
both Build and BuildRun.

Co-authored-by: Adam Kaplan <[email protected]>

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()

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()

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
 }

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
 }

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 := 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
+}
+
+// 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}
+}

pkg/shp/flags/core_envvvar_value_test.go

@@ -0,0 +1,35 @@
+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 := 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]]"))
+}

pkg/shp/flags/csv.go

@@ -0,0 +1,40 @@
+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
+}
+
+// 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
+}

pkg/shp/flags/csv_test.go

@@ -0,0 +1,76 @@
+package flags
+
+import (
+	"testing"
+
+	o "github.com/onsi/gomega"
+)
+
+func TestSplitKeyValue(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,
+	}, {
+		"success on splitting 'key=value=value' (double equal sign)",
+		"k=v=v",
+		"k",
+		"v=v",
+		false,
+	}}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, got1, err := splitKeyValue(tt.value)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("splitKeyValue() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			if got != tt.k {
+				t.Errorf("splitKeyValue() got = %v, want %v", got, tt.k)
+			}
+			if got1 != tt.v {
+				t.Errorf("splitKeyValue() got1 = %v, want %v", got1, tt.v)
+			}
+		})
+	}
+}
+
+func TestReadAsCSV(t *testing.T) {
+	g := o.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 TestWriteAsCSV(t *testing.T) {
+	g := o.NewGomegaWithT(t)
+
+	s, err := writeAsCSV([]string{"a", "b", "c"})
+	g.Expect(err).To(o.BeNil())
+	g.Expect(s).To(o.Equal("a,b,c"))
+}

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",
 	)
 }