From d6effe3c6d2bbdd1d7738f84bd42880b64da32b2 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Fri, 24 Oct 2014 09:43:14 -0700 Subject: [PATCH] Rename api ErrorList for clarity --- pkg/api/errors/errors.go | 2 +- pkg/api/errors/errors_test.go | 2 +- pkg/api/errors/etcd/etcd.go | 2 +- pkg/api/errors/validation.go | 30 ++++---- pkg/api/errors/validation_test.go | 4 +- pkg/api/validation/validation.go | 102 +++++++++++++------------- pkg/api/validation/validation_test.go | 2 +- 7 files changed, 74 insertions(+), 70 deletions(-) diff --git a/pkg/api/errors/errors.go b/pkg/api/errors/errors.go index 148b1068793..ded92e11e61 100644 --- a/pkg/api/errors/errors.go +++ b/pkg/api/errors/errors.go @@ -92,7 +92,7 @@ func NewConflict(kind, name string, err error) error { } // NewInvalid returns an error indicating the item is invalid and cannot be processed. -func NewInvalid(kind, name string, errs ErrorList) error { +func NewInvalid(kind, name string, errs ValidationErrorList) error { causes := make([]api.StatusCause, 0, len(errs)) for i := range errs { if err, ok := errs[i].(ValidationError); ok { diff --git a/pkg/api/errors/errors_test.go b/pkg/api/errors/errors_test.go index 01a0a39aaae..54bfee1d681 100644 --- a/pkg/api/errors/errors_test.go +++ b/pkg/api/errors/errors_test.go @@ -116,7 +116,7 @@ func TestNewInvalid(t *testing.T) { for i, testCase := range testCases { vErr, expected := testCase.Err, testCase.Details expected.Causes[0].Message = vErr.Error() - err := NewInvalid("kind", "name", ErrorList{vErr}) + err := NewInvalid("kind", "name", ValidationErrorList{vErr}) status := err.(*statusError).Status() if status.Code != 422 || status.Reason != api.StatusReasonInvalid { t.Errorf("%d: unexpected status: %#v", i, status) diff --git a/pkg/api/errors/etcd/etcd.go b/pkg/api/errors/etcd/etcd.go index af4a3c8b796..ac819d4db0a 100644 --- a/pkg/api/errors/etcd/etcd.go +++ b/pkg/api/errors/etcd/etcd.go @@ -69,5 +69,5 @@ func InterpretDeleteError(err error, kind, name string) error { // for a failure to convert the resource version of an object sent // to the API to an etcd uint64 index. func InterpretResourceVersionError(err error, kind, value string) error { - return errors.NewInvalid(kind, "", errors.ErrorList{errors.NewFieldInvalid("resourceVersion", value)}) + return errors.NewInvalid(kind, "", errors.ValidationErrorList{errors.NewFieldInvalid("resourceVersion", value)}) } diff --git a/pkg/api/errors/validation.go b/pkg/api/errors/validation.go index a7658907e3f..2d01fbb9f22 100644 --- a/pkg/api/errors/validation.go +++ b/pkg/api/errors/validation.go @@ -113,20 +113,22 @@ func NewFieldNotFound(field string, value interface{}) ValidationError { return ValidationError{ValidationErrorTypeNotFound, field, value} } -// ErrorList is a collection of errors. This does not implement the error -// interface to avoid confusion where an empty ErrorList would still be an -// error (non-nil). To produce a single error instance from an ErrorList, use -// the ToError() method, which will return nil for an empty ErrorList. -type ErrorList util.ErrorList +// ValidationErrorList is a collection of ValidationErrors. This does not +// implement the error interface to avoid confusion where an empty +// ValidationErrorList would still be an error (non-nil). To produce a single +// error instance from a ValidationErrorList, use the ToError() method, which +// will return nil for an empty ValidationErrorList. +type ValidationErrorList util.ErrorList -// ToError converts an ErrorList into a "normal" error, or nil if the list is empty. -func (list ErrorList) ToError() error { +// ToError converts a ValidationErrorList into a "normal" error, or nil if the +// list is empty. +func (list ValidationErrorList) ToError() error { return util.ErrorList(list).ToError() } -// Prefix adds a prefix to the Field of every ValidationError in the list. Returns -// the list for convenience. -func (list ErrorList) Prefix(prefix string) ErrorList { +// Prefix adds a prefix to the Field of every ValidationError in the list. +// Returns the list for convenience. +func (list ValidationErrorList) Prefix(prefix string) ValidationErrorList { for i := range list { if err, ok := list[i].(ValidationError); ok { if strings.HasPrefix(err.Field, "[") { @@ -137,13 +139,15 @@ func (list ErrorList) Prefix(prefix string) ErrorList { err.Field = prefix } list[i] = err + } else { + glog.Warningf("ValidationErrorList holds non-ValidationError: %T", list) } } return list } -// PrefixIndex adds an index to the Field of every ValidationError in the list. Returns -// the list for convenience. -func (list ErrorList) PrefixIndex(index int) ErrorList { +// PrefixIndex adds an index to the Field of every ValidationError in the list. +// Returns the list for convenience. +func (list ValidationErrorList) PrefixIndex(index int) ValidationErrorList { return list.Prefix(fmt.Sprintf("[%d]", index)) } diff --git a/pkg/api/errors/validation_test.go b/pkg/api/errors/validation_test.go index a1c513a158c..0ed29359a30 100644 --- a/pkg/api/errors/validation_test.go +++ b/pkg/api/errors/validation_test.go @@ -82,7 +82,7 @@ func TestErrListPrefix(t *testing.T) { }, } for _, testCase := range testCases { - errList := ErrorList{testCase.Err} + errList := ValidationErrorList{testCase.Err} prefix := errList.Prefix("foo") if prefix == nil || len(prefix) != len(errList) { t.Errorf("Prefix should return self") @@ -112,7 +112,7 @@ func TestErrListPrefixIndex(t *testing.T) { }, } for _, testCase := range testCases { - errList := ErrorList{testCase.Err} + errList := ValidationErrorList{testCase.Err} prefix := errList.PrefixIndex(1) if prefix == nil || len(prefix) != len(errList) { t.Errorf("PrefixIndex should return self") diff --git a/pkg/api/validation/validation.go b/pkg/api/validation/validation.go index ec16c25a6ed..4605b7e4f79 100644 --- a/pkg/api/validation/validation.go +++ b/pkg/api/validation/validation.go @@ -27,13 +27,13 @@ import ( "github.com/GoogleCloudPlatform/kubernetes/pkg/util" ) -func validateVolumes(volumes []api.Volume) (util.StringSet, errs.ErrorList) { - allErrs := errs.ErrorList{} +func validateVolumes(volumes []api.Volume) (util.StringSet, errs.ValidationErrorList) { + allErrs := errs.ValidationErrorList{} allNames := util.StringSet{} for i := range volumes { vol := &volumes[i] // so we can set default values - el := errs.ErrorList{} + el := errs.ValidationErrorList{} if vol.Source == nil { // TODO: Enforce that a source is set once we deprecate the implied form. vol.Source = &api.VolumeSource{ @@ -57,9 +57,9 @@ func validateVolumes(volumes []api.Volume) (util.StringSet, errs.ErrorList) { return allNames, allErrs } -func validateSource(source *api.VolumeSource) errs.ErrorList { +func validateSource(source *api.VolumeSource) errs.ValidationErrorList { numVolumes := 0 - allErrs := errs.ErrorList{} + allErrs := errs.ValidationErrorList{} if source.HostDir != nil { numVolumes++ allErrs = append(allErrs, validateHostDir(source.HostDir).Prefix("hostDirectory")...) @@ -78,8 +78,8 @@ func validateSource(source *api.VolumeSource) errs.ErrorList { return allErrs } -func validateHostDir(hostDir *api.HostDir) errs.ErrorList { - allErrs := errs.ErrorList{} +func validateHostDir(hostDir *api.HostDir) errs.ValidationErrorList { + allErrs := errs.ValidationErrorList{} if hostDir.Path == "" { allErrs = append(allErrs, errs.NewNotFound("path", hostDir.Path)) } @@ -88,8 +88,8 @@ func validateHostDir(hostDir *api.HostDir) errs.ErrorList { var supportedPortProtocols = util.NewStringSet(string(api.ProtocolTCP), string(api.ProtocolUDP)) -func validateGCEPersistentDisk(PD *api.GCEPersistentDisk) errs.ErrorList { - allErrs := errs.ErrorList{} +func validateGCEPersistentDisk(PD *api.GCEPersistentDisk) errs.ValidationErrorList { + allErrs := errs.ValidationErrorList{} if PD.PDName == "" { allErrs = append(allErrs, errs.NewFieldInvalid("PD.PDName", PD.PDName)) } @@ -102,12 +102,12 @@ func validateGCEPersistentDisk(PD *api.GCEPersistentDisk) errs.ErrorList { return allErrs } -func validatePorts(ports []api.Port) errs.ErrorList { - allErrs := errs.ErrorList{} +func validatePorts(ports []api.Port) errs.ValidationErrorList { + allErrs := errs.ValidationErrorList{} allNames := util.StringSet{} for i := range ports { - pErrs := errs.ErrorList{} + pErrs := errs.ValidationErrorList{} port := &ports[i] // so we can set default values if len(port.Name) > 0 { if len(port.Name) > 63 || !util.IsDNSLabel(port.Name) { @@ -136,11 +136,11 @@ func validatePorts(ports []api.Port) errs.ErrorList { return allErrs } -func validateEnv(vars []api.EnvVar) errs.ErrorList { - allErrs := errs.ErrorList{} +func validateEnv(vars []api.EnvVar) errs.ValidationErrorList { + allErrs := errs.ValidationErrorList{} for i := range vars { - vErrs := errs.ErrorList{} + vErrs := errs.ValidationErrorList{} ev := &vars[i] // so we can set default values if len(ev.Name) == 0 { vErrs = append(vErrs, errs.NewFieldRequired("name", ev.Name)) @@ -153,11 +153,11 @@ func validateEnv(vars []api.EnvVar) errs.ErrorList { return allErrs } -func validateVolumeMounts(mounts []api.VolumeMount, volumes util.StringSet) errs.ErrorList { - allErrs := errs.ErrorList{} +func validateVolumeMounts(mounts []api.VolumeMount, volumes util.StringSet) errs.ValidationErrorList { + allErrs := errs.ValidationErrorList{} for i := range mounts { - mErrs := errs.ErrorList{} + mErrs := errs.ValidationErrorList{} mnt := &mounts[i] // so we can set default values if len(mnt.Name) == 0 { mErrs = append(mErrs, errs.NewFieldRequired("name", mnt.Name)) @@ -174,11 +174,11 @@ func validateVolumeMounts(mounts []api.VolumeMount, volumes util.StringSet) errs // AccumulateUniquePorts runs an extraction function on each Port of each Container, // accumulating the results and returning an error if any ports conflict. -func AccumulateUniquePorts(containers []api.Container, accumulator map[int]bool, extract func(*api.Port) int) errs.ErrorList { - allErrs := errs.ErrorList{} +func AccumulateUniquePorts(containers []api.Container, accumulator map[int]bool, extract func(*api.Port) int) errs.ValidationErrorList { + allErrs := errs.ValidationErrorList{} for ci := range containers { - cErrs := errs.ErrorList{} + cErrs := errs.ValidationErrorList{} ctr := &containers[ci] for pi := range ctr.Ports { port := extract(&ctr.Ports[pi]) @@ -198,29 +198,29 @@ func AccumulateUniquePorts(containers []api.Container, accumulator map[int]bool, // checkHostPortConflicts checks for colliding Port.HostPort values across // a slice of containers. -func checkHostPortConflicts(containers []api.Container) errs.ErrorList { +func checkHostPortConflicts(containers []api.Container) errs.ValidationErrorList { allPorts := map[int]bool{} return AccumulateUniquePorts(containers, allPorts, func(p *api.Port) int { return p.HostPort }) } -func validateExecAction(exec *api.ExecAction) errs.ErrorList { - allErrors := errs.ErrorList{} +func validateExecAction(exec *api.ExecAction) errs.ValidationErrorList { + allErrors := errs.ValidationErrorList{} if len(exec.Command) == 0 { allErrors = append(allErrors, errs.NewFieldRequired("command", exec.Command)) } return allErrors } -func validateHTTPGetAction(http *api.HTTPGetAction) errs.ErrorList { - allErrors := errs.ErrorList{} +func validateHTTPGetAction(http *api.HTTPGetAction) errs.ValidationErrorList { + allErrors := errs.ValidationErrorList{} if len(http.Path) == 0 { allErrors = append(allErrors, errs.NewFieldRequired("path", http.Path)) } return allErrors } -func validateHandler(handler *api.Handler) errs.ErrorList { - allErrors := errs.ErrorList{} +func validateHandler(handler *api.Handler) errs.ValidationErrorList { + allErrors := errs.ValidationErrorList{} if handler.Exec != nil { allErrors = append(allErrors, validateExecAction(handler.Exec).Prefix("exec")...) } else if handler.HTTPGet != nil { @@ -231,8 +231,8 @@ func validateHandler(handler *api.Handler) errs.ErrorList { return allErrors } -func validateLifecycle(lifecycle *api.Lifecycle) errs.ErrorList { - allErrs := errs.ErrorList{} +func validateLifecycle(lifecycle *api.Lifecycle) errs.ValidationErrorList { + allErrs := errs.ValidationErrorList{} if lifecycle.PostStart != nil { allErrs = append(allErrs, validateHandler(lifecycle.PostStart).Prefix("postStart")...) } @@ -242,12 +242,12 @@ func validateLifecycle(lifecycle *api.Lifecycle) errs.ErrorList { return allErrs } -func validateContainers(containers []api.Container, volumes util.StringSet) errs.ErrorList { - allErrs := errs.ErrorList{} +func validateContainers(containers []api.Container, volumes util.StringSet) errs.ValidationErrorList { + allErrs := errs.ValidationErrorList{} allNames := util.StringSet{} for i := range containers { - cErrs := errs.ErrorList{} + cErrs := errs.ValidationErrorList{} ctr := &containers[i] // so we can set default values capabilities := capabilities.Get() if len(ctr.Name) == 0 { @@ -288,8 +288,8 @@ var supportedManifestVersions = util.NewStringSet("v1beta1", "v1beta2") // This includes checking formatting and uniqueness. It also canonicalizes the // structure by setting default values and implementing any backwards-compatibility // tricks. -func ValidateManifest(manifest *api.ContainerManifest) errs.ErrorList { - allErrs := errs.ErrorList{} +func ValidateManifest(manifest *api.ContainerManifest) errs.ValidationErrorList { + allErrs := errs.ValidationErrorList{} if len(manifest.Version) == 0 { allErrs = append(allErrs, errs.NewFieldRequired("version", manifest.Version)) @@ -303,9 +303,9 @@ func ValidateManifest(manifest *api.ContainerManifest) errs.ErrorList { return allErrs } -func validateRestartPolicy(restartPolicy *api.RestartPolicy) errs.ErrorList { +func validateRestartPolicy(restartPolicy *api.RestartPolicy) errs.ValidationErrorList { numPolicies := 0 - allErrors := errs.ErrorList{} + allErrors := errs.ValidationErrorList{} if restartPolicy.Always != nil { numPolicies++ } @@ -324,14 +324,14 @@ func validateRestartPolicy(restartPolicy *api.RestartPolicy) errs.ErrorList { return allErrors } -func ValidatePodState(podState *api.PodState) errs.ErrorList { - allErrs := errs.ErrorList(ValidateManifest(&podState.Manifest)).Prefix("manifest") +func ValidatePodState(podState *api.PodState) errs.ValidationErrorList { + allErrs := errs.ValidationErrorList(ValidateManifest(&podState.Manifest)).Prefix("manifest") return allErrs } // ValidatePod tests if required fields in the pod are set. -func ValidatePod(pod *api.Pod) errs.ErrorList { - allErrs := errs.ErrorList{} +func ValidatePod(pod *api.Pod) errs.ValidationErrorList { + allErrs := errs.ValidationErrorList{} if len(pod.Name) == 0 { allErrs = append(allErrs, errs.NewFieldRequired("name", pod.Name)) } @@ -343,8 +343,8 @@ func ValidatePod(pod *api.Pod) errs.ErrorList { } // ValidatePodUpdate tests to see if the update is legal -func ValidatePodUpdate(newPod, oldPod *api.Pod) errs.ErrorList { - allErrs := errs.ErrorList{} +func ValidatePodUpdate(newPod, oldPod *api.Pod) errs.ValidationErrorList { + allErrs := errs.ValidationErrorList{} if newPod.Name != oldPod.Name { allErrs = append(allErrs, errs.NewFieldInvalid("name", newPod.Name)) @@ -371,8 +371,8 @@ func ValidatePodUpdate(newPod, oldPod *api.Pod) errs.ErrorList { } // ValidateService tests if required fields in the service are set. -func ValidateService(service *api.Service) errs.ErrorList { - allErrs := errs.ErrorList{} +func ValidateService(service *api.Service) errs.ValidationErrorList { + allErrs := errs.ValidationErrorList{} if len(service.Name) == 0 { allErrs = append(allErrs, errs.NewFieldRequired("name", service.Name)) } else if !util.IsDNS952Label(service.Name) { @@ -396,8 +396,8 @@ func ValidateService(service *api.Service) errs.ErrorList { } // ValidateReplicationController tests if required fields in the replication controller are set. -func ValidateReplicationController(controller *api.ReplicationController) errs.ErrorList { - allErrs := errs.ErrorList{} +func ValidateReplicationController(controller *api.ReplicationController) errs.ValidationErrorList { + allErrs := errs.ValidationErrorList{} if len(controller.Name) == 0 { allErrs = append(allErrs, errs.NewFieldRequired("name", controller.Name)) } @@ -409,8 +409,8 @@ func ValidateReplicationController(controller *api.ReplicationController) errs.E } // ValidateReplicationControllerState tests if required fields in the replication controller state are set. -func ValidateReplicationControllerState(state *api.ReplicationControllerState) errs.ErrorList { - allErrs := errs.ErrorList{} +func ValidateReplicationControllerState(state *api.ReplicationControllerState) errs.ValidationErrorList { + allErrs := errs.ValidationErrorList{} if labels.Set(state.ReplicaSelector).AsSelector().Empty() { allErrs = append(allErrs, errs.NewFieldRequired("replicaSelector", state.ReplicaSelector)) } @@ -426,8 +426,8 @@ func ValidateReplicationControllerState(state *api.ReplicationControllerState) e allErrs = append(allErrs, ValidateReadOnlyPersistentDisks(state.PodTemplate.DesiredState.Manifest.Volumes).Prefix("podTemplate.desiredState.manifest")...) return allErrs } -func ValidateReadOnlyPersistentDisks(volumes []api.Volume) errs.ErrorList { - allErrs := errs.ErrorList{} +func ValidateReadOnlyPersistentDisks(volumes []api.Volume) errs.ValidationErrorList { + allErrs := errs.ValidationErrorList{} for _, vol := range volumes { if vol.Source.GCEPersistentDisk != nil { if vol.Source.GCEPersistentDisk.ReadOnly == false { diff --git a/pkg/api/validation/validation_test.go b/pkg/api/validation/validation_test.go index 75ea895abdf..4289efd78e0 100644 --- a/pkg/api/validation/validation_test.go +++ b/pkg/api/validation/validation_test.go @@ -26,7 +26,7 @@ import ( "github.com/GoogleCloudPlatform/kubernetes/pkg/util" ) -func expectPrefix(t *testing.T, prefix string, errs errors.ErrorList) { +func expectPrefix(t *testing.T, prefix string, errs errors.ValidationErrorList) { for i := range errs { if f, p := errs[i].(errors.ValidationError).Field, prefix; !strings.HasPrefix(f, p) { t.Errorf("expected prefix '%s' for field '%s' (%v)", p, f, errs[i])