diff --git a/hack/testdata/invalid-pod.yaml b/hack/testdata/invalid-pod.yaml new file mode 100644 index 00000000000..0aa88a7edd3 --- /dev/null +++ b/hack/testdata/invalid-pod.yaml @@ -0,0 +1,8 @@ +apiVersion: v1 +kind: Pod +metadata: + name: test +spec: + containers: + - name: + image: nginx diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/diff/diff.go b/staging/src/k8s.io/kubectl/pkg/cmd/diff/diff.go index ac762a71189..139f33a72c5 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/diff/diff.go +++ b/staging/src/k8s.io/kubectl/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/staging/src/k8s.io/kubectl/pkg/cmd/util/helpers.go b/staging/src/k8s.io/kubectl/pkg/cmd/util/helpers.go index daee405272d..a3e338430fe 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/util/helpers.go +++ b/staging/src/k8s.io/kubectl/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)) { diff --git a/test/cmd/diff.sh b/test/cmd/diff.sh index 1ac63837278..e56a08eacf2 100755 --- a/test/cmd/diff.sh +++ b/test/cmd/diff.sh @@ -32,9 +32,18 @@ run_kubectl_diff_tests() { kubectl apply -f hack/testdata/pod.yaml - output_message=$(! kubectl diff -f hack/testdata/pod-changed.yaml) + # Make sure that diffing the resource right after returns nothing (0 exit code). + kubectl diff -f hack/testdata/pod.yaml + + # Make sure that: + # 1. the exit code for diff is 1 because it found a difference + # 2. the difference contains the changed image + output_message=$(kubectl diff -f hack/testdata/pod-changed.yaml || test $? -eq 1) kube::test::if_has_string "${output_message}" 'k8s.gcr.io/pause:3.0' + # Test that we have a return code bigger than 1 if there is an error when diffing + kubectl diff -f hack/testdata/invalid-pod.yaml || test $? -gt 1 + kubectl delete -f hack/testdata/pod.yaml set +o nounset