Repository: knative/client (go)

Issue:
"kn service delete" exited with code ZERO when it ran into failure
<!-- If you need to report a security issue with Knative, send an email to knative-security@googlegroups.com. -->

### Bug report

```
➜  ~ kn service delete hello
Internal error occurred: failed calling webhook "validation.webhook.serving.knative.dev": Post https://webhook.knative-serving.svc:443/resource-validation?timeout=10s: context deadline exceeded.
➜  ~ echo $?
0
➜  ~ kn service delete hello --no-wait
Internal error occurred: failed calling webhook "validation.webhook.serving.knative.dev": Post https://webhook.knative-serving.svc:443/resource-validation?timeout=10s: context deadline exceeded.
➜  ~ echo $?
0
➜  ~ kn service delete abc
services.serving.knative.dev "abc" not found.
➜  ~ echo $?
0
```

### Expected behavior

I suppose I should get a return code "1" for the delete failure.

### Steps to reproduce the problem

Delete n nonexistent ksvc:
```
➜  ~ kn service delete abc
services.serving.knative.dev "abc" not found.
➜  ~ echo $?
0
```

### kn version

```
➜  ~ kn version
Version:      v0.16.0
Build Date:   2020-07-15 07:01:17
Git Revision: 5cb78308
Supported APIs:
* Serving
  - serving.knative.dev/v1 (knative-serving v0.16.0)
* Eventing
  - sources.knative.dev/v1alpha2 (knative-eventing v0.16.0)
  - eventing.knative.dev/v1beta1 (knative-eventing v0.16.0)
```

### Knative (serving/eventing) version

<!-- Remove all except the known affected versions of Knative running on the cluster on which you have detected the issue -->

> v0.16.0


Pull request description:
Fix exit code on service delete and revision delete
## Description

<!-- Please add a short summary about what the pull request is going to bring on the table -->
Fixed exit code issue on service delete

## Changes

<!-- Please add list of more detailed changes. These changes should be reflected also in the commit messages -->

* Fixed exit code to 1 which was earlier returned upon any failure of `kn service delete`

## Reference

<!-- Please add the corresponding issue number which this pull request is about to fix -->
Fixes #960

<!--
Please add an entrty to CHANGELOG.adoc file, too, as part of your Pull Request.

In the following cases, add a short description of PR to the unreleased section in CHANGELOG.adoc:

- 🎁 New feature
- 🐛 Bug fix
- ✨ Feature Update
- 🐣 Refactoring
- 🗑️ Remove feature or internal logic

See other entries in CHANGELOG.adoc as an example for how to add the entry, including a reference to the corresponding issue/PR

PLEASE DON'T ADD THAT LINE HERE IN THE PULL-REQUEST DESCRIPTION BUT DIRECTLY IN CHANGELOG.ADOC AND ADD CHANGELOG.ADOC AS PART OF YOUR PULL-REQUEST.
-->

<!--
To automatically lint go code in this pull request uncomment the line below. You get feedback as comments on your pull request then -->

<!--
/lint
-->


Patch:
diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc
index 60bf8eb5..21a8c2d2 100644
--- a/CHANGELOG.adoc
+++ b/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)
diff --git a/pkg/kn/commands/revision/delete.go b/pkg/kn/commands/revision/delete.go
index 78d101ca..9e6875f7 100644
--- a/pkg/kn/commands/revision/delete.go
+++ b/pkg/kn/commands/revision/delete.go
@@ -17,6 +17,7 @@ package revision
 import (
 	"errors"
 	"fmt"
+	"strings"
 	"time"
 
 	"github.com/spf13/cobra"
@@ -47,6 +48,7 @@ func NewRevisionDeleteCommand(p *commands.KnParams) *cobra.Command {
 				return err
 			}
 
+			errs := []string{}
 			for _, name := range args {
 				timeout := time.Duration(0)
 				if waitFlags.Wait {
@@ -54,11 +56,14 @@ func NewRevisionDeleteCommand(p *commands.KnParams) *cobra.Command {
 				}
 				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
 		},
 	}
diff --git a/pkg/kn/commands/service/delete.go b/pkg/kn/commands/service/delete.go
index 54953cf4..698cb52d 100644
--- a/pkg/kn/commands/service/delete.go
+++ b/pkg/kn/commands/service/delete.go
@@ -17,6 +17,7 @@ package service
 import (
 	"errors"
 	"fmt"
+	"strings"
 	"time"
 
 	"github.com/spf13/cobra"
@@ -77,6 +78,7 @@ func NewServiceDeleteCommand(p *commands.KnParams) *cobra.Command {
 				}
 			}
 
+			errs := []string{}
 			for _, name := range args {
 				timeout := time.Duration(0)
 				if waitFlags.Wait {
@@ -84,11 +86,14 @@ func NewServiceDeleteCommand(p *commands.KnParams) *cobra.Command {
 				}
 				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
 		},
 	}


Test patch:
diff --git a/lib/test/revision.go b/lib/test/revision.go
index dce199fd..24ed9c5d 100644
--- a/lib/test/revision.go
+++ b/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'
diff --git a/pkg/kn/commands/revision/delete_test.go b/pkg/kn/commands/revision/delete_test.go
index 66d5d9fb..6eb5363b 100644
--- a/pkg/kn/commands/revision/delete_test.go
+++ b/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()
+}
diff --git a/pkg/kn/commands/revision/revision_test.go b/pkg/kn/commands/revision/revision_test.go
index 63e49e70..27cbb994 100644
--- a/pkg/kn/commands/revision/revision_test.go
+++ b/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
+}
diff --git a/pkg/kn/commands/service/delete_mock_test.go b/pkg/kn/commands/service/delete_mock_test.go
index 8b5d918f..40eb44f2 100644
--- a/pkg/kn/commands/service/delete_mock_test.go
+++ b/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()
+}
diff --git a/test/e2e/service_test.go b/test/e2e/service_test.go
index 128cb10c..b951d619 100644
--- a/test/e2e/service_test.go
+++ b/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) {