Cleanup addmetadata.

pkg/commands/addmetadata.go

@@ -22,21 +22,19 @@ import (
 
 	"github.com/kubernetes-sigs/kustomize/pkg/constants"
 	"github.com/kubernetes-sigs/kustomize/pkg/fs"
+	"github.com/kubernetes-sigs/kustomize/pkg/validators"
 	"github.com/spf13/cobra"
-	"k8s.io/apimachinery/pkg/api/validation"
-	v1validation "k8s.io/apimachinery/pkg/apis/meta/v1/validation"
-	"k8s.io/apimachinery/pkg/util/validation/field"
 )
 
-// KindOfAdd is the kind of metadata being added: label or annotation
-type KindOfAdd int
+// kindOfAdd is the kind of metadata being added: label or annotation
+type kindOfAdd int
 
 const (
-	annotation KindOfAdd = iota
+	annotation kindOfAdd = iota
 	label
 )
 
-func (k KindOfAdd) String() string {
+func (k kindOfAdd) String() string {
 	kinds := [...]string{
 		"annotation",
 		"label",
@@ -48,98 +46,96 @@ func (k KindOfAdd) String() string {
 }
 
 type addMetadataOptions struct {
-	metadata            map[string]string
-	validateAnnotations func(map[string]string) field.ErrorList
-	validateLabels      func(map[string]string) field.ErrorList
+	metadata     map[string]string
+	mapValidator validators.MapValidatorFunc
+	kind         kindOfAdd
 }
 
 // newCmdAddAnnotation adds one or more commonAnnotations to the kustomization file.
-func newCmdAddAnnotation(fsys fs.FileSystem) *cobra.Command {
+func newCmdAddAnnotation(fSys fs.FileSystem, v validators.MapValidatorFunc) *cobra.Command {
 	var o addMetadataOptions
-	o.validateAnnotations = func(x map[string]string) field.ErrorList {
-		return validation.ValidateAnnotations(x, field.NewPath("field"))
-	}
+	o.kind = annotation
+	o.mapValidator = v
 
 	cmd := &cobra.Command{
 		Use:   "annotation",
-		Short: "Adds one or more commonAnnotations to the kustomization.yaml in current directory",
+		Short: "Adds one or more commonAnnotations to " + constants.KustomizationFileName,
 		Example: `
 		add annotation {annotationKey1:annotationValue1},{annotationKey2:annotationValue2}`,
 		RunE: func(cmd *cobra.Command, args []string) error {
-			err := o.ValidateAndParse(args, annotation)
+			err := o.ValidateAndParse(args)
 			if err != nil {
 				return err
 			}
-			return o.RunAddAnnotation(fsys, annotation)
+			return o.RunAddAnnotation(fSys)
 		},
 	}
 	return cmd
 }
 
 // newCmdAddLabel adds one or more commonLabels to the kustomization file.
-func newCmdAddLabel(fsys fs.FileSystem) *cobra.Command {
+func newCmdAddLabel(fSys fs.FileSystem, v validators.MapValidatorFunc) *cobra.Command {
 	var o addMetadataOptions
-	o.validateLabels = func(x map[string]string) field.ErrorList {
-		return v1validation.ValidateLabels(x, field.NewPath("field"))
-	}
+	o.kind = label
+	o.mapValidator = v
 
 	cmd := &cobra.Command{
 		Use:   "label",
-		Short: "Adds one or more commonLabels to the kustomization.yaml in current directory",
+		Short: "Adds one or more commonLabels to " + constants.KustomizationFileName,
 		Example: `
 		add label {labelKey1:labelValue1},{labelKey2:labelValue2}`,
 		RunE: func(cmd *cobra.Command, args []string) error {
-			err := o.ValidateAndParse(args, label)
+			err := o.ValidateAndParse(args)
 			if err != nil {
 				return err
 			}
-			return o.RunAddLabel(fsys, label)
+			return o.RunAddLabel(fSys)
 		},
 	}
 	return cmd
 }
 
 // ValidateAndParse validates addLabel and addAnnotation commands and parses them into o.metadata
-func (o *addMetadataOptions) ValidateAndParse(args []string, k KindOfAdd) error {
-	o.metadata = make(map[string]string)
+func (o *addMetadataOptions) ValidateAndParse(args []string) error {
 	if len(args) < 1 {
-		return fmt.Errorf("must specify %s", k)
+		return fmt.Errorf("must specify %s", o.kind)
 	}
 	if len(args) > 1 {
-		return fmt.Errorf("%ss must be comma-separated, with no spaces. See help text for example", k)
+		return fmt.Errorf("%ss must be comma-separated, with no spaces", o.kind)
 	}
-	inputs := strings.Split(args[0], ",")
-	for _, input := range inputs {
-		metadata := make(map[string]string)
-		//parse annotation keys and values into metadata
-		kv := strings.Split(input, ":")
-		if len(kv[0]) < 1 {
-			return fmt.Errorf("invalid %s format: %s", k, input)
-		}
-		if len(kv) > 1 {
-			metadata[kv[0]] = kv[1]
-		} else {
-			metadata[kv[0]] = ""
-		}
-		switch k {
-		case label:
-			if errs := o.validateLabels(metadata); len(errs) != 0 {
-				return fmt.Errorf("invalid %s format: %s", k, input)
-			}
-		case annotation:
-			if errs := o.validateAnnotations(metadata); len(errs) != 0 {
-				return fmt.Errorf("invalid %s format: %s", k, input)
-			}
-		default:
-			return fmt.Errorf("unknown metadata kind %s", k)
-		}
-		o.metadata[kv[0]] = metadata[kv[0]]
+	m, err := o.convertToMap(args[0])
+	if err != nil {
+		return err
 	}
+	if err = o.mapValidator(m); err != nil {
+		return err
+	}
+	o.metadata = m
 	return nil
 }
 
+func (o *addMetadataOptions) convertToMap(arg string) (map[string]string, error) {
+	result := make(map[string]string)
+	inputs := strings.Split(arg, ",")
+	for _, input := range inputs {
+		kv := strings.Split(input, ":")
+		if len(kv[0]) < 1 {
+			return nil, makeError(o.kind, input, "empty key")
+		}
+		if len(kv) > 2 {
+			return nil, makeError(o.kind, input, "too many colons")
+		}
+		if len(kv) > 1 {
+			result[kv[0]] = kv[1]
+		} else {
+			result[kv[0]] = ""
+		}
+	}
+	return result, nil
+}
+
 // RunAddAnnotation runs addAnnotation command, doing the real work.
-func (o *addMetadataOptions) RunAddAnnotation(fsys fs.FileSystem, k KindOfAdd) error {
+func (o *addMetadataOptions) RunAddAnnotation(fsys fs.FileSystem) error {
 	mf, err := newKustomizationFile(constants.KustomizationFileName, fsys)
 	if err != nil {
 		return err
@@ -154,18 +150,16 @@ func (o *addMetadataOptions) RunAddAnnotation(fsys fs.FileSystem, k KindOfAdd) e
 	}
 
 	for key, value := range o.metadata {
-		if k == annotation {
-			if _, ok := m.CommonAnnotations[key]; ok {
-				return fmt.Errorf("%s %s already in kustomization file", k, key)
-			}
-			m.CommonAnnotations[key] = value
+		if _, ok := m.CommonAnnotations[key]; ok {
+			return fmt.Errorf("%s %s already in kustomization file", o.kind, key)
 		}
+		m.CommonAnnotations[key] = value
 	}
 	return mf.write(m)
 }
 
 // RunAddLabel runs addLabel command, doing the real work.
-func (o *addMetadataOptions) RunAddLabel(fsys fs.FileSystem, k KindOfAdd) error {
+func (o *addMetadataOptions) RunAddLabel(fsys fs.FileSystem) error {
 	mf, err := newKustomizationFile(constants.KustomizationFileName, fsys)
 	if err != nil {
 		return err
@@ -181,9 +175,13 @@ func (o *addMetadataOptions) RunAddLabel(fsys fs.FileSystem, k KindOfAdd) error
 
 	for key, value := range o.metadata {
 		if _, ok := m.CommonLabels[key]; ok {
-			return fmt.Errorf("%s %s already in kustomization file", k, key)
+			return fmt.Errorf("%s %s already in kustomization file", o.kind, key)
 		}
 		m.CommonLabels[key] = value
 	}
 	return mf.write(m)
 }
+
+func makeError(k kindOfAdd, input string, message string) error {
+	return fmt.Errorf("invalid %s: %s (%s)", k, input, message)
+}

pkg/commands/addmetadata_test.go

@@ -21,6 +21,7 @@ import (
 
 	"github.com/kubernetes-sigs/kustomize/pkg/constants"
 	"github.com/kubernetes-sigs/kustomize/pkg/fs"
+	"github.com/kubernetes-sigs/kustomize/pkg/validators"
 )
 
 func TestRunAddAnnotation(t *testing.T) {
@@ -29,18 +30,18 @@ func TestRunAddAnnotation(t *testing.T) {
 	var o addMetadataOptions
 	o.metadata = map[string]string{"owls": "cute", "otters": "adorable"}
 
-	err := o.RunAddAnnotation(fakeFS, annotation)
+	err := o.RunAddAnnotation(fakeFS)
 	if err != nil {
 		t.Errorf("unexpected error: could not write to kustomization file")
 	}
 	// adding the same test input should not work
-	err = o.RunAddAnnotation(fakeFS, annotation)
+	err = o.RunAddAnnotation(fakeFS)
 	if err == nil {
 		t.Errorf("expected already in kustomization file error")
 	}
 	// adding new annotations should work
 	o.metadata = map[string]string{"new": "annotation"}
-	err = o.RunAddAnnotation(fakeFS, annotation)
+	err = o.RunAddAnnotation(fakeFS)
 	if err != nil {
 		t.Errorf("unexpected error: could not write to kustomization file")
 	}
@@ -48,38 +49,72 @@ func TestRunAddAnnotation(t *testing.T) {
 
 func TestAddAnnotationNoArgs(t *testing.T) {
 	fakeFS := fs.MakeFakeFS()
-	cmd := newCmdAddAnnotation(fakeFS)
+	v := validators.MakeHappyMapValidator(t)
+	cmd := newCmdAddAnnotation(fakeFS, v.Validator)
 	err := cmd.Execute()
+	v.VerifyNoCall()
 	if err == nil {
-		t.Errorf("expected an error but error is %v", err)
+		t.Errorf("expected an error")
 	}
-	if err != nil && err.Error() != "must specify annotation" {
+	if err.Error() != "must specify annotation" {
 		t.Errorf("incorrect error: %v", err.Error())
 	}
 }
 
 func TestAddAnnotationInvalidFormat(t *testing.T) {
 	fakeFS := fs.MakeFakeFS()
-	cmd := newCmdAddAnnotation(fakeFS)
-	args := []string{"exclamation!:point"}
+	v := validators.MakeSadMapValidator(t)
+	cmd := newCmdAddAnnotation(fakeFS, v.Validator)
+	args := []string{"whatever:whatever"}
 	err := cmd.RunE(cmd, args)
+	v.VerifyCall()
 	if err == nil {
-		t.Errorf("expected an error but error is %v", err)
+		t.Errorf("expected an error")
 	}
-	if err != nil && err.Error() != "invalid annotation format: exclamation!:point" {
+	if err.Error() != validators.SAD {
 		t.Errorf("incorrect error: %v", err.Error())
 	}
 }
 
+func TestAddAnnotationManyArgs(t *testing.T) {
+	fakeFS := fs.MakeFakeFS()
+	fakeFS.WriteFile(constants.KustomizationFileName, []byte(kustomizationContent))
+	v := validators.MakeHappyMapValidator(t)
+	cmd := newCmdAddAnnotation(fakeFS, v.Validator)
+	args := []string{"k1:v1,k2:v2,k3:v3,k4:v5"}
+	err := cmd.RunE(cmd, args)
+	v.VerifyCall()
+	if err != nil {
+		t.Errorf("unexpected error: %v", err.Error())
+	}
+}
+
 func TestAddAnnotationNoKey(t *testing.T) {
 	fakeFS := fs.MakeFakeFS()
-	cmd := newCmdAddAnnotation(fakeFS)
+	v := validators.MakeHappyMapValidator(t)
+	cmd := newCmdAddAnnotation(fakeFS, v.Validator)
 	args := []string{":nokey"}
 	err := cmd.RunE(cmd, args)
+	v.VerifyNoCall()
 	if err == nil {
-		t.Errorf("expected an error but error is %v", err)
+		t.Errorf("expected an error")
 	}
-	if err != nil && err.Error() != "invalid annotation format: :nokey" {
+	if err.Error() != "invalid annotation: :nokey (empty key)" {
+		t.Errorf("incorrect error: %v", err.Error())
+	}
+}
+
+func TestAddAnnotationTooManyColons(t *testing.T) {
+	fakeFS := fs.MakeFakeFS()
+	v := validators.MakeHappyMapValidator(t)
+	cmd := newCmdAddAnnotation(fakeFS, v.Validator)
+	args := []string{"key:v1:v2"}
+	err := cmd.RunE(cmd, args)
+	v.VerifyNoCall()
+	if err == nil {
+		t.Errorf("expected an error")
+	}
+	if err.Error() != "invalid annotation: key:v1:v2 (too many colons)" {
 		t.Errorf("incorrect error: %v", err.Error())
 	}
 }
@@ -87,9 +122,11 @@ func TestAddAnnotationNoKey(t *testing.T) {
 func TestAddAnnotationNoValue(t *testing.T) {
 	fakeFS := fs.MakeFakeFS()
 	fakeFS.WriteFile(constants.KustomizationFileName, []byte(kustomizationContent))
-	cmd := newCmdAddAnnotation(fakeFS)
+	v := validators.MakeHappyMapValidator(t)
+	cmd := newCmdAddAnnotation(fakeFS, v.Validator)
 	args := []string{"no:,value"}
 	err := cmd.RunE(cmd, args)
+	v.VerifyCall()
 	if err != nil {
 		t.Errorf("unexpected error: %v", err.Error())
 	}
@@ -98,13 +135,15 @@ func TestAddAnnotationNoValue(t *testing.T) {
 func TestAddAnnotationMultipleArgs(t *testing.T) {
 	fakeFS := fs.MakeFakeFS()
 	fakeFS.WriteFile(constants.KustomizationFileName, []byte(kustomizationContent))
-	cmd := newCmdAddAnnotation(fakeFS)
+	v := validators.MakeHappyMapValidator(t)
+	cmd := newCmdAddAnnotation(fakeFS, v.Validator)
 	args := []string{"this:annotation", "has:spaces"}
 	err := cmd.RunE(cmd, args)
+	v.VerifyNoCall()
 	if err == nil {
-		t.Errorf("expected an error but error is %v", err)
+		t.Errorf("expected an error")
 	}
-	if err != nil && err.Error() != "annotations must be comma-separated, with no spaces. See help text for example" {
+	if err.Error() != "annotations must be comma-separated, with no spaces" {
 		t.Errorf("incorrect error: %v", err.Error())
 	}
 }
@@ -115,18 +154,18 @@ func TestRunAddLabel(t *testing.T) {
 	var o addMetadataOptions
 	o.metadata = map[string]string{"owls": "cute", "otters": "adorable"}
 
-	err := o.RunAddLabel(fakeFS, label)
+	err := o.RunAddLabel(fakeFS)
 	if err != nil {
 		t.Errorf("unexpected error: could not write to kustomization file")
 	}
 	// adding the same test input should not work
-	err = o.RunAddLabel(fakeFS, label)
+	err = o.RunAddLabel(fakeFS)
 	if err == nil {
 		t.Errorf("expected already in kustomization file error")
 	}
 	// adding new labels should work
 	o.metadata = map[string]string{"new": "label"}
-	err = o.RunAddLabel(fakeFS, label)
+	err = o.RunAddLabel(fakeFS)
 	if err != nil {
 		t.Errorf("unexpected error: could not write to kustomization file")
 	}
@@ -134,38 +173,59 @@ func TestRunAddLabel(t *testing.T) {
 
 func TestAddLabelNoArgs(t *testing.T) {
 	fakeFS := fs.MakeFakeFS()
-	cmd := newCmdAddLabel(fakeFS)
+	v := validators.MakeHappyMapValidator(t)
+	cmd := newCmdAddLabel(fakeFS, v.Validator)
 	err := cmd.Execute()
+	v.VerifyNoCall()
 	if err == nil {
-		t.Errorf("expected an error but error is: %v", err)
+		t.Errorf("expected an error")
 	}
-	if err != nil && err.Error() != "must specify label" {
+	if err.Error() != "must specify label" {
 		t.Errorf("incorrect error: %v", err.Error())
 	}
 }
 
 func TestAddLabelInvalidFormat(t *testing.T) {
 	fakeFS := fs.MakeFakeFS()
-	cmd := newCmdAddLabel(fakeFS)
+	v := validators.MakeSadMapValidator(t)
+	cmd := newCmdAddLabel(fakeFS, v.Validator)
 	args := []string{"exclamation!:point"}
 	err := cmd.RunE(cmd, args)
+	v.VerifyCall()
 	if err == nil {
-		t.Errorf("expected an error but error is: %v", err)
+		t.Errorf("expected an error")
 	}
-	if err != nil && err.Error() != "invalid label format: exclamation!:point" {
+	if err.Error() != validators.SAD {
 		t.Errorf("incorrect error: %v", err.Error())
 	}
 }
 
 func TestAddLabelNoKey(t *testing.T) {
 	fakeFS := fs.MakeFakeFS()
-	cmd := newCmdAddLabel(fakeFS)
+	v := validators.MakeHappyMapValidator(t)
+	cmd := newCmdAddLabel(fakeFS, v.Validator)
 	args := []string{":nokey"}
 	err := cmd.RunE(cmd, args)
+	v.VerifyNoCall()
 	if err == nil {
-		t.Errorf("expected an error but error is: %v", err)
+		t.Errorf("expected an error")
 	}
-	if err != nil && err.Error() != "invalid label format: :nokey" {
+	if err.Error() != "invalid label: :nokey (empty key)" {
+		t.Errorf("incorrect error: %v", err.Error())
+	}
+}
+
+func TestAddLabelTooManyColons(t *testing.T) {
+	fakeFS := fs.MakeFakeFS()
+	v := validators.MakeHappyMapValidator(t)
+	cmd := newCmdAddLabel(fakeFS, v.Validator)
+	args := []string{"key:v1:v2"}
+	err := cmd.RunE(cmd, args)
+	v.VerifyNoCall()
+	if err == nil {
+		t.Errorf("expected an error")
+	}
+	if err.Error() != "invalid label: key:v1:v2 (too many colons)" {
 		t.Errorf("incorrect error: %v", err.Error())
 	}
 }
@@ -173,9 +233,11 @@ func TestAddLabelNoKey(t *testing.T) {
 func TestAddLabelNoValue(t *testing.T) {
 	fakeFS := fs.MakeFakeFS()
 	fakeFS.WriteFile(constants.KustomizationFileName, []byte(kustomizationContent))
-	cmd := newCmdAddLabel(fakeFS)
+	v := validators.MakeHappyMapValidator(t)
+	cmd := newCmdAddLabel(fakeFS, v.Validator)
 	args := []string{"no,value:"}
 	err := cmd.RunE(cmd, args)
+	v.VerifyCall()
 	if err != nil {
 		t.Errorf("unexpected error: %v", err.Error())
 	}
@@ -184,13 +246,15 @@ func TestAddLabelNoValue(t *testing.T) {
 func TestAddLabelMultipleArgs(t *testing.T) {
 	fakeFS := fs.MakeFakeFS()
 	fakeFS.WriteFile(constants.KustomizationFileName, []byte(kustomizationContent))
-	cmd := newCmdAddLabel(fakeFS)
+	v := validators.MakeHappyMapValidator(t)
+	cmd := newCmdAddLabel(fakeFS, v.Validator)
 	args := []string{"this:input", "has:spaces"}
 	err := cmd.RunE(cmd, args)
+	v.VerifyNoCall()
 	if err == nil {
-		t.Errorf("expected an error but error is: %v", err)
+		t.Errorf("expected an error")
 	}
-	if err != nil && err.Error() != "labels must be comma-separated, with no spaces. See help text for example" {
+	if err.Error() != "labels must be comma-separated, with no spaces" {
 		t.Errorf("incorrect error: %v", err.Error())
 	}
 }

pkg/commands/commands.go

@@ -22,6 +22,7 @@ import (
 	"os"
 
 	"github.com/kubernetes-sigs/kustomize/pkg/fs"
+	"github.com/kubernetes-sigs/kustomize/pkg/validators"
 	"github.com/spf13/cobra"
 )
 
@@ -109,8 +110,8 @@ func newCmdAdd(fsys fs.FileSystem) *cobra.Command {
 		newCmdAddPatch(fsys),
 		newCmdAddConfigMap(fsys),
 		newCmdAddBase(fsys),
-		newCmdAddLabel(fsys),
-		newCmdAddAnnotation(fsys),
+		newCmdAddLabel(fsys, validators.MakeLabelValidator()),
+		newCmdAddAnnotation(fsys, validators.MakeAnnotationValidator()),
 	)
 	return c
 }

pkg/validators/validators.go

@@ -0,0 +1,79 @@
+package validators
+
+import (
+	"errors"
+	apivalidation "k8s.io/apimachinery/pkg/api/validation"
+	v1validation "k8s.io/apimachinery/pkg/apis/meta/v1/validation"
+	"k8s.io/apimachinery/pkg/util/validation/field"
+	"testing"
+)
+
+// MapValidatorFunc returns an error if a map contains errors.
+type MapValidatorFunc func(map[string]string) error
+
+// MakeAnnotationValidator returns a MapValidatorFunc using apimachinery.
+func MakeAnnotationValidator() MapValidatorFunc {
+	return func(x map[string]string) error {
+		errs := apivalidation.ValidateAnnotations(x, field.NewPath("field"))
+		if errs != nil {
+			return errors.New(errs.ToAggregate().Error())
+		}
+		return nil
+	}
+}
+
+// MakeLabelValidator returns a MapValidatorFunc using apimachinery.
+func MakeLabelValidator() MapValidatorFunc {
+	return func(x map[string]string) error {
+		errs := v1validation.ValidateLabels(x, field.NewPath("field"))
+		if errs != nil {
+			return errors.New(errs.ToAggregate().Error())
+		}
+		return nil
+	}
+}
+
+// FakeValidator can be used in tests.
+type FakeValidator struct {
+	happy  bool
+	called bool
+	t      *testing.T
+}
+
+// SAD is an error string.
+const SAD = "i'm not happy Bob, NOT HAPPY"
+
+// MakeHappyMapValidator makes a FakeValidator that always passes.
+func MakeHappyMapValidator(t *testing.T) *FakeValidator {
+	return &FakeValidator{happy: true, t: t}
+}
+
+// MakeSadMapValidator makes a FakeValidator that always fails.
+func MakeSadMapValidator(t *testing.T) *FakeValidator {
+	return &FakeValidator{happy: false, t: t}
+}
+
+// Validator replaces apimachinery validation in tests.
+// Can be set to fail or succeed to test error handling.
+// Can confirm if run or not run by surrounding code.
+func (v *FakeValidator) Validator(_ map[string]string) error {
+	v.called = true
+	if v.happy {
+		return nil
+	}
+	return errors.New(SAD)
+}
+
+// VerifyCall returns true if Validator was used.
+func (v *FakeValidator) VerifyCall() {
+	if !v.called {
+		v.t.Errorf("should have called Validator")
+	}
+}
+
+// VerifyNoCall returns true if Validator was not used.
+func (v *FakeValidator) VerifyNoCall() {
+	if v.called {
+		v.t.Errorf("should not have called Validator")
+	}
+}