From 4c36396b352800dc6f797c05f60c7dd2a2ac8219 Mon Sep 17 00:00:00 2001
From: Antoine Pelisse <apelisse@google.com>
Date: Tue, 21 Jan 2020 13:52:41 -0800
Subject: [PATCH] kubectl-diff: Return non-1 errors on kubectl failures

Currently, diff AND kubectl can return 1 errors either when diff finds
differences, or when kubectl fails something. It also prints an ugly
error when diff finds a differences, since 1 is treated as an error.

Two things this PR does:

1. If diff returns 1, then do not treat it as an error, and exit with
exit code 1. It no longer prints the ugly error.

2. Kubectl errors are +1'd so that they never return 1 which shouldn't
be considered an error.

We need to update the documentation accordingly, to mention that
`KUBECTL_EXTERNAL_DIFF` programs must also follow the convention of
using one as their exit code for changes detected.

Kubernetes-commit: f2b21f08d95291212ba8987aa9d446c02a96089a
---
 pkg/cmd/diff/diff.go    | 31 ++++++++++++++++++++++++++++---
 pkg/cmd/util/helpers.go | 12 ++++++++++++
 2 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/pkg/cmd/diff/diff.go b/pkg/cmd/diff/diff.go
index ac762a71..139f33a7 100644
--- a/pkg/cmd/diff/diff.go
+++ b/pkg/cmd/diff/diff.go
@@ -68,6 +68,15 @@ var (
 // Number of times we try to diff before giving-up
 const maxRetries = 4
 
+// diffError returns the ExitError if the status code is less than 1,
+// nil otherwise.
+func diffError(err error) exec.ExitError {
+	if err, ok := err.(exec.ExitError); ok && err.ExitStatus() <= 1 {
+		return err
+	}
+	return nil
+}
+
 type DiffOptions struct {
 	FilenameOptions resource.FilenameOptions
 
@@ -109,9 +118,20 @@ func NewCmdDiff(f cmdutil.Factory, streams genericclioptions.IOStreams) *cobra.C
 		Long:                  diffLong,
 		Example:               diffExample,
 		Run: func(cmd *cobra.Command, args []string) {
-			cmdutil.CheckErr(options.Complete(f, cmd))
-			cmdutil.CheckErr(validateArgs(cmd, args))
-			cmdutil.CheckErr(options.Run())
+			cmdutil.CheckDiffErr(options.Complete(f, cmd))
+			cmdutil.CheckDiffErr(validateArgs(cmd, args))
+			// `kubectl diff` propagates the error code from
+			// diff or `KUBECTL_EXTERNAL_DIFF`. Also, we
+			// don't want to print an error if diff returns
+			// error code 1, which simply means that changes
+			// were found. We also don't want kubectl to
+			// return 1 if there was a problem.
+			if err := options.Run(); err != nil {
+				if exitErr := diffError(err); exitErr != nil {
+					os.Exit(exitErr.ExitStatus())
+				}
+				cmdutil.CheckDiffErr(err)
+			}
 		},
 	}
 
@@ -150,6 +170,11 @@ func (d *DiffProgram) getCommand(args ...string) (string, exec.Cmd) {
 func (d *DiffProgram) Run(from, to string) error {
 	diff, cmd := d.getCommand(from, to)
 	if err := cmd.Run(); err != nil {
+		// Let's not wrap diff errors, or we won't be able to
+		// differentiate them later.
+		if diffErr := diffError(err); diffErr != nil {
+			return diffErr
+		}
 		return fmt.Errorf("failed to run %q: %v", diff, err)
 	}
 	return nil
diff --git a/pkg/cmd/util/helpers.go b/pkg/cmd/util/helpers.go
index daee4052..a3e33843 100644
--- a/pkg/cmd/util/helpers.go
+++ b/pkg/cmd/util/helpers.go
@@ -115,6 +115,18 @@ func CheckErr(err error) {
 	checkErr(err, fatalErrHandler)
 }
 
+// CheckDiffErr prints a user friendly error to STDERR and exits with a
+// non-zero and non-one exit code. Unrecognized errors will be printed
+// with an "error: " prefix.
+//
+// This method is meant specifically for `kubectl diff` and may be used
+// by other commands.
+func CheckDiffErr(err error) {
+	checkErr(err, func(msg string, code int) {
+		fatalErrHandler(msg, code+1)
+	})
+}
+
 // checkErr formats a given error as a string and calls the passed handleErr
 // func with that string and an kubectl exit code.
 func checkErr(err error, handleErr func(string, int)) {