Merge pull request #69 from otaviof/hackday-envvar-flag

Refactoring corev1.EnvVar Flag using pflag.Value

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

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

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,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}
+}

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]"))
+}

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

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
+}

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)
+			}
+		})
+	}
+}