feature(service): Wait on update for service to become ready. (#271)

* feature(service): Wait on update for service to become ready.

The behaviour is now the same as for `kn service create`. This should also fix some flakiness in the tests again.

* chore: Fixed changelog

* chore(service): Adapt help message for update

* fix(test): Updated renamed method

CHANGELOG.adoc

@@ -5,12 +5,29 @@
 [cols="1,10,3", options="header", width="100%"]
 |===
 | | Description | PR
+
 | 🎁🐛🧽🗑️
 |
 | https://github.com/knative/client/pull/[#]
 |===
 ////
 
+## v0.8.0 (unreleased)
+
+[cols="1,10,3", options="header", width="100%"]
+|===
+| | Description | PR
+
+| 🎁
+| Wait for service to become ready with `kn service update` (same behaviour as for `kn service create`)
+| https://github.com/knative/client/pull/271[#271]
+
+| 🐛
+| Better error handling when providing wrong kubeconfig option
+| https://github.com/knative/client/pull/222[#222]
+
+|===
+
 ## v0.2.0 (2019-07-10)
 
 [cols="1,10,3", options="header", width="100%"]
@@ -35,7 +52,7 @@
 
 | 🐛
 | Retry update operation on an optimistic lock failure
-| https://github.com/knative/client/pull/[#240]
+| https://github.com/knative/client/pull/240[#240]
 
 | 🎁
 | Add `kn route list`

docs/cmd/kn_service_update.md

@@ -27,6 +27,7 @@ kn service update NAME [flags]
 ### Options
 
 ```
+      --async                    Update service and don't wait for it to become ready.
       --concurrency-limit int    Hard Limit of concurrent requests to be processed by a single replica.
       --concurrency-target int   Recommendation for when to scale up based on the concurrent number of incoming request. Defaults to --concurrency-limit when given.
   -e, --env stringArray          Environment variable to set. NAME=value; you may provide this flag any number of times to set multiple environment variables.
@@ -40,6 +41,7 @@ kn service update NAME [flags]
   -p, --port int32               The port where application listens on.
       --requests-cpu string      The requested CPU (e.g., 250m).
       --requests-memory string   The requested memory (e.g., 64Mi).
+      --wait-timeout int         Seconds to wait before giving up on waiting for service to be ready. (default 60)
 ```
 
 ### Options inherited from parent commands

pkg/kn/commands/service/service.go

@@ -15,7 +15,14 @@
 package service
 
 import (
+	"io"
+	"time"
+
 	"github.com/knative/client/pkg/kn/commands"
+	serving_kn_v1alpha1 "github.com/knative/client/pkg/serving/v1alpha1"
+
+	"fmt"
+
 	"github.com/spf13/cobra"
 )
 
@@ -36,3 +43,16 @@ func NewServiceCommand(p *commands.KnParams) *cobra.Command {
 	serviceCmd.AddCommand(NewServiceUpdateCommand(p))
 	return serviceCmd
 }
+
+func waitForService(client serving_kn_v1alpha1.KnClient, serviceName string, out io.Writer, timeout int) error {
+	fmt.Fprintf(out, "Waiting for service '%s' to become ready ... ", serviceName)
+	flush(out)
+
+	err := client.WaitForService(serviceName, time.Duration(timeout)*time.Second)
+	if err != nil {
+		fmt.Fprintln(out)
+		return err
+	}
+	fmt.Fprintln(out, "OK")
+	return nil
+}

pkg/kn/commands/service/service_create.go

@@ -18,7 +18,6 @@ import (
 	"errors"
 	"fmt"
 	"io"
-	"time"
 
 	"github.com/knative/client/pkg/kn/commands"
 	"github.com/knative/client/pkg/serving/v1alpha1"
@@ -105,16 +104,11 @@ func NewServiceCreateCommand(p *commands.KnParams) *cobra.Command {
 
 			if !waitFlags.Async {
 				out := cmd.OutOrStdout()
-				fmt.Fprintf(out, "Waiting for service '%s' to become ready ... ", name)
+				err := waitForService(client, name, out, waitFlags.TimeoutInSeconds)
-				flush(out)
-
-				err := client.WaitForService(name, time.Duration(waitFlags.TimeoutInSeconds)*time.Second)
 				if err != nil {
-					fmt.Fprintln(out)
 					return err
 				}
-				fmt.Fprintln(out, "OK")
+				return showUrl(client, name, namespace, out)
-				return showUrl(client, name, namespace, cmd.OutOrStdout())
 			}
 
 			return nil
@@ -122,7 +116,7 @@ func NewServiceCreateCommand(p *commands.KnParams) *cobra.Command {
 	}
 	commands.AddNamespaceFlags(serviceCreateCommand.Flags(), false)
 	editFlags.AddCreateFlags(serviceCreateCommand)
-	waitFlags.AddConditionWaitFlags(serviceCreateCommand, 60, "service")
+	waitFlags.AddConditionWaitFlags(serviceCreateCommand, 60, "Create", "service")
 	return serviceCreateCommand
 }
 

pkg/kn/commands/service/service_update.go

@@ -26,6 +26,7 @@ import (
 
 func NewServiceUpdateCommand(p *commands.KnParams) *cobra.Command {
 	var editFlags ConfigurationEditFlags
+	var waitFlags commands.WaitFlags
 
 	serviceUpdateCommand := &cobra.Command{
 		Use:   "update NAME",
@@ -56,7 +57,8 @@ func NewServiceUpdateCommand(p *commands.KnParams) *cobra.Command {
 
 			var retries = 0
 			for {
-				service, err := client.GetService(args[0])
+				name := args[0]
+				service, err := client.GetService(name)
 				if err != nil {
 					return err
 				}
@@ -76,6 +78,15 @@ func NewServiceUpdateCommand(p *commands.KnParams) *cobra.Command {
 					}
 					return err
 				}
+
+				if !waitFlags.Async {
+					out := cmd.OutOrStdout()
+					err := waitForService(client, name, out, waitFlags.TimeoutInSeconds)
+					if err != nil {
+						return err
+					}
+				}
+
 				fmt.Fprintf(cmd.OutOrStdout(), "Service '%s' updated in namespace '%s'.\n", args[0], namespace)
 				return nil
 			}
@@ -84,5 +95,6 @@ func NewServiceUpdateCommand(p *commands.KnParams) *cobra.Command {
 
 	commands.AddNamespaceFlags(serviceUpdateCommand.Flags(), false)
 	editFlags.AddUpdateFlags(serviceUpdateCommand)
+	waitFlags.AddConditionWaitFlags(serviceUpdateCommand, 60, "Update", "service")
 	return serviceUpdateCommand
 }

pkg/kn/commands/service/service_update_test.go

@@ -21,17 +21,23 @@ import (
 	"strings"
 	"testing"
 
+	"gotest.tools/assert"
+
 	"github.com/knative/client/pkg/kn/commands"
 	servinglib "github.com/knative/client/pkg/serving"
+	"github.com/knative/client/pkg/util"
+	"github.com/knative/client/pkg/wait"
+
 	"github.com/knative/serving/pkg/apis/serving/v1alpha1"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/watch"
 	client_testing "k8s.io/client-go/testing"
 )
 
-func fakeServiceUpdate(original *v1alpha1.Service, args []string) (
+func fakeServiceUpdate(original *v1alpha1.Service, args []string, sync bool) (
 	action client_testing.Action,
 	updated *v1alpha1.Service,
 	output string,
@@ -55,6 +61,24 @@ func fakeServiceUpdate(original *v1alpha1.Service, args []string) (
 		func(a client_testing.Action) (bool, runtime.Object, error) {
 			return true, original, nil
 		})
+	if sync {
+		fakeServing.AddWatchReactor("services",
+			func(a client_testing.Action) (bool, watch.Interface, error) {
+				watchAction := a.(client_testing.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(getServiceEvents("test-service"))
+				w.Start()
+				return true, w, nil
+			})
+		fakeServing.AddReactor("get", "services",
+			func(a client_testing.Action) (bool, runtime.Object, error) {
+				return true, &v1alpha1.Service{}, nil
+			})
+	}
+
 	cmd.SetArgs(args)
 	err = cmd.Execute()
 	if err != nil {
@@ -64,6 +88,29 @@ func fakeServiceUpdate(original *v1alpha1.Service, args []string) (
 	return
 }
 
+func TestServiceUpdateImageSync(t *testing.T) {
+	orig := newEmptyService()
+
+	template, err := servinglib.RevisionTemplateOfService(orig)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	servinglib.UpdateImage(template, "gcr.io/foo/bar:baz")
+
+	action, updated, output, err := fakeServiceUpdate(orig, []string{
+		"service", "update", "foo", "--image", "gcr.io/foo/quux:xyzzy", "--namespace", "bar"}, true)
+
+	assert.NilError(t, err)
+	assert.Assert(t, action.Matches("update", "services"))
+
+	template, err = servinglib.RevisionTemplateOfService(updated)
+	assert.NilError(t, err)
+
+	assert.Equal(t, template.Spec.DeprecatedContainer.Image, "gcr.io/foo/quux:xyzzy")
+	assert.Assert(t, util.ContainsAll(strings.ToLower(output), "update", "foo", "service", "namespace", "bar", "ok", "waiting"))
+}
+
 func TestServiceUpdateImage(t *testing.T) {
 	orig := newEmptyService()
 
@@ -75,7 +122,7 @@ func TestServiceUpdateImage(t *testing.T) {
 	servinglib.UpdateImage(template, "gcr.io/foo/bar:baz")
 
 	action, updated, output, err := fakeServiceUpdate(orig, []string{
-		"service", "update", "foo", "--image", "gcr.io/foo/quux:xyzzy", "--namespace", "bar"})
+		"service", "update", "foo", "--image", "gcr.io/foo/quux:xyzzy", "--namespace", "bar", "--async"}, false)
 
 	if err != nil {
 		t.Fatal(err)
@@ -104,7 +151,7 @@ func TestServiceUpdateMaxMinScale(t *testing.T) {
 
 	action, updated, _, err := fakeServiceUpdate(original, []string{
 		"service", "update", "foo",
-		"--min-scale", "1", "--max-scale", "5", "--concurrency-target", "10", "--concurrency-limit", "100"})
+		"--min-scale", "1", "--max-scale", "5", "--concurrency-target", "10", "--concurrency-limit", "100", "--async"}, false)
 
 	if err != nil {
 		t.Fatal(err)
@@ -169,7 +216,7 @@ func TestServiceUpdateEnv(t *testing.T) {
 	servinglib.UpdateImage(template, "gcr.io/foo/bar:baz")
 
 	action, updated, _, err := fakeServiceUpdate(orig, []string{
-		"service", "update", "foo", "-e", "TARGET=Awesome"})
+		"service", "update", "foo", "-e", "TARGET=Awesome", "--async"}, false)
 
 	if err != nil {
 		t.Fatal(err)
@@ -195,7 +242,7 @@ func TestServiceUpdateRequestsLimitsCPU(t *testing.T) {
 	service := createMockServiceWithResources(t, "250", "64Mi", "1000m", "1024Mi")
 
 	action, updated, _, err := fakeServiceUpdate(service, []string{
-		"service", "update", "foo", "--requests-cpu", "500m", "--limits-cpu", "1000m"})
+		"service", "update", "foo", "--requests-cpu", "500m", "--limits-cpu", "1000m", "--async"}, false)
 	if err != nil {
 		t.Fatal(err)
 	} else if !action.Matches("update", "services") {
@@ -233,7 +280,7 @@ func TestServiceUpdateRequestsLimitsMemory(t *testing.T) {
 	service := createMockServiceWithResources(t, "100m", "64Mi", "1000m", "1024Mi")
 
 	action, updated, _, err := fakeServiceUpdate(service, []string{
-		"service", "update", "foo", "--requests-memory", "128Mi", "--limits-memory", "2048Mi"})
+		"service", "update", "foo", "--requests-memory", "128Mi", "--limits-memory", "2048Mi", "--async"}, false)
 	if err != nil {
 		t.Fatal(err)
 	} else if !action.Matches("update", "services") {
@@ -273,7 +320,7 @@ func TestServiceUpdateRequestsLimitsCPU_and_Memory(t *testing.T) {
 	action, updated, _, err := fakeServiceUpdate(service, []string{
 		"service", "update", "foo",
 		"--requests-cpu", "500m", "--limits-cpu", "2000m",
-		"--requests-memory", "128Mi", "--limits-memory", "2048Mi"})
+		"--requests-memory", "128Mi", "--limits-memory", "2048Mi", "--async"}, false)
 	if err != nil {
 		t.Fatal(err)
 	} else if !action.Matches("update", "services") {

pkg/kn/commands/service/wait_args.go

@@ -1,43 +0,0 @@
-// Copyright © 2019 The Knative Authors
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//     http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package service
-
-import (
-	"fmt"
-
-	"github.com/knative/client/pkg/wait"
-	"github.com/knative/pkg/apis"
-	serving_v1alpha1_api "github.com/knative/serving/pkg/apis/serving/v1alpha1"
-	serving_v1alpha1_client "github.com/knative/serving/pkg/client/clientset/versioned/typed/serving/v1alpha1"
-	"k8s.io/apimachinery/pkg/runtime"
-)
-
-// Create wait arguments for a Knative service which can be used to wait for
-// a create/update options to be finished
-// Can be used by `service_create` and `service_update`, hence this extra file
-func newServiceWaitForReady(client serving_v1alpha1_client.ServingV1alpha1Interface, namespace string) wait.WaitForReady {
-	return wait.NewWaitForReady(
-		"service",
-		client.Services(namespace).Watch,
-		serviceConditionExtractor)
-}
-
-func serviceConditionExtractor(obj runtime.Object) (apis.Conditions, error) {
-	service, ok := obj.(*serving_v1alpha1_api.Service)
-	if !ok {
-		return nil, fmt.Errorf("%v is not a service", obj)
-	}
-	return apis.Conditions(service.Status.Conditions), nil
-}

pkg/kn/commands/wait_flags.go

@@ -32,8 +32,8 @@ 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, what string) {
+func (p *WaitFlags) AddConditionWaitFlags(command *cobra.Command, waitTimeoutDefault int, action string, what string) {
-	waitUsage := fmt.Sprintf("Create %s and don't wait for it to become ready.", what)
+	waitUsage := fmt.Sprintf("%s %s and don't wait for it to become ready.", action, what)
 	command.Flags().BoolVar(&p.Async, "async", false, waitUsage)
 
 	timeoutUsage := fmt.Sprintf("Seconds to wait before giving up on waiting for %s to be ready.", what)

pkg/kn/commands/wait_flags_test.go

@@ -41,7 +41,7 @@ func TestAddWaitForReadyFlags(t *testing.T) {
 
 		flags := &WaitFlags{}
 		cmd := cobra.Command{}
-		flags.AddConditionWaitFlags(&cmd, 60, "service")
+		flags.AddConditionWaitFlags(&cmd, 60, "Create", "service")
 
 		err := cmd.ParseFlags(tc.args)
 		if err != nil && !tc.isParseErrorExpected {
@@ -66,7 +66,7 @@ func TestAddWaitUsageMessage(t *testing.T) {
 
 	flags := &WaitFlags{}
 	cmd := cobra.Command{}
-	flags.AddConditionWaitFlags(&cmd, 60, "blub")
+	flags.AddConditionWaitFlags(&cmd, 60, "bla", "blub")
 	if !strings.Contains(cmd.UsageString(), "blub") {
 		t.Error("no type returned in usage")
 	}