feat(wait): add wait for delete operation

CHANGELOG.adoc

@@ -45,13 +45,16 @@
 | https://github.com/knative/client/pull/639[#639]
 
 | 🧽
-| Refactor service `create_test.go` and `update_test.go` to
-| remove unecessary `sync` parameter in setup call
+| Refactor service `create_test.go` and `update_test.go` to remove unecessary `sync` parameter in setup call
 | https://github.com/knative/client/pull/656[#656]
 
 | 🐛
 | Fix `--image` flag to only allow single occurence
 | https://github.com/knative/client/pull/647[#647]
+
+| 🧽
+| Add `--wait` and `--no-wait` to service delete operation. Change service delete to wait by default.   
+| https://github.com/knative/client/pull/682[#682]
 |===
 
 ## v0.12.0 (2020-01-29)

docs/cmd/kn_service_create.md

@@ -44,7 +44,7 @@ kn service create NAME --image IMAGE [flags]
 ```
       --annotation stringArray    Service annotation to set. name=value; you may provide this flag any number of times to set multiple annotations. To unset, specify the annotation name followed by a "-" (e.g., name-).
       --arg stringArray           Add argument to the container command. Example: --arg myArg1 --arg --myArg2 --arg myArg3=3. You can use this flag multiple times.
-      --async                     DEPRECATED: please use --no-wait instead. Create service and don't wait for it to become ready.
+      --async                     DEPRECATED: please use --no-wait instead. Create service and don't wait for it to be ready.
       --autoscale-window string   Duration to look back for making auto-scaling decisions. The service is scaled to zero if no request was received in during that time. (eg: 10s)
       --cmd string                Specify command to be used as entrypoint instead of default one. Example: --cmd /app/start or --cmd /app/start --arg myArg to pass aditional arguments.
       --concurrency-limit int     Hard Limit of concurrent requests to be processed by a single replica.
@@ -63,7 +63,7 @@ kn service create NAME --image IMAGE [flags]
       --mount stringArray         Mount a ConfigMap (prefix cm: or config-map:), a Secret (prefix secret: or sc:), or an existing Volume (without any prefix) on the specified directory. Example: --mount /mydir=cm:myconfigmap, --mount /mydir=secret:mysecret, or --mount /mydir=myvolume. When a configmap or a secret is specified, a corresponding volume is automatically generated. You can use this flag multiple times. For unmounting a directory, append "-", e.g. --mount /mydir-, which also removes any auto-generated volume.
   -n, --namespace string          Specify the namespace to operate in.
       --no-lock-to-digest         Do not keep the running image for the service constant when not explicitly specifying the image. (--no-lock-to-digest pulls the image tag afresh with each new revision)
-      --no-wait                   Create service and don't wait for it to become ready.
+      --no-wait                   Create service and don't wait for it to be ready.
   -p, --port int32                The port where application listens on.
       --pull-secret string        Image pull secret to set. An empty argument ("") clears the pull secret. The referenced secret must exist in the service's namespace.
       --requests-cpu string       The requested CPU (e.g., 250m).

docs/cmd/kn_service_delete.md

@@ -24,8 +24,11 @@ kn service delete NAME [flags]
 ### Options
 
 ```
+      --async              DEPRECATED: please use --no-wait instead. Delete service and don't wait for it to be deleted.
   -h, --help               help for delete
   -n, --namespace string   Specify the namespace to operate in.
+      --no-wait            Delete service and don't wait for it to be deleted.
+      --wait-timeout int   Seconds to wait before giving up on waiting for service to be deleted. (default 600)
 ```
 
 ### Options inherited from parent commands

docs/cmd/kn_service_update.md

@@ -40,7 +40,7 @@ kn service update NAME [flags]
 ```
       --annotation stringArray    Service annotation to set. name=value; you may provide this flag any number of times to set multiple annotations. To unset, specify the annotation name followed by a "-" (e.g., name-).
       --arg stringArray           Add argument to the container command. Example: --arg myArg1 --arg --myArg2 --arg myArg3=3. You can use this flag multiple times.
-      --async                     DEPRECATED: please use --no-wait instead. Update service and don't wait for it to become ready.
+      --async                     DEPRECATED: please use --no-wait instead. Update service and don't wait for it to be ready.
       --autoscale-window string   Duration to look back for making auto-scaling decisions. The service is scaled to zero if no request was received in during that time. (eg: 10s)
       --cmd string                Specify command to be used as entrypoint instead of default one. Example: --cmd /app/start or --cmd /app/start --arg myArg to pass aditional arguments.
       --concurrency-limit int     Hard Limit of concurrent requests to be processed by a single replica.
@@ -58,7 +58,7 @@ kn service update NAME [flags]
       --mount stringArray         Mount a ConfigMap (prefix cm: or config-map:), a Secret (prefix secret: or sc:), or an existing Volume (without any prefix) on the specified directory. Example: --mount /mydir=cm:myconfigmap, --mount /mydir=secret:mysecret, or --mount /mydir=myvolume. When a configmap or a secret is specified, a corresponding volume is automatically generated. You can use this flag multiple times. For unmounting a directory, append "-", e.g. --mount /mydir-, which also removes any auto-generated volume.
   -n, --namespace string          Specify the namespace to operate in.
       --no-lock-to-digest         Do not keep the running image for the service constant when not explicitly specifying the image. (--no-lock-to-digest pulls the image tag afresh with each new revision)
-      --no-wait                   Update service and don't wait for it to become ready.
+      --no-wait                   Update service and don't wait for it to be ready.
   -p, --port int32                The port where application listens on.
       --pull-secret string        Image pull secret to set. An empty argument ("") clears the pull secret. The referenced secret must exist in the service's namespace.
       --requests-cpu string       The requested CPU (e.g., 250m).

pkg/kn/commands/service/create.go

@@ -114,7 +114,7 @@ func NewServiceCreateCommand(p *commands.KnParams) *cobra.Command {
 	}
 	commands.AddNamespaceFlags(serviceCreateCommand.Flags(), false)
 	editFlags.AddCreateFlags(serviceCreateCommand)
-	waitFlags.AddConditionWaitFlags(serviceCreateCommand, commands.WaitDefaultTimeout, "Create", "service")
+	waitFlags.AddConditionWaitFlags(serviceCreateCommand, commands.WaitDefaultTimeout, "Create", "service", "ready")
 	return serviceCreateCommand
 }
 

pkg/kn/commands/service/delete.go

@@ -17,6 +17,7 @@ package service
 import (
 	"errors"
 	"fmt"
+	"time"
 
 	"github.com/spf13/cobra"
 
@@ -25,6 +26,8 @@ import (
 
 // NewServiceDeleteCommand represent 'service delete' command
 func NewServiceDeleteCommand(p *commands.KnParams) *cobra.Command {
+	var waitFlags commands.WaitFlags
+
 	serviceDeleteCommand := &cobra.Command{
 		Use:   "delete NAME",
 		Short: "Delete a service.",
@@ -48,9 +51,12 @@ func NewServiceDeleteCommand(p *commands.KnParams) *cobra.Command {
 			if err != nil {
 				return err
 			}
-
 			for _, name := range args {
-				err = client.DeleteService(name)
+				timeout := time.Duration(0)
+				if !waitFlags.NoWait {
+					timeout = time.Duration(waitFlags.TimeoutInSeconds) * time.Second
+				}
+				err = client.DeleteService(name, timeout)
 				if err != nil {
 					fmt.Fprintf(cmd.OutOrStdout(), "%s.\n", err)
 				} else {
@@ -61,5 +67,6 @@ func NewServiceDeleteCommand(p *commands.KnParams) *cobra.Command {
 		},
 	}
 	commands.AddNamespaceFlags(serviceDeleteCommand.Flags(), false)
+	waitFlags.AddConditionWaitFlags(serviceDeleteCommand, commands.WaitDefaultTimeout, "Delete", "service", "deleted")
 	return serviceDeleteCommand
 }

pkg/kn/commands/service/delete_mock_test.go

@@ -21,6 +21,7 @@ import (
 
 	clientservingv1 "knative.dev/client/pkg/serving/v1"
 	"knative.dev/client/pkg/util"
+	"knative.dev/client/pkg/util/mock"
 )
 
 func TestServiceDeleteMock(t *testing.T) {
@@ -30,7 +31,7 @@ func TestServiceDeleteMock(t *testing.T) {
 	// Recording:
 	r := client.Recorder()
 
-	r.DeleteService("foo", nil)
+	r.DeleteService("foo", mock.Any(), nil)
 
 	output, err := executeServiceCommand(client, "delete", "foo")
 	assert.NilError(t, err)
@@ -40,16 +41,34 @@ func TestServiceDeleteMock(t *testing.T) {
 
 }
 
+func TestServiceDeleteMockNoWait(t *testing.T) {
+	// New mock client
+	client := clientservingv1.NewMockKnServiceClient(t)
+
+	// Recording:
+	r := client.Recorder()
+
+	r.DeleteService("foo", mock.Any(), nil)
+
+	output, err := executeServiceCommand(client, "delete", "foo", "--no-wait")
+	assert.NilError(t, err)
+	assert.Assert(t, util.ContainsAll(output, "deleted", "foo", "default"))
+
+	r.Validate()
+
+}
+
 func TestMultipleServiceDeleteMock(t *testing.T) {
 	// New mock client
 	client := clientservingv1.NewMockKnServiceClient(t)
 
 	// Recording:
 	r := client.Recorder()
+	// Wait for delete event
 
-	r.DeleteService("foo", nil)
-	r.DeleteService("bar", nil)
-	r.DeleteService("baz", nil)
+	r.DeleteService("foo", mock.Any(), nil)
+	r.DeleteService("bar", mock.Any(), nil)
+	r.DeleteService("baz", mock.Any(), nil)
 
 	output, err := executeServiceCommand(client, "delete", "foo", "bar", "baz")
 	assert.NilError(t, err)

pkg/kn/commands/service/delete_test.go

@@ -17,12 +17,17 @@ package service
 import (
 	"testing"
 
+	"github.com/pkg/errors"
 	"gotest.tools/assert"
+	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/watch"
 	client_testing "k8s.io/client-go/testing"
+	clienttesting "k8s.io/client-go/testing"
 
 	"knative.dev/client/pkg/kn/commands"
 	"knative.dev/client/pkg/util"
+	"knative.dev/client/pkg/wait"
 )
 
 func fakeServiceDelete(args []string) (action client_testing.Action, name string, output string, err error) {
@@ -35,6 +40,17 @@ func fakeServiceDelete(args []string) (action client_testing.Action, name string
 			name = deleteAction.GetName()
 			return true, nil, nil
 		})
+	fakeServing.AddWatchReactor("services",
+		func(a client_testing.Action) (bool, watch.Interface, error) {
+			watchAction := a.(clienttesting.WatchAction)
+			_, found := watchAction.GetWatchRestrictions().Fields.RequiresExactMatch("metadata.name")
+			if !found {
+				return true, nil, errors.New("no field selector on metadata.name found")
+			}
+			w := wait.NewFakeWatch(getServiceDeleteEvents("test-service"))
+			w.Start()
+			return true, w, nil
+		})
 	cmd.SetArgs(args)
 	err = cmd.Execute()
 	if err != nil {
@@ -79,3 +95,11 @@ func TestMultipleServiceDelete(t *testing.T) {
 	assert.Check(t, util.ContainsAll(output, "Service", sevName2, "deleted", "namespace", commands.FakeNamespace))
 	assert.Check(t, util.ContainsAll(output, "Service", sevName3, "deleted", "namespace", commands.FakeNamespace))
 }
+
+func getServiceDeleteEvents(name string) []watch.Event {
+	return []watch.Event{
+		{watch.Added, wait.CreateTestServiceWithConditions(name, corev1.ConditionUnknown, corev1.ConditionUnknown, "", "msg1")},
+		{watch.Modified, wait.CreateTestServiceWithConditions(name, corev1.ConditionUnknown, corev1.ConditionTrue, "", "msg2")},
+		{watch.Deleted, wait.CreateTestServiceWithConditions(name, corev1.ConditionTrue, corev1.ConditionTrue, "", "")},
+	}
+}

pkg/kn/commands/service/update.go

@@ -136,7 +136,7 @@ func NewServiceUpdateCommand(p *commands.KnParams) *cobra.Command {
 
 	commands.AddNamespaceFlags(serviceUpdateCommand.Flags(), false)
 	editFlags.AddUpdateFlags(serviceUpdateCommand)
-	waitFlags.AddConditionWaitFlags(serviceUpdateCommand, commands.WaitDefaultTimeout, "Update", "service")
+	waitFlags.AddConditionWaitFlags(serviceUpdateCommand, commands.WaitDefaultTimeout, "Update", "service", "ready")
 	trafficFlags.Add(serviceUpdateCommand)
 	return serviceUpdateCommand
 }

pkg/kn/commands/wait_flags.go

@@ -39,12 +39,12 @@ type WaitFlags struct {
 // Add flags which influence the sync/async behaviour when creating or updating
 // resources. Set `waitDefault` argument if the default behaviour is synchronous.
 // Use `what` for describing what is waited for.
-func (p *WaitFlags) AddConditionWaitFlags(command *cobra.Command, waitTimeoutDefault int, action string, what string) {
-	waitUsage := fmt.Sprintf("%s %s and don't wait for it to become ready.", action, what)
+func (p *WaitFlags) AddConditionWaitFlags(command *cobra.Command, waitTimeoutDefault int, action, what, until string) {
+	waitUsage := fmt.Sprintf("%s %s and don't wait for it to be %s.", action, what, until)
 	//TODO: deprecated flag should be removed in next release
 	command.Flags().BoolVar(&p.Async, "async", false, "DEPRECATED: please use --no-wait instead. "+waitUsage)
 	command.Flags().BoolVar(&p.NoWait, "no-wait", false, waitUsage)
 
-	timeoutUsage := fmt.Sprintf("Seconds to wait before giving up on waiting for %s to be ready.", what)
+	timeoutUsage := fmt.Sprintf("Seconds to wait before giving up on waiting for %s to be %s.", what, until)
 	command.Flags().IntVar(&p.TimeoutInSeconds, "wait-timeout", waitTimeoutDefault, timeoutUsage)
 }

pkg/kn/commands/wait_flags_test.go

@@ -42,7 +42,7 @@ func TestAddWaitForReadyDeprecatedFlags(t *testing.T) {
 
 		flags := &WaitFlags{}
 		cmd := cobra.Command{}
-		flags.AddConditionWaitFlags(&cmd, 60, "Create", "service")
+		flags.AddConditionWaitFlags(&cmd, 60, "Create", "service", "ready")
 
 		err := cmd.ParseFlags(tc.args)
 		if err != nil && !tc.isParseErrorExpected {
@@ -76,7 +76,7 @@ func TestAddWaitForReadyFlags(t *testing.T) {
 
 		flags := &WaitFlags{}
 		cmd := cobra.Command{}
-		flags.AddConditionWaitFlags(&cmd, 60, "Create", "service")
+		flags.AddConditionWaitFlags(&cmd, 60, "Create", "service", "ready")
 
 		err := cmd.ParseFlags(tc.args)
 		if err != nil && !tc.isParseErrorExpected {
@@ -101,7 +101,7 @@ func TestAddWaitUsageMessage(t *testing.T) {
 
 	flags := &WaitFlags{}
 	cmd := cobra.Command{}
-	flags.AddConditionWaitFlags(&cmd, 60, "bla", "blub")
+	flags.AddConditionWaitFlags(&cmd, 60, "bla", "blub", "deleted")
 	if !strings.Contains(cmd.UsageString(), "blub") {
 		t.Error("no type returned in usage")
 	}
@@ -111,4 +111,7 @@ func TestAddWaitUsageMessage(t *testing.T) {
 	if !strings.Contains(cmd.UsageString(), "60") {
 		t.Error("default timeout not contained")
 	}
+	if !strings.Contains(cmd.UsageString(), "deleted") {
+		t.Error("wrong until message")
+	}
 }

pkg/serving/v1/client.go

@@ -69,7 +69,7 @@ type KnServingClient interface {
 	UpdateServiceWithRetry(name string, updateFunc serviceUpdateFunc, nrRetries int) error
 
 	// Delete a service by name
-	DeleteService(name string) error
+	DeleteService(name string, timeout time.Duration) error
 
 	// Wait for a service to become ready, but not longer than provided timeout.
 	// Return error and how long has been waited
@@ -253,10 +253,29 @@ func updateServiceWithRetry(cl KnServingClient, name string, updateFunc serviceU
 }
 
 // Delete a service by name
-func (cl *knServingClient) DeleteService(serviceName string) error {
+// Param `timeout` represents a duration to wait for a delete op to finish.
+// For `timeout == 0` delete is performed async without any wait.
+func (cl *knServingClient) DeleteService(serviceName string, timeout time.Duration) error {
+	if timeout == 0 {
+		return cl.deleteService(serviceName, v1.DeletePropagationBackground)
+	}
+	waitC := make(chan error)
+	go func() {
+		waitForEvent := wait.NewWaitForEvent("service", cl.WatchService, func(evt *watch.Event) bool { return evt.Type == watch.Deleted })
+		err, _ := waitForEvent.Wait(serviceName, timeout, wait.NoopMessageCallback())
+		waitC <- err
+	}()
+	err := cl.deleteService(serviceName, v1.DeletePropagationForeground)
+	if err != nil {
+		return err
+	}
+	return <-waitC
+}
+
+func (cl *knServingClient) deleteService(serviceName string, propagationPolicy v1.DeletionPropagation) error {
 	err := cl.client.Services(cl.namespace).Delete(
 		serviceName,
-		&v1.DeleteOptions{},
+		&v1.DeleteOptions{PropagationPolicy: &propagationPolicy},
 	)
 	if err != nil {
 		return clienterrors.GetError(err)

pkg/serving/v1/client_mock.go

@@ -106,12 +106,12 @@ func (c *MockKnServingClient) UpdateServiceWithRetry(name string, updateFunc ser
 }
 
 // Delete a service by name
-func (sr *ServingRecorder) DeleteService(name interface{}, err error) {
-	sr.r.Add("DeleteService", []interface{}{name}, []interface{}{err})
+func (sr *ServingRecorder) DeleteService(name, timeout interface{}, err error) {
+	sr.r.Add("DeleteService", []interface{}{name, timeout}, []interface{}{err})
 }
 
-func (c *MockKnServingClient) DeleteService(name string) error {
-	call := c.recorder.r.VerifyCall("DeleteService", name)
+func (c *MockKnServingClient) DeleteService(name string, timeout time.Duration) error {
+	call := c.recorder.r.VerifyCall("DeleteService", name, timeout)
 	return mock.ErrorOrNil(call.Result[0])
 }
 

pkg/serving/v1/client_mock_test.go

@@ -36,7 +36,7 @@ func TestMockKnClient(t *testing.T) {
 	recorder.ListServices(mock.Any(), nil, nil)
 	recorder.CreateService(&servingv1.Service{}, nil)
 	recorder.UpdateService(&servingv1.Service{}, nil)
-	recorder.DeleteService("hello", nil)
+	recorder.DeleteService("hello", time.Duration(10)*time.Second, nil)
 	recorder.WaitForService("hello", time.Duration(10)*time.Second, wait.NoopMessageCallback(), nil, 10*time.Second)
 	recorder.GetRevision("hello", nil, nil)
 	recorder.ListRevisions(mock.Any(), nil, nil)
@@ -50,7 +50,7 @@ func TestMockKnClient(t *testing.T) {
 	client.ListServices(WithName("blub"))
 	client.CreateService(&servingv1.Service{})
 	client.UpdateService(&servingv1.Service{})
-	client.DeleteService("hello")
+	client.DeleteService("hello", time.Duration(10)*time.Second)
 	client.WaitForService("hello", time.Duration(10)*time.Second, wait.NoopMessageCallback())
 	client.GetRevision("hello")
 	client.ListRevisions(WithName("blub"))

pkg/serving/v1/client_test.go

@@ -179,19 +179,38 @@ func TestDeleteService(t *testing.T) {
 			}
 			return true, nil, errors.NewNotFound(servingv1.Resource("service"), name)
 		})
+	serving.AddWatchReactor("services",
+		func(a clienttesting.Action) (bool, watch.Interface, error) {
+			watchAction := a.(clienttesting.WatchAction)
+			name, found := watchAction.GetWatchRestrictions().Fields.RequiresExactMatch("metadata.name")
+			if !found {
+				return true, nil, errors.NewNotFound(servingv1.Resource("service"), name)
+			}
+			w := wait.NewFakeWatch(getServiceDeleteEvents("test-service"))
+			w.Start()
+			return true, w, nil
+		})
 
 	t.Run("delete existing service returns no error", func(t *testing.T) {
-		err := client.DeleteService(serviceName)
+		err := client.DeleteService(serviceName, time.Duration(10)*time.Second)
 		assert.NilError(t, err)
 	})
 
 	t.Run("trying to delete non-existing service returns error", func(t *testing.T) {
-		err := client.DeleteService(nonExistingServiceName)
+		err := client.DeleteService(nonExistingServiceName, time.Duration(10)*time.Second)
 		assert.ErrorContains(t, err, "not found")
 		assert.ErrorContains(t, err, nonExistingServiceName)
 	})
 }
 
+func getServiceDeleteEvents(name string) []watch.Event {
+	return []watch.Event{
+		{watch.Added, wait.CreateTestServiceWithConditions(name, corev1.ConditionUnknown, corev1.ConditionUnknown, "", "msg1")},
+		{watch.Modified, wait.CreateTestServiceWithConditions(name, corev1.ConditionUnknown, corev1.ConditionTrue, "", "msg2")},
+		{watch.Deleted, wait.CreateTestServiceWithConditions(name, corev1.ConditionTrue, corev1.ConditionTrue, "", "")},
+	}
+}
+
 func TestGetRevision(t *testing.T) {
 	serving, client := setup()