Rectify kubectl error output

pkg/kubectl/cmd/cmd_test.go

@@ -46,8 +46,8 @@ import (
 )
 
 func initTestErrorHandler(t *testing.T) {
-	cmdutil.BehaviorOnFatal(func(str string) {
+	cmdutil.BehaviorOnFatal(func(str string, code int) {
-		t.Errorf("Error running command: %s", str)
+		t.Errorf("Error running command (exit code %d): %s", code, str)
 	})
 }
 

pkg/kubectl/cmd/drain_test.go

@@ -177,7 +177,7 @@ func TestCordon(t *testing.T) {
 				// Restore cmdutil behavior
 				cmdutil.DefaultBehaviorOnFatal()
 			}()
-			cmdutil.BehaviorOnFatal(func(e string) { saw_fatal = true; panic(e) })
+			cmdutil.BehaviorOnFatal(func(e string, code int) { saw_fatal = true; panic(e) })
 			cmd.SetArgs([]string{test.arg})
 			cmd.Execute()
 		}()
@@ -521,7 +521,7 @@ func TestDrain(t *testing.T) {
 				// Restore cmdutil behavior
 				cmdutil.DefaultBehaviorOnFatal()
 			}()
-			cmdutil.BehaviorOnFatal(func(e string) { saw_fatal = true; panic(e) })
+			cmdutil.BehaviorOnFatal(func(e string, code int) { saw_fatal = true; panic(e) })
 			cmd.SetArgs(test.args)
 			cmd.Execute()
 		}()

pkg/kubectl/cmd/taint_test.go

@@ -296,7 +296,7 @@ func TestTaint(t *testing.T) {
 				// Restore cmdutil behavior
 				cmdutil.DefaultBehaviorOnFatal()
 			}()
-			cmdutil.BehaviorOnFatal(func(e string) { saw_fatal = true; panic(e) })
+			cmdutil.BehaviorOnFatal(func(e string, code int) { saw_fatal = true; panic(e) })
 			cmd.SetArgs(test.args)
 			cmd.Execute()
 		}()

pkg/kubectl/cmd/util/helpers.go

@@ -50,6 +50,7 @@ import (
 
 const (
 	ApplyAnnotationsFlag = "save-config"
+	DefaultErrorExitCode = 1
 )
 
 type debugError interface {
@@ -74,9 +75,9 @@ func AddSourceToErr(verb string, source string, err error) error {
 var fatalErrHandler = fatal
 
 // BehaviorOnFatal allows you to override the default behavior when a fatal
-// error occurs, which is call os.Exit(1). You can pass 'panic' as a function
+// error occurs, which is to call os.Exit(code). You can pass 'panic' as a function
 // here if you prefer the panic() over os.Exit(1).
-func BehaviorOnFatal(f func(string)) {
+func BehaviorOnFatal(f func(string, int)) {
 	fatalErrHandler = f
 }
 
@@ -86,19 +87,21 @@ func DefaultBehaviorOnFatal() {
 	fatalErrHandler = fatal
 }
 
-// fatal prints the message and then exits. If V(2) or greater, glog.Fatal
+// fatal prints the message if set and then exits. If V(2) or greater, glog.Fatal
 // is invoked for extended information.
-func fatal(msg string) {
+func fatal(msg string, code int) {
-	// add newline if needed
+	if len(msg) > 0 {
-	if !strings.HasSuffix(msg, "\n") {
+		// add newline if needed
-		msg += "\n"
+		if !strings.HasSuffix(msg, "\n") {
-	}
+			msg += "\n"
+		}
 
-	if glog.V(2) {
+		if glog.V(2) {
-		glog.FatalDepth(2, msg)
+			glog.FatalDepth(2, msg)
+		}
+		fmt.Fprint(os.Stderr, msg)
 	}
-	fmt.Fprint(os.Stderr, msg)
+	os.Exit(code)
-	os.Exit(1)
 }
 
 // CheckErr prints a user friendly error to STDERR and exits with a non-zero
@@ -115,51 +118,49 @@ func checkErrWithPrefix(prefix string, err error) {
 	checkErr(prefix, err, fatalErrHandler)
 }
 
-func checkErr(pref string, err error, handleErr func(string)) {
+// checkErr formats a given error as a string and calls the passed handleErr
-	if err == nil {
+// func with that string and an kubectl exit code.
+func checkErr(prefix string, err error, handleErr func(string, int)) {
+	switch {
+	case err == nil:
 		return
-	}
+	case kerrors.IsInvalid(err):
-
-	if kerrors.IsInvalid(err) {
 		details := err.(*kerrors.StatusError).Status().Details
-		prefix := fmt.Sprintf("%sThe %s %q is invalid.\n", pref, details.Kind, details.Name)
+		s := fmt.Sprintf("%sThe %s %q is invalid", prefix, details.Kind, details.Name)
-		errs := statusCausesToAggrError(details.Causes)
+		if len(details.Causes) > 0 {
-		handleErr(MultilineError(prefix, errs))
+			errs := statusCausesToAggrError(details.Causes)
-		return
+			handleErr(MultilineError(s+": ", errs), DefaultErrorExitCode)
-	}
+		} else {
-
+			handleErr(s, DefaultErrorExitCode)
-	if noMatch, ok := err.(*meta.NoResourceMatchError); ok {
-		switch {
-		case len(noMatch.PartialResource.Group) > 0 && len(noMatch.PartialResource.Version) > 0:
-			handleErr(fmt.Sprintf("%sthe server doesn't have a resource type %q in group %q and version %q", pref, noMatch.PartialResource.Resource, noMatch.PartialResource.Group, noMatch.PartialResource.Version))
-		case len(noMatch.PartialResource.Group) > 0:
-			handleErr(fmt.Sprintf("%sthe server doesn't have a resource type %q in group %q", pref, noMatch.PartialResource.Resource, noMatch.PartialResource.Group))
-		case len(noMatch.PartialResource.Version) > 0:
-			handleErr(fmt.Sprintf("%sthe server doesn't have a resource type %q in version %q", pref, noMatch.PartialResource.Resource, noMatch.PartialResource.Version))
-		default:
-			handleErr(fmt.Sprintf("%sthe server doesn't have a resource type %q", pref, noMatch.PartialResource.Resource))
 		}
-		return
+	case clientcmd.IsConfigurationInvalid(err):
-	}
+		handleErr(MultilineError(fmt.Sprintf("%sError in configuration: ", prefix), err), DefaultErrorExitCode)
-
+	default:
-	// handle multiline errors
+		switch err := err.(type) {
-	if clientcmd.IsConfigurationInvalid(err) {
+		case *meta.NoResourceMatchError:
-		handleErr(MultilineError(fmt.Sprintf("%sError in configuration: ", pref), err))
+			switch {
-		return
+			case len(err.PartialResource.Group) > 0 && len(err.PartialResource.Version) > 0:
-	}
+				handleErr(fmt.Sprintf("%sthe server doesn't have a resource type %q in group %q and version %q", prefix, err.PartialResource.Resource, err.PartialResource.Group, err.PartialResource.Version), DefaultErrorExitCode)
-	if agg, ok := err.(utilerrors.Aggregate); ok && len(agg.Errors()) > 0 {
+			case len(err.PartialResource.Group) > 0:
-		handleErr(MultipleErrors(pref, agg.Errors()))
+				handleErr(fmt.Sprintf("%sthe server doesn't have a resource type %q in group %q", prefix, err.PartialResource.Resource, err.PartialResource.Group), DefaultErrorExitCode)
-		return
+			case len(err.PartialResource.Version) > 0:
-	}
+				handleErr(fmt.Sprintf("%sthe server doesn't have a resource type %q in version %q", prefix, err.PartialResource.Resource, err.PartialResource.Version), DefaultErrorExitCode)
-
+			default:
-	msg, ok := StandardErrorMessage(err)
+				handleErr(fmt.Sprintf("%sthe server doesn't have a resource type %q", prefix, err.PartialResource.Resource), DefaultErrorExitCode)
-	if !ok {
+			}
-		msg = err.Error()
+		case utilerrors.Aggregate:
-		if !strings.HasPrefix(msg, "error: ") {
+			handleErr(MultipleErrors(prefix, err.Errors()), DefaultErrorExitCode)
-			msg = fmt.Sprintf("error: %s", msg)
+		default: // for any other error type
+			msg, ok := StandardErrorMessage(err)
+			if !ok {
+				msg = err.Error()
+				if !strings.HasPrefix(msg, "error: ") {
+					msg = fmt.Sprintf("error: %s", msg)
+				}
+			}
+			handleErr(msg, DefaultErrorExitCode)
 		}
 	}
-	handleErr(fmt.Sprintf("%s%s", pref, msg))
 }
 
 func statusCausesToAggrError(scs []unversioned.StatusCause) utilerrors.Aggregate {

pkg/kubectl/cmd/util/helpers_test.go

@@ -213,91 +213,78 @@ func (f *fileHandler) ServeHTTP(res http.ResponseWriter, req *http.Request) {
 	res.Write(f.data)
 }
 
+type checkErrTestCase struct {
+	err          error
+	expectedErr  string
+	expectedCode int
+}
+
 func TestCheckInvalidErr(t *testing.T) {
-	tests := []struct {
+	testCheckError(t, []checkErrTestCase{
-		err      error
-		expected string
-	}{
 		{
 			errors.NewInvalid(api.Kind("Invalid1"), "invalidation", field.ErrorList{field.Invalid(field.NewPath("field"), "single", "details")}),
-			`error: The Invalid1 "invalidation" is invalid. field: Invalid value: "single": details`,
+			"The Invalid1 \"invalidation\" is invalid: field: Invalid value: \"single\": details\n",
+			DefaultErrorExitCode,
 		},
 		{
 			errors.NewInvalid(api.Kind("Invalid2"), "invalidation", field.ErrorList{field.Invalid(field.NewPath("field1"), "multi1", "details"), field.Invalid(field.NewPath("field2"), "multi2", "details")}),
-			`error: The Invalid2 "invalidation" is invalid. * field1: Invalid value: "multi1": details, * field2: Invalid value: "multi2": details`,
+			"The Invalid2 \"invalidation\" is invalid: \n* field1: Invalid value: \"multi1\": details\n* field2: Invalid value: \"multi2\": details\n",
+			DefaultErrorExitCode,
 		},
 		{
 			errors.NewInvalid(api.Kind("Invalid3"), "invalidation", field.ErrorList{}),
-			`error: The Invalid3 "invalidation" is invalid. %!s(<nil>)`,
+			"The Invalid3 \"invalidation\" is invalid",
+			DefaultErrorExitCode,
 		},
 		{
 			errors.NewInvalid(api.Kind("Invalid4"), "invalidation", field.ErrorList{field.Invalid(field.NewPath("field4"), "multi4", "details"), field.Invalid(field.NewPath("field4"), "multi4", "details")}),
-			`error: The Invalid4 "invalidation" is invalid. field4: Invalid value: "multi4": details`,
+			"The Invalid4 \"invalidation\" is invalid: field4: Invalid value: \"multi4\": details\n",
+			DefaultErrorExitCode,
 		},
-	}
+	})
-
-	var errReturned string
-	errHandle := func(err string) {
-		for _, v := range strings.Split(err, "\n") {
-			separator := " "
-			if errReturned == "" || v == "" {
-				separator = ""
-			} else if !strings.HasSuffix(errReturned, ".") {
-				separator = ", "
-			}
-			errReturned = fmt.Sprintf("%s%s%s", errReturned, separator, v)
-		}
-		if !strings.HasPrefix(errReturned, "error: ") {
-			errReturned = fmt.Sprintf("error: %s", errReturned)
-		}
-		if strings.HasSuffix(errReturned, ", ") {
-			errReturned = errReturned[:len(errReturned)-len(" ,")]
-		}
-	}
-
-	for _, test := range tests {
-		checkErr("", test.err, errHandle)
-
-		if errReturned != test.expected {
-			t.Fatalf("Got: %s, expected: %s", errReturned, test.expected)
-		}
-		errReturned = ""
-	}
 }
 
 func TestCheckNoResourceMatchError(t *testing.T) {
-	tests := []struct {
+	testCheckError(t, []checkErrTestCase{
-		err      error
-		expected string
-	}{
 		{
 			&meta.NoResourceMatchError{PartialResource: unversioned.GroupVersionResource{Resource: "foo"}},
 			`the server doesn't have a resource type "foo"`,
+			DefaultErrorExitCode,
 		},
 		{
 			&meta.NoResourceMatchError{PartialResource: unversioned.GroupVersionResource{Version: "theversion", Resource: "foo"}},
 			`the server doesn't have a resource type "foo" in version "theversion"`,
+			DefaultErrorExitCode,
 		},
 		{
 			&meta.NoResourceMatchError{PartialResource: unversioned.GroupVersionResource{Group: "thegroup", Version: "theversion", Resource: "foo"}},
 			`the server doesn't have a resource type "foo" in group "thegroup" and version "theversion"`,
+			DefaultErrorExitCode,
 		},
 		{
 			&meta.NoResourceMatchError{PartialResource: unversioned.GroupVersionResource{Group: "thegroup", Resource: "foo"}},
 			`the server doesn't have a resource type "foo" in group "thegroup"`,
+			DefaultErrorExitCode,
 		},
-	}
+	})
+}
 
+func testCheckError(t *testing.T, tests []checkErrTestCase) {
 	var errReturned string
-	errHandle := func(err string) {
+	var codeReturned int
+	errHandle := func(err string, code int) {
 		errReturned = err
+		codeReturned = code
 	}
 
 	for _, test := range tests {
 		checkErr("", test.err, errHandle)
 
-		if errReturned != test.expected {
+		if errReturned != test.expectedErr {
-			t.Fatalf("Got: %s, expected: %s", errReturned, test.expected)
+			t.Fatalf("Got: %s, expected: %s", errReturned, test.expectedErr)
+		}
+		if codeReturned != test.expectedCode {
+			t.Fatalf("Got: %d, expected: %d", codeReturned, test.expectedCode)
 		}
 	}
 }

pkg/util/exec/exec.go

@@ -113,7 +113,7 @@ func (cmd *cmdWrapper) Output() ([]byte, error) {
 func handleError(err error) error {
 	if ee, ok := err.(*osexec.ExitError); ok {
 		// Force a compile fail if exitErrorWrapper can't convert to ExitError.
-		var x ExitError = &exitErrorWrapper{ee}
+		var x ExitError = &ExitErrorWrapper{ee}
 		return x
 	}
 	if ee, ok := err.(*osexec.Error); ok {
@@ -124,14 +124,16 @@ func handleError(err error) error {
 	return err
 }
 
-// exitErrorWrapper is an implementation of ExitError in terms of os/exec ExitError.
+// ExitErrorWrapper is an implementation of ExitError in terms of os/exec ExitError.
 // Note: standard exec.ExitError is type *os.ProcessState, which already implements Exited().
-type exitErrorWrapper struct {
+type ExitErrorWrapper struct {
 	*osexec.ExitError
 }
 
+var _ ExitError = ExitErrorWrapper{}
+
 // ExitStatus is part of the ExitError interface.
-func (eew exitErrorWrapper) ExitStatus() int {
+func (eew ExitErrorWrapper) ExitStatus() int {
 	ws, ok := eew.Sys().(syscall.WaitStatus)
 	if !ok {
 		panic("can't call ExitStatus() on a non-WaitStatus exitErrorWrapper")