From 980a6a946983fbb601e44548eb1ac0bbc15faf1e Mon Sep 17 00:00:00 2001 From: Himanshu Ranjan Date: Thu, 6 Aug 2020 16:11:28 +0530 Subject: [PATCH] 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 | 4 +++ lib/test/revision.go | 4 +-- pkg/kn/commands/revision/delete.go | 7 ++++- pkg/kn/commands/revision/delete_test.go | 22 ++++++++++++++++ pkg/kn/commands/revision/revision_test.go | 29 +++++++++++++++++++++ pkg/kn/commands/service/delete.go | 7 ++++- pkg/kn/commands/service/delete_mock_test.go | 21 +++++++++++++++ test/e2e/service_test.go | 8 +++--- 8 files changed, 94 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index 60bf8eb584..21a8c2d28e 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/lib/test/revision.go b/lib/test/revision.go index dce199fd45..24ed9c5d6f 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.go b/pkg/kn/commands/revision/delete.go index 78d101cabf..9e6875f709 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/revision/delete_test.go b/pkg/kn/commands/revision/delete_test.go index 66d5d9fb23..6eb5363bcf 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 63e49e70a3..27cbb9942d 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.go b/pkg/kn/commands/service/delete.go index 54953cf4e6..698cb52da8 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 }, } diff --git a/pkg/kn/commands/service/delete_mock_test.go b/pkg/kn/commands/service/delete_mock_test.go index 8b5d918fae..40eb44f20b 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 128cb10c3b..b951d619f8 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) {