feat(wait): add wait for delete operation

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 become ready.
   -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 become ready.
+      --wait-timeout int   Seconds to wait before giving up on waiting for service to be ready. (default 600)
 ```
 
 ### Options inherited from parent commands

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")
 	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/serving/v1/client.go

@@ -69,12 +69,14 @@ 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
 	WaitForService(name string, timeout time.Duration, msgCallback wait.MessageCallback) (error, time.Duration)
 
+	// WaitForEvent(kind, name string, timeout time.Duration, done wait.EventDone) error
+
 	// Get a configuration by name
 	GetConfiguration(name string) (*servingv1.Configuration, error)
 
@@ -253,7 +255,24 @@ func updateServiceWithRetry(cl KnServingClient, name string, updateFunc serviceU
 }
 
 // Delete a service by name
-func (cl *knServingClient) DeleteService(serviceName string) error {
+func (cl *knServingClient) DeleteService(serviceName string, timeout time.Duration) error {
+	if timeout == 0 {
+		return cl.deleteService(serviceName)
+	}
+	waitC := make(chan error)
+	go func(name string, c chan error) {
+		waitForEvent := wait.NewWaitForEvent("service", cl.WatchService, func(evt *watch.Event) bool { return evt.Type == watch.Deleted })
+		err, _ := waitForEvent.Wait(name, timeout, wait.NoopMessageCallback())
+		c <- err
+	}(serviceName, waitC)
+	err := cl.deleteService(serviceName)
+	if err != nil {
+		return err
+	}
+	return <-waitC
+}
+
+func (cl *knServingClient) deleteService(serviceName string) error {
 	err := cl.client.Services(cl.namespace).Delete(
 		serviceName,
 		&v1.DeleteOptions{},

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()
 

pkg/wait/wait_for_ready.go

@@ -36,9 +36,19 @@ type waitForReadyConfig struct {
 	kind                string
 }
 
+// Callbacks and configuration used while waiting for event
+type waitForEvent struct {
+	watchMaker WatchMaker
+	eventDone  EventDone
+	kind       string
+}
+
+// Done marker to stop actual waiting on given event state
+type EventDone func(ev *watch.Event) bool
+
 // Interface used for waiting of a resource of a given name to reach a definitive
 // state in its "Ready" condition.
-type WaitForReady interface {
+type Wait interface {
 
 	// Wait on resource the resource with this name until a given timeout
 	// and write event messages for unknown event to the status writer.
@@ -56,14 +66,22 @@ type ConditionsExtractor func(obj runtime.Object) (apis.Conditions, error)
 type MessageCallback func(durationSinceState time.Duration, message string)
 
 // Constructor with resource type specific configuration
-func NewWaitForReady(kind string, watchMaker WatchMaker, extractor ConditionsExtractor) WaitForReady {
+func NewWaitForReady(kind string, watchMaker WatchMaker, extractor ConditionsExtractor) Wait {
 	return &waitForReadyConfig{
 		kind:                kind,
 		watchMaker:          watchMaker,
 		conditionsExtractor: extractor,
 	}
 }
 
+func NewWaitForEvent(kind string, watchMaker WatchMaker, eventDone EventDone) Wait {
+	return &waitForEvent{
+		kind:       kind,
+		watchMaker: watchMaker,
+		eventDone:  eventDone,
+	}
+}
+
 // A simple message callback which prints out messages line by line
 func SimpleMessageCallback(out io.Writer) MessageCallback {
 	oldMessage := ""
@@ -178,6 +196,30 @@ func (w *waitForReadyConfig) waitForReadyCondition(start time.Time, name string,
 	}
 }
 
+// Wait until the expected EventDone is satisfied
+func (w *waitForEvent) Wait(name string, timeout time.Duration, msgCallback MessageCallback) (error, time.Duration) {
+	watcher, err := w.watchMaker(name, timeout)
+	if err != nil {
+		return err, 0
+	}
+	defer watcher.Stop()
+	start := time.Now()
+	// channel used to transport the error
+	errChan := make(chan error)
+	for {
+		select {
+		case <-time.After(timeout):
+			return fmt.Errorf("timeout: %s '%s' not ready after %d seconds", w.kind, name, int(timeout/time.Second)), time.Since(start)
+		case err = <-errChan:
+			return err, time.Since(start)
+		case event := <-watcher.ResultChan():
+			if w.eventDone(&event) {
+				return nil, time.Since(start)
+			}
+		}
+	}
+}
+
 // Going over Unstructured to keep that function generally applicable.
 // Alternative implemenentation: Add a func-field to waitForReadyConfig which has to be
 // provided for every resource (like the conditions extractor)