fix (build_validation) Incorrect field paths returned (#495)

* fixed field-paths returned for setting the Template field and also including Steps

* switched to correct FieldError: ErrMultipleOneOf instead of disallowing Steps

* changed errors: multiple sources and missing both template/steps

* fixed field paths in build_validation.go, added comments around some extraneous code/potential bugs

* fixed field-path for bad-kind in template, tested with unit test

* added ViaField() to Template validation caller, chained a bunch of validation calls with .Also(), made the reciever variable for validateTimeOut bs in line with the rest of build_validation.go

* addressed most of review comments, targetpath->targetPath

* removed ViaField(source) from validateSources errors

* fixed comment formatting

* updated error sent back for target path validation

* corrected fieldpaths for duplicate step names and multiple sources w/ subpaths

* standardized volume validation error field path

* Redid build_validation_test.go: check errors recieved against expected values, fixed bugs in Build Timeout tests

* fixed targetPath validation tests to reflect the FieldError change

* Added ErrOutOfBoundsValue for build timeout validation

* including vendor updates for unit tests

* rebased with upstream/master, fixed merge conflicts

* ran update-codegen.sh

Gopkg.toml

@@ -47,8 +47,8 @@ required = [
 
 [[override]]
   name = "github.com/knative/pkg"
-  # HEAD as of 2018-11-06
-  revision = "088e3f7faf9d7ed0122937f66289cc050d0a78b0"
+  # HEAD as of 2018-12-09
+  revision = "585076ba67e86ccbe9c2c146d49502d7433da7c0"
 
 [prune]
   go-tests = true

pkg/apis/build/v1alpha1/build_template_validation.go

@@ -47,7 +47,7 @@ func ValidateVolumes(volumes []corev1.Volume) *apis.FieldError {
 	vols := map[string]struct{}{}
 	for _, v := range volumes {
 		if _, ok := vols[v.Name]; ok {
-			return apis.ErrMultipleOneOf("volumeName")
+			return apis.ErrMultipleOneOf("name")
 		}
 		vols[v.Name] = struct{}{}
 	}
@@ -66,7 +66,7 @@ func validateSteps(steps []corev1.Container) *apis.FieldError {
 			continue
 		}
 		if _, ok := names[s.Name]; ok {
-			return apis.ErrMultipleOneOf("stepName")
+			return apis.ErrMultipleOneOf("name")
 		}
 		names[s.Name] = struct{}{}
 	}

pkg/apis/build/v1alpha1/build_validation.go

@@ -17,7 +17,6 @@ limitations under the License.
 package v1alpha1
 
 import (
-	"fmt"
 	"time"
 
 	"github.com/knative/pkg/apis"
@@ -31,68 +30,55 @@ func (b *Build) Validate() *apis.FieldError {
 // Validate for build spec
 func (bs *BuildSpec) Validate() *apis.FieldError {
 	if bs.Template == nil && len(bs.Steps) == 0 {
-		return apis.ErrMissingField("b.spec.template").Also(apis.ErrMissingField("b.spec.steps"))
+		return apis.ErrMissingOneOf("template", "steps")
 	}
 	if bs.Template != nil && len(bs.Steps) > 0 {
-		return apis.ErrMissingField("b.spec.template").Also(apis.ErrMissingField("b.spec.steps"))
+		return apis.ErrMultipleOneOf("template", "steps")
 	}
 
-	if bs.Template != nil && bs.Template.Name == "" {
-		apis.ErrMissingField("build.spec.template.name")
-	}
-
-	if err := bs.validateSources(); err != nil {
-		return err
-	}
 	// If a build specifies a template, all the template's parameters without
 	// defaults must be satisfied by the build's parameters.
 	if bs.Template != nil {
-		return bs.Template.Validate()
-	}
-	if err := ValidateVolumes(bs.Volumes); err != nil {
-		return err
-	}
-	if err := bs.validateTimeout(); err != nil {
-		return err
+		return bs.Template.Validate().ViaField("template")
 	}
 
-	if err := validateSteps(bs.Steps); err != nil {
-		return err
-	}
-	return nil
+	// Below method potentially has a bug:
+	// It does not Validate if only a "Source" has been set, it only validates if multiple sources have been set
+	return bs.validateSources().
+		Also(ValidateVolumes(bs.Volumes).ViaField("volumes")).
+		Also(bs.validateTimeout()).
+		Also(validateSteps(bs.Steps).ViaField("steps"))
 }
 
-// Validate templateKind
+// Validate template
 func (b *TemplateInstantiationSpec) Validate() *apis.FieldError {
 	if b == nil {
 		return nil
 	}
 	if b.Name == "" {
-		return apis.ErrMissingField("build.spec.template.name")
+		return apis.ErrMissingField("name")
 	}
 	if b.Kind != "" {
 		switch b.Kind {
 		case ClusterBuildTemplateKind,
 			BuildTemplateKind:
 			return nil
 		default:
-			return apis.ErrInvalidValue(string(b.Kind), apis.CurrentField)
+			return apis.ErrInvalidValue(string(b.Kind), "kind")
 		}
 	}
 	return nil
 }
 
 // Validate build timeout
-func (bt *BuildSpec) validateTimeout() *apis.FieldError {
-	if bt.Timeout == nil {
+func (bs *BuildSpec) validateTimeout() *apis.FieldError {
+	if bs.Timeout == nil {
 		return nil
 	}
 	maxTimeout := time.Duration(24 * time.Hour)
 
-	if bt.Timeout.Duration > maxTimeout {
-		return apis.ErrInvalidValue(fmt.Sprintf("%s should be < 24h", bt.Timeout), "b.spec.timeout")
-	} else if bt.Timeout.Duration < 0 {
-		return apis.ErrInvalidValue(fmt.Sprintf("%s should be > 0", bt.Timeout), "b.spec.timeout")
+	if bs.Timeout.Duration > maxTimeout || bs.Timeout.Duration < 0 {
+		return apis.ErrOutOfBoundsValue(bs.Timeout.Duration.String(), "0", "24", "timeout")
 	}
 	return nil
 }
@@ -106,20 +92,19 @@ func (bs BuildSpec) validateSources() *apis.FieldError {
 		nodeMap: map[string]map[string]string{},
 	}
 
-	// both source and sources cannot be defined in build
+	// Both source and sources cannot be defined in build
 	if len(bs.Sources) > 0 && bs.Source != nil {
-		return apis.ErrMultipleOneOf("b.spec.source", "b.spec.sources")
+		return apis.ErrMultipleOneOf("source", "sources")
 	}
-
 	for _, source := range bs.Sources {
-		// check all source have unique names
+		// Check all source have unique names
 		if _, ok := names[source.Name]; ok {
-			return apis.ErrMultipleOneOf("b.spec.sources.names")
+			return apis.ErrMultipleOneOf("name").ViaField("sources")
 		}
-		// multiple sources cannot have subpath defined
+		// Multiple sources cannot have subpath defined
 		if source.SubPath != "" {
 			if subPathExists {
-				return apis.ErrInvalidValue("b.spec.sources.subpath", source.SubPath)
+				return apis.ErrMultipleOneOf("subpath").ViaField("sources")
 			}
 			subPathExists = true
 		}
@@ -130,14 +115,14 @@ func (bs BuildSpec) validateSources() *apis.FieldError {
 				continue
 			}
 			if emptyTargetPath {
-				return apis.ErrInvalidValue("empty target path", "b.spec.sources.targetPath")
+				return apis.ErrInvalidValue("Empty Target Path", "targetPath").ViaField("sources")
 			}
 			emptyTargetPath = true
 		} else {
 			if source.Custom != nil {
-				return apis.ErrInvalidValue(source.TargetPath, "b.spec.sources.targetPath")
+				return apis.ErrInvalidValue(source.TargetPath, "targetPath").ViaField("sources")
 			}
-			if err := insertNode(source.TargetPath, pathtree); err != nil {
+			if err := insertNode(source.TargetPath, pathtree).ViaField("sources"); err != nil {
 				return err
 			}
 		}