Fix exit code on service delete and revision delete (#971)

* Fix exit code 0 upon service delete

* Mentioned bug fix in CHANGELOG.adoc

* Add error check test for service not found

* Fix for kn revision delete failure exit code and add test cases

* Testing changes in test cases for failure in intergration tests

* Fix test case error causing integration test failure

CHANGELOG.adoc

@@ -34,6 +34,10 @@
 | Fix Missing NAMESPACE column header for 'kn source list -A'
 | https://github.com/knative/client/pull/951[#951]
 
+| 🐛
+| Fix exit code for `kn service delete` and `kn revision delete` failures
+| https://github.com/knative/client/pull/971[#971]
+
 |===
 
 ## v0.16.0 (2020-07-14)

lib/test/revision.go

@@ -60,11 +60,11 @@ func RevisionMultipleDelete(r *KnRunResultCollector, existRevision1, existRevisi
 	assert.Check(r.T(), strings.Contains(out.Stdout, existRevision2), "Required revision2 does not exist")
 
 	out = r.KnTest().Kn().Run("revision", "delete", existRevision1, existRevision2, nonexistRevision)
-	r.AssertNoError(out)
+	r.AssertError(out)
 
 	assert.Check(r.T(), util.ContainsAll(out.Stdout, "Revision", existRevision1, "deleted", "namespace", r.KnTest().Kn().Namespace()), "Failed to get 'deleted' first revision message")
 	assert.Check(r.T(), util.ContainsAll(out.Stdout, "Revision", existRevision2, "deleted", "namespace", r.KnTest().Kn().Namespace()), "Failed to get 'deleted' second revision message")
-	assert.Check(r.T(), util.ContainsAll(out.Stdout, "revisions.serving.knative.dev", nonexistRevision, "not found"), "Failed to get 'not found' error")
+	assert.Check(r.T(), util.ContainsAll(out.Stderr, "revisions.serving.knative.dev", nonexistRevision, "not found"), "Failed to get 'not found' error")
 }
 
 // RevisionDescribeWithPrintFlags verifies describing given revision using print flag '--output=name'

pkg/kn/commands/revision/delete.go

@@ -17,6 +17,7 @@ package revision
 import (
 	"errors"
 	"fmt"
+	"strings"
 	"time"
 
 	"github.com/spf13/cobra"
@@ -47,18 +48,22 @@ func NewRevisionDeleteCommand(p *commands.KnParams) *cobra.Command {
 				return err
 			}
 
+			errs := []string{}
 			for _, name := range args {
 				timeout := time.Duration(0)
 				if waitFlags.Wait {
 					timeout = time.Duration(waitFlags.TimeoutInSeconds) * time.Second
 				}
 				err = client.DeleteRevision(name, timeout)
 				if err != nil {
-					fmt.Fprintf(cmd.OutOrStdout(), "%s.\n", err)
+					errs = append(errs, err.Error())
 				} else {
 					fmt.Fprintf(cmd.OutOrStdout(), "Revision '%s' deleted in namespace '%s'.\n", name, namespace)
 				}
 			}
+			if len(errs) > 0 {
+				return errors.New("Error: " + strings.Join(errs, "\nError: "))
+			}
 			return nil
 		},
 	}

pkg/kn/commands/revision/delete_test.go

@@ -25,7 +25,9 @@ import (
 	clienttesting "k8s.io/client-go/testing"
 
 	"knative.dev/client/pkg/kn/commands"
+	clientservingv1 "knative.dev/client/pkg/serving/v1"
 	"knative.dev/client/pkg/util"
+	"knative.dev/client/pkg/util/mock"
 	"knative.dev/client/pkg/wait"
 	servingv1 "knative.dev/serving/pkg/apis/serving/v1"
 )
@@ -102,3 +104,23 @@ func getRevisionDeleteEvents(name string) []watch.Event {
 		{watch.Deleted, &servingv1.Revision{ObjectMeta: metav1.ObjectMeta{Name: name}}},
 	}
 }
+
+func TestRevisionDeleteCheckErrorForNotFoundRevisionsMock(t *testing.T) {
+	// New mock client
+	client := clientservingv1.NewMockKnServiceClient(t)
+
+	// Recording:
+	r := client.Recorder()
+
+	r.DeleteRevision("foo", mock.Any(), nil)
+	r.DeleteRevision("bar", mock.Any(), errors.New("revisions.serving.knative.dev \"bar\" not found."))
+	r.DeleteRevision("baz", mock.Any(), errors.New("revisions.serving.knative.dev \"baz\" not found."))
+
+	output, err := executeRevisionCommand(client, "delete", "foo", "bar", "baz")
+	if err == nil {
+		t.Fatal("Expected revision not found error, returned nil")
+	}
+	assert.Assert(t, util.ContainsAll(output, "'foo' deleted", "\"bar\" not found", "\"baz\" not found"))
+
+	r.Validate()
+}

pkg/kn/commands/revision/revision_test.go

@@ -15,15 +15,24 @@
 package revision
 
 import (
+	"bytes"
 	"strings"
 	"testing"
 
+	"github.com/spf13/cobra"
 	"gotest.tools/assert"
+	"k8s.io/client-go/tools/clientcmd"
+	knflags "knative.dev/client/pkg/kn/flags"
 	servingv1 "knative.dev/serving/pkg/apis/serving/v1"
 
+	"knative.dev/client/pkg/kn/commands"
+	clientservingv1 "knative.dev/client/pkg/serving/v1"
 	"knative.dev/client/pkg/util"
 )
 
+// Helper methods
+var blankConfig clientcmd.ClientConfig
+
 func TestExtractTrafficAndTag(t *testing.T) {
 
 	service := &servingv1.Service{
@@ -52,3 +61,23 @@ func createTarget(rev string, percent int64, tag string) servingv1.TrafficTarget
 		Percent:      &percent,
 	}
 }
+
+func executeRevisionCommand(client clientservingv1.KnServingClient, args ...string) (string, error) {
+	knParams := &commands.KnParams{}
+	knParams.ClientConfig = blankConfig
+
+	output := new(bytes.Buffer)
+	knParams.Output = output
+	knParams.NewServingClient = func(namespace string) (clientservingv1.KnServingClient, error) {
+		return client, nil
+	}
+	cmd := NewRevisionCommand(knParams)
+	cmd.SetArgs(args)
+	cmd.SetOutput(output)
+
+	cmd.PersistentPreRunE = func(cmd *cobra.Command, args []string) error {
+		return knflags.ReconcileBoolFlags(cmd.Flags())
+	}
+	err := cmd.Execute()
+	return output.String(), err
+}

pkg/kn/commands/service/delete.go

@@ -17,6 +17,7 @@ package service
 import (
 	"errors"
 	"fmt"
+	"strings"
 	"time"
 
 	"github.com/spf13/cobra"
@@ -77,18 +78,22 @@ func NewServiceDeleteCommand(p *commands.KnParams) *cobra.Command {
 				}
 			}
 
+			errs := []string{}
 			for _, name := range args {
 				timeout := time.Duration(0)
 				if waitFlags.Wait {
 					timeout = time.Duration(waitFlags.TimeoutInSeconds) * time.Second
 				}
 				err = client.DeleteService(name, timeout)
 				if err != nil {
-					fmt.Fprintf(cmd.OutOrStdout(), "%s.\n", err)
+					errs = append(errs, err.Error())
 				} else {
 					fmt.Fprintf(cmd.OutOrStdout(), "Service '%s' successfully deleted in namespace '%s'.\n", name, namespace)
 				}
 			}
+			if len(errs) > 0 {
+				return errors.New("Error: " + strings.Join(errs, "\nError: "))
+			}
 			return nil
 		},
 	}

pkg/kn/commands/service/delete_mock_test.go

@@ -15,6 +15,7 @@
 package service
 
 import (
+	"errors"
 	"testing"
 
 	"gotest.tools/assert"
@@ -141,3 +142,23 @@ func TestServiceDeleteNoSvcNameMock(t *testing.T) {
 	r.Validate()
 
 }
+
+func TestServiceDeleteCheckErrorForNotFoundServicesMock(t *testing.T) {
+	// New mock client
+	client := clientservingv1.NewMockKnServiceClient(t)
+
+	// Recording:
+	r := client.Recorder()
+
+	r.DeleteService("foo", mock.Any(), nil)
+	r.DeleteService("bar", mock.Any(), errors.New("services.serving.knative.dev \"bar\" not found."))
+	r.DeleteService("baz", mock.Any(), errors.New("services.serving.knative.dev \"baz\" not found."))
+
+	output, err := executeServiceCommand(client, "delete", "foo", "bar", "baz")
+	if err == nil {
+		t.Fatal("Expected service not found error, returned nil")
+	}
+	assert.Assert(t, util.ContainsAll(output, "'foo' successfully deleted", "\"bar\" not found", "\"baz\" not found"))
+
+	r.Validate()
+}

test/e2e/service_test.go

@@ -125,8 +125,8 @@ func serviceDeleteNonexistent(r *test.KnRunResultCollector, serviceName string)
 	assert.Check(r.T(), !strings.Contains(out.Stdout, serviceName), "The service exists")
 
 	out = r.KnTest().Kn().Run("service", "delete", serviceName)
-	r.AssertNoError(out)
-	assert.Check(r.T(), util.ContainsAll(out.Stdout, "hello", "not found"), "Failed to get 'not found' error")
+	r.AssertError(out)
+	assert.Check(r.T(), util.ContainsAll(out.Stderr, "hello", "not found"), "Failed to get 'not found' error")
 }
 
 func serviceMultipleDelete(r *test.KnRunResultCollector, existService, nonexistService string) {
@@ -136,12 +136,12 @@ func serviceMultipleDelete(r *test.KnRunResultCollector, existService, nonexistS
 	assert.Check(r.T(), !strings.Contains(out.Stdout, nonexistService), "The service", nonexistService, " exists (but is supposed to be not)")
 
 	out = r.KnTest().Kn().Run("service", "delete", existService, nonexistService)
-	r.AssertNoError(out)
+	r.AssertError(out)
 
 	expectedSuccess := fmt.Sprintf(`Service '%s' successfully deleted in namespace '%s'.`, existService, r.KnTest().Kn().Namespace())
 	expectedErr := fmt.Sprintf(`services.serving.knative.dev "%s" not found`, nonexistService)
 	assert.Check(r.T(), strings.Contains(out.Stdout, expectedSuccess), "Failed to get 'successfully deleted' message")
-	assert.Check(r.T(), strings.Contains(out.Stdout, expectedErr), "Failed to get 'not found' error")
+	assert.Check(r.T(), strings.Contains(out.Stderr, expectedErr), "Failed to get 'not found' error")
 }
 
 func serviceUntagTagThatDoesNotExist(r *test.KnRunResultCollector, serviceName string) {