From 18d32751fd4c71797339799cfc3007eaf73bb5bb Mon Sep 17 00:00:00 2001 From: Chao Xu Date: Fri, 31 Jul 2015 16:43:39 -0700 Subject: [PATCH] improve the error message of update pod --- pkg/api/validation/validation.go | 6 ++- pkg/api/validation/validation_test.go | 8 ++-- pkg/kubectl/cmd/patch.go | 2 +- pkg/kubectl/cmd/replace.go | 2 +- pkg/kubectl/cmd/util/helpers.go | 54 +-------------------------- pkg/kubectl/cmd/util/helpers_test.go | 4 +- pkg/util/fielderrors/fielderrors.go | 2 +- test/e2e/service.go | 4 +- 8 files changed, 16 insertions(+), 66 deletions(-) diff --git a/pkg/api/validation/validation.go b/pkg/api/validation/validation.go index b9bc64f6577..002b6fa726e 100644 --- a/pkg/api/validation/validation.go +++ b/pkg/api/validation/validation.go @@ -983,7 +983,8 @@ func ValidatePodUpdate(newPod, oldPod *api.Pod) errs.ValidationErrorList { allErrs = append(allErrs, ValidateObjectMetaUpdate(&newPod.ObjectMeta, &oldPod.ObjectMeta).Prefix("metadata")...) if len(newPod.Spec.Containers) != len(oldPod.Spec.Containers) { - allErrs = append(allErrs, errs.NewFieldInvalid("spec.containers", newPod.Spec.Containers, "may not add or remove containers")) + //TODO: Pinpoint the specific container that causes the invalid error after we have strategic merge diff + allErrs = append(allErrs, errs.NewFieldInvalid("spec.containers", "content of spec.containers is not printed out, please refer to the \"details\"", "may not add or remove containers")) return allErrs } pod := *newPod @@ -995,7 +996,8 @@ func ValidatePodUpdate(newPod, oldPod *api.Pod) errs.ValidationErrorList { } pod.Spec.Containers = newContainers if !api.Semantic.DeepEqual(pod.Spec, oldPod.Spec) { - allErrs = append(allErrs, errs.NewFieldInvalid("spec", newPod.Spec, "may not update fields other than container.image")) + //TODO: Pinpoint the specific field that causes the invalid error after we have strategic merge diff + allErrs = append(allErrs, errs.NewFieldInvalid("spec", "content of spec is not printed out, please refer to the \"details\"", "may not update fields other than container.image")) } newPod.Status = oldPod.Status diff --git a/pkg/api/validation/validation_test.go b/pkg/api/validation/validation_test.go index 1f1b7a5dd89..1f75deb0b7e 100644 --- a/pkg/api/validation/validation_test.go +++ b/pkg/api/validation/validation_test.go @@ -603,7 +603,7 @@ func TestValidateEnv(t *testing.T) { { name: "name not a C identifier", envs: []api.EnvVar{{Name: "a.b.c"}}, - expectedError: `[0].name: invalid value 'a.b.c': must be a C identifier (matching regex [A-Za-z_][A-Za-z0-9_]*): e.g. "my_name" or "MyName"`, + expectedError: `[0].name: invalid value 'a.b.c', Details: must be a C identifier (matching regex [A-Za-z_][A-Za-z0-9_]*): e.g. "my_name" or "MyName"`, }, { name: "value and valueFrom specified", @@ -617,7 +617,7 @@ func TestValidateEnv(t *testing.T) { }, }, }}, - expectedError: "[0].valueFrom: invalid value '': sources cannot be specified when value is not empty", + expectedError: "[0].valueFrom: invalid value '', Details: sources cannot be specified when value is not empty", }, { name: "missing FieldPath on ObjectFieldSelector", @@ -654,7 +654,7 @@ func TestValidateEnv(t *testing.T) { }, }, }}, - expectedError: "[0].valueFrom.fieldRef.fieldPath: invalid value 'metadata.whoops': error converting fieldPath", + expectedError: "[0].valueFrom.fieldRef.fieldPath: invalid value 'metadata.whoops', Details: error converting fieldPath", }, { name: "unsupported fieldPath", @@ -667,7 +667,7 @@ func TestValidateEnv(t *testing.T) { }, }, }}, - expectedError: "[0].valueFrom.fieldRef.fieldPath: unsupported value 'status.phase': supported values: metadata.name, metadata.namespace", + expectedError: "[0].valueFrom.fieldRef.fieldPath: unsupported value 'status.phase', Details: supported values: metadata.name, metadata.namespace", }, } for _, tc := range errorCases { diff --git a/pkg/kubectl/cmd/patch.go b/pkg/kubectl/cmd/patch.go index dbc38781dff..490b3481118 100644 --- a/pkg/kubectl/cmd/patch.go +++ b/pkg/kubectl/cmd/patch.go @@ -45,7 +45,7 @@ func NewCmdPatch(f *cmdutil.Factory, out io.Writer) *cobra.Command { cmdutil.CheckErr(cmdutil.ValidateOutputArgs(cmd)) shortOutput := cmdutil.GetFlagString(cmd, "output") == "name" err := RunPatch(f, out, cmd, args, shortOutput) - cmdutil.CheckCustomErr("Patch failed", err) + cmdutil.CheckErr(err) }, } cmd.Flags().StringP("patch", "p", "", "The patch to be applied to the resource JSON file.") diff --git a/pkg/kubectl/cmd/replace.go b/pkg/kubectl/cmd/replace.go index 72fcb23486f..d82637663f2 100644 --- a/pkg/kubectl/cmd/replace.go +++ b/pkg/kubectl/cmd/replace.go @@ -59,7 +59,7 @@ func NewCmdReplace(f *cmdutil.Factory, out io.Writer) *cobra.Command { cmdutil.CheckErr(cmdutil.ValidateOutputArgs(cmd)) shortOutput := cmdutil.GetFlagString(cmd, "output") == "name" err := RunReplace(f, out, cmd, args, filenames, shortOutput) - cmdutil.CheckCustomErr("Replace failed", err) + cmdutil.CheckErr(err) }, } usage := "Filename, directory, or URL to file to use to replace the resource." diff --git a/pkg/kubectl/cmd/util/helpers.go b/pkg/kubectl/cmd/util/helpers.go index a0e21af7230..3356592975a 100644 --- a/pkg/kubectl/cmd/util/helpers.go +++ b/pkg/kubectl/cmd/util/helpers.go @@ -79,59 +79,7 @@ func checkErr(err error, handleErr func(string)) { if errors.IsInvalid(err) { details := err.(*errors.StatusError).Status().Details - prefix := fmt.Sprintf("The %s %q is invalid:", details.Kind, details.Name) - errs := statusCausesToAggrError(details.Causes) - handleErr(MultilineError(prefix, errs)) - } - - // handle multiline errors - if clientcmd.IsConfigurationInvalid(err) { - handleErr(MultilineError("Error in configuration: ", err)) - } - if agg, ok := err.(utilerrors.Aggregate); ok && len(agg.Errors()) > 0 { - handleErr(MultipleErrors("", agg.Errors())) - } - - msg, ok := StandardErrorMessage(err) - if !ok { - msg = fmt.Sprintf("error: %s\n", err.Error()) - } - handleErr(msg) -} - -// CheckCustomErr is like CheckErr except a custom prefix error -// string may be provied to help produce more specific error messages. -// For example, for the update failed case this function could be called -// with: -// cmdutil.CheckCustomErr("Update failed", err) -// This function supresses the detailed output that is produced by CheckErr -// and specifically the field is erased and the error message has the details -// of the spec removed. Unfortunately, what starts off as a detail message is -// a sperate field ends up being concatentated into one string which contains -// the spec and the detail string. To avoid significant refactoring of the error -// data structures we just extract the required detail string by looking for it -// after "}': " which is horrible but expedient. -func CheckCustomErr(customPrefix string, err error) { - checkCustomErr(customPrefix, err, fatal) -} - -func checkCustomErr(customPrefix string, err error, handleErr func(string)) { - if err == nil { - return - } - - if errors.IsInvalid(err) { - details := err.(*errors.StatusError).Status().Details - for i := range details.Causes { - c := &details.Causes[i] - s := strings.Split(c.Message, "}': ") - if len(s) == 2 { - c.Message = - s[1] - c.Field = "" - } - } - prefix := fmt.Sprintf("%s", customPrefix) + prefix := fmt.Sprintf("The %s %q is invalid.\n", details.Kind, details.Name) errs := statusCausesToAggrError(details.Causes) handleErr(MultilineError(prefix, errs)) } diff --git a/pkg/kubectl/cmd/util/helpers_test.go b/pkg/kubectl/cmd/util/helpers_test.go index 57d0999e3c6..d57114d616e 100644 --- a/pkg/kubectl/cmd/util/helpers_test.go +++ b/pkg/kubectl/cmd/util/helpers_test.go @@ -274,11 +274,11 @@ func TestCheckInvalidErr(t *testing.T) { }{ { errors.NewInvalid("Invalid1", "invalidation", fielderrors.ValidationErrorList{fielderrors.NewFieldInvalid("Cause", "single", "details")}), - `Error from server: Invalid1 "invalidation" is invalid: Cause: invalid value 'single': details`, + `Error from server: Invalid1 "invalidation" is invalid: Cause: invalid value 'single', Details: details`, }, { errors.NewInvalid("Invalid2", "invalidation", fielderrors.ValidationErrorList{fielderrors.NewFieldInvalid("Cause", "multi1", "details"), fielderrors.NewFieldInvalid("Cause", "multi2", "details")}), - `Error from server: Invalid2 "invalidation" is invalid: [Cause: invalid value 'multi1': details, Cause: invalid value 'multi2': details]`, + `Error from server: Invalid2 "invalidation" is invalid: [Cause: invalid value 'multi1', Details: details, Cause: invalid value 'multi2', Details: details]`, }, { errors.NewInvalid("Invalid3", "invalidation", fielderrors.ValidationErrorList{}), diff --git a/pkg/util/fielderrors/fielderrors.go b/pkg/util/fielderrors/fielderrors.go index 0c548482fb7..d4a8d0dc660 100644 --- a/pkg/util/fielderrors/fielderrors.go +++ b/pkg/util/fielderrors/fielderrors.go @@ -102,7 +102,7 @@ func (v *ValidationError) ErrorBody() string { s = spew.Sprintf("%s '%+v'", v.Type, v.BadValue) } if len(v.Detail) != 0 { - s += fmt.Sprintf(": %s", v.Detail) + s += fmt.Sprintf(", Details: %s", v.Detail) } return s } diff --git a/test/e2e/service.go b/test/e2e/service.go index 4b470b497e2..3ad64094300 100644 --- a/test/e2e/service.go +++ b/test/e2e/service.go @@ -770,7 +770,7 @@ var _ = Describe("Services", func() { if err == nil { Failf("Created service with conflicting NodePort: %v", result2) } - expectedErr := fmt.Sprintf("Service \"%s\" is invalid: spec.ports[0].nodePort: invalid value '%d': provided port is already allocated", serviceName2, port.NodePort) + expectedErr := fmt.Sprintf("Service \"%s\" is invalid: spec.ports[0].nodePort: invalid value '%d', Details: provided port is already allocated", serviceName2, port.NodePort) Expect(fmt.Sprintf("%v", err)).To(Equal(expectedErr)) By("deleting original service " + serviceName + " with type NodePort in namespace " + ns) @@ -830,7 +830,7 @@ var _ = Describe("Services", func() { if err == nil { Failf("failed to prevent update of service with out-of-range NodePort: %v", result) } - expectedErr := fmt.Sprintf("Service \"%s\" is invalid: spec.ports[0].nodePort: invalid value '%d': provided port is not in the valid range", serviceName, outOfRangeNodePort) + expectedErr := fmt.Sprintf("Service \"%s\" is invalid: spec.ports[0].nodePort: invalid value '%d', Details: provided port is not in the valid range", serviceName, outOfRangeNodePort) Expect(fmt.Sprintf("%v", err)).To(Equal(expectedErr)) By("deleting original service " + serviceName)