Make wait, no-wait and async flags per bool var CLI convention (#802)

* Make wait, no-wait and async flags per bool var CLI convention

 Fixes #800

 - Deprecated bool vars can be supported for CLI convention
 - Bind --async flag value to --no-wait
 - Only one flag among [wait, no-wait, async] can be provided, else raise an error

* Simplify conditionals

* Add unit tests for deprecated flag async

* Fix a typo

docs/cmd/kn_revision_delete.md

@@ -21,10 +21,11 @@ kn revision delete NAME [flags]
 ### Options
 
 ```
-      --async              DEPRECATED: please use --no-wait instead. Delete revision and don't wait for it to be deleted.
+      --async              DEPRECATED: please use --no-wait instead. Do not wait for 'revision delete' operation to be completed. (default true)
   -h, --help               help for delete
   -n, --namespace string   Specify the namespace to operate in.
-      --no-wait            Delete revision and don't wait for it to be deleted. (default true)
+      --no-wait            Do not wait for 'revision delete' operation to be completed. (default true)
+      --wait               Wait for 'revision delete' operation to be completed.
       --wait-timeout int   Seconds to wait before giving up on waiting for revision to be deleted. (default 600)
 ```
 

docs/cmd/kn_service_create.md

@@ -47,7 +47,7 @@ kn service create NAME --image IMAGE [flags]
 ```
   -a, --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 be ready.
+      --async                         DEPRECATED: please use --no-wait instead. Do not wait for 'service create' operation to be completed.
       --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)
       --cluster-local                 Specify that the service be private. (--no-cluster-local will make the service publicly available)
       --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.
@@ -71,7 +71,7 @@ kn service create NAME --image IMAGE [flags]
   -n, --namespace string              Specify the namespace to operate in.
       --no-cluster-local              Do not specify that the service be private. (--no-cluster-local will make the service publicly available) (default true)
       --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 be ready.
+      --no-wait                       Do not wait for 'service create' operation to be completed.
   -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).
@@ -80,6 +80,7 @@ kn service create NAME --image IMAGE [flags]
       --service-account string        Service account name to set. An empty argument ("") clears the service account. The referenced service account must exist in the service's namespace.
       --user int                      The user ID to run the container (e.g., 1001).
       --volume stringArray            Add a volume from a ConfigMap (prefix cm: or config-map:) or a Secret (prefix secret: or sc:). Example: --volume myvolume=cm:myconfigmap or --volume myvolume=secret:mysecret. You can use this flag multiple times. To unset a ConfigMap/Secret reference, append "-" to the name, e.g. --volume myvolume-.
+      --wait                          Wait for 'service create' operation to be completed. (default true)
       --wait-timeout int              Seconds to wait before giving up on waiting for service to be ready. (default 600)
 ```
 

docs/cmd/kn_service_delete.md

@@ -24,10 +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.
+      --async              DEPRECATED: please use --no-wait instead. Do not wait for 'service delete' operation to be completed. (default true)
   -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. (default true)
+      --no-wait            Do not wait for 'service delete' operation to be completed. (default true)
+      --wait               Wait for 'service delete' operation to be completed.
       --wait-timeout int   Seconds to wait before giving up on waiting for service to be deleted. (default 600)
 ```
 

docs/cmd/kn_service_update.md

@@ -40,7 +40,7 @@ kn service update NAME [flags]
 ```
   -a, --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 be ready.
+      --async                         DEPRECATED: please use --no-wait instead. Do not wait for 'service update' operation to be completed.
       --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)
       --cluster-local                 Specify that the service be private. (--no-cluster-local will make the service publicly available)
       --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.
@@ -63,7 +63,7 @@ kn service update NAME [flags]
   -n, --namespace string              Specify the namespace to operate in.
       --no-cluster-local              Do not specify that the service be private. (--no-cluster-local will make the service publicly available) (default true)
       --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 be ready.
+      --no-wait                       Do not wait for 'service update' operation to be completed.
   -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).
@@ -75,6 +75,7 @@ kn service update NAME [flags]
       --untag strings                 Untag revision (format: --untag tagName). This flag can be specified multiple times.
       --user int                      The user ID to run the container (e.g., 1001).
       --volume stringArray            Add a volume from a ConfigMap (prefix cm: or config-map:) or a Secret (prefix secret: or sc:). Example: --volume myvolume=cm:myconfigmap or --volume myvolume=secret:mysecret. You can use this flag multiple times. To unset a ConfigMap/Secret reference, append "-" to the name, e.g. --volume myvolume-.
+      --wait                          Wait for 'service update' operation to be completed. (default true)
       --wait-timeout int              Seconds to wait before giving up on waiting for service to be ready. (default 600)
 ```
 

pkg/kn/commands/revision/delete.go

@@ -49,7 +49,7 @@ func NewRevisionDeleteCommand(p *commands.KnParams) *cobra.Command {
 
 			for _, name := range args {
 				timeout := time.Duration(0)
-				if !waitFlags.NoWait {
+				if waitFlags.Wait {
 					timeout = time.Duration(waitFlags.TimeoutInSeconds) * time.Second
 				}
 				err = client.DeleteRevision(name, timeout)
@@ -63,6 +63,6 @@ func NewRevisionDeleteCommand(p *commands.KnParams) *cobra.Command {
 		},
 	}
 	commands.AddNamespaceFlags(RevisionDeleteCommand.Flags(), false)
-	waitFlags.AddConditionWaitFlags(RevisionDeleteCommand, commands.WaitDefaultTimeout, "Delete", "revision", "deleted")
+	waitFlags.AddConditionWaitFlags(RevisionDeleteCommand, commands.WaitDefaultTimeout, "delete", "revision", "deleted")
 	return RevisionDeleteCommand
 }

pkg/kn/commands/service/create.go

@@ -117,7 +117,7 @@ func NewServiceCreateCommand(p *commands.KnParams) *cobra.Command {
 	}
 	commands.AddNamespaceFlags(serviceCreateCommand.Flags(), false)
 	editFlags.AddCreateFlags(serviceCreateCommand)
-	waitFlags.AddConditionWaitFlags(serviceCreateCommand, commands.WaitDefaultTimeout, "Create", "service", "ready")
+	waitFlags.AddConditionWaitFlags(serviceCreateCommand, commands.WaitDefaultTimeout, "create", "service", "ready")
 	return serviceCreateCommand
 }
 
@@ -145,7 +145,7 @@ func waitIfRequested(client clientservingv1.KnServingClient, service *servingv1.
 		fmt.Fprintf(out, "Service '%s' %s in namespace '%s'.\n", service.Name, verbDone, client.Namespace())
 		return nil
 	}
-	if waitFlags.NoWait {
+	if !waitFlags.Wait {
 		fmt.Fprintf(out, "Service '%s' %s in namespace '%s'.\n", service.Name, verbDone, client.Namespace())
 		return nil
 	}

pkg/kn/commands/service/delete.go

@@ -53,7 +53,7 @@ func NewServiceDeleteCommand(p *commands.KnParams) *cobra.Command {
 			}
 			for _, name := range args {
 				timeout := time.Duration(0)
-				if !waitFlags.NoWait {
+				if waitFlags.Wait {
 					timeout = time.Duration(waitFlags.TimeoutInSeconds) * time.Second
 				}
 				err = client.DeleteService(name, timeout)
@@ -67,6 +67,6 @@ func NewServiceDeleteCommand(p *commands.KnParams) *cobra.Command {
 		},
 	}
 	commands.AddNamespaceFlags(serviceDeleteCommand.Flags(), false)
-	waitFlags.AddConditionWaitFlags(serviceDeleteCommand, commands.WaitDefaultTimeout, "Delete", "service", "deleted")
+	waitFlags.AddConditionWaitFlags(serviceDeleteCommand, commands.WaitDefaultTimeout, "delete", "service", "deleted")
 	return serviceDeleteCommand
 }

pkg/kn/commands/service/service_test.go

@@ -17,9 +17,11 @@ package service
 import (
 	"bytes"
 
+	"github.com/spf13/cobra"
 	"k8s.io/client-go/tools/clientcmd"
 
 	"knative.dev/client/pkg/kn/commands"
+	knflags "knative.dev/client/pkg/kn/flags"
 	clientservingv1 "knative.dev/client/pkg/serving/v1"
 )
 
@@ -60,6 +62,10 @@ func executeServiceCommand(client clientservingv1.KnServingClient, args ...strin
 	cmd := NewServiceCommand(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
 }

pkg/kn/commands/service/update.go

@@ -110,7 +110,7 @@ func NewServiceUpdateCommand(p *commands.KnParams) *cobra.Command {
 
 			out := cmd.OutOrStdout()
 			//TODO: deprecated condition should be once --async is gone
-			if !waitFlags.Async && !waitFlags.NoWait {
+			if !waitFlags.Async && waitFlags.Wait {
 				fmt.Fprintf(out, "Updating Service '%s' in namespace '%s':\n", args[0], namespace)
 				fmt.Fprintln(out, "")
 				err := waitForService(client, name, out, waitFlags.TimeoutInSeconds)
@@ -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", "ready")
+	waitFlags.AddConditionWaitFlags(serviceUpdateCommand, commands.WaitDefaultTimeout, "update", "service", "ready")
 	trafficFlags.Add(serviceUpdateCommand)
 	return serviceUpdateCommand
 }

pkg/kn/commands/wait_flags.go

@@ -18,6 +18,8 @@ import (
 	"fmt"
 
 	"github.com/spf13/cobra"
+
+	knflags "knative.dev/client/pkg/kn/flags"
 )
 
 // Default time out to use when waiting for reconciliation. It is deliberately very long as it is expected that
@@ -29,9 +31,8 @@ const WaitDefaultTimeout = 600
 type WaitFlags struct {
 	// Timeout in seconds for how long to wait for a command to return
 	TimeoutInSeconds int
-
-	// If set then just apply resources and don't wait
-	NoWait bool
+	// If set then apply resources and wait for completion
+	Wait bool
 	//TODO: deprecated variable should be removed with --async flag
 	Async bool
 }
@@ -40,18 +41,16 @@ type WaitFlags struct {
 // 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, 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
-
+	waitUsage := fmt.Sprintf("Wait for '%s %s' operation to be completed.", what, action)
+	waitDefault := true
 	// Special-case 'delete' command so it comes back to the user immediately
-	noWaitDefault := false
-	if action == "Delete" {
-		noWaitDefault = true
+	if action == "delete" {
+		waitDefault = false
 	}
 
-	command.Flags().BoolVar(&p.Async, "async", false, "DEPRECATED: please use --no-wait instead. "+waitUsage)
-	command.Flags().BoolVar(&p.NoWait, "no-wait", noWaitDefault, waitUsage)
-
+	//TODO: deprecated flag should be removed in next release
+	command.Flags().BoolVar(&p.Async, "async", !waitDefault, "DEPRECATED: please use --no-wait instead. "+knflags.InvertUsage(waitUsage))
+	knflags.AddBothBoolFlagsUnhidden(command.Flags(), &p.Wait, "wait", "", waitDefault, waitUsage)
 	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

@@ -15,34 +15,37 @@
 package commands
 
 import (
+	"fmt"
 	"strings"
 	"testing"
 
+	knflags "knative.dev/client/pkg/kn/flags"
+
 	"github.com/spf13/cobra"
+	"gotest.tools/assert"
 )
 
 type waitTestCase struct {
 	args                 []string
 	timeoutExpected      int
-	isNoWaitExpected     bool
+	isWaitExpected       bool
 	isParseErrorExpected bool
 }
 
 //TODO: deprecated test should be removed with --async flag
 func TestAddWaitForReadyDeprecatedFlags(t *testing.T) {
-
 	for i, tc := range []waitTestCase{
-		{[]string{"--async"}, 60, true, false},
-		{[]string{}, 60, false, false},
-		{[]string{"--wait-timeout=120"}, 120, false, false},
+		{[]string{"--async"}, 60, false, false},
+		{[]string{}, 60, true, false},
+		{[]string{"--wait-timeout=120"}, 120, true, false},
 		// Can't be easily prevented, the timeout is just ignored in this case:
-		{[]string{"--async", "--wait-timeout=120"}, 120, true, false},
+		{[]string{"--async", "--wait-timeout=120"}, 120, false, false},
 		{[]string{"--wait-timeout=bla"}, 0, true, true},
 	} {
 
 		flags := &WaitFlags{}
 		cmd := cobra.Command{}
-		flags.AddConditionWaitFlags(&cmd, 60, "Create", "service", "ready")
+		flags.AddConditionWaitFlags(&cmd, 60, "create", "service", "ready")
 
 		err := cmd.ParseFlags(tc.args)
 		if err != nil && !tc.isParseErrorExpected {
@@ -54,8 +57,13 @@ func TestAddWaitForReadyDeprecatedFlags(t *testing.T) {
 		if tc.isParseErrorExpected {
 			continue
 		}
-		if flags.Async != tc.isNoWaitExpected {
-			t.Errorf("%d: wrong async mode detected: %t (expected) != %t (actual)", i, tc.isNoWaitExpected, flags.Async)
+
+		//  reconcile to ensure wait, no-wait and async behaves as expected
+		err = knflags.ReconcileBoolFlags(cmd.Flags())
+		assert.NilError(t, err)
+
+		if flags.Async == tc.isWaitExpected {
+			t.Errorf("%d: wrong async mode detected: %t (expected) != %t (actual)", i, tc.isWaitExpected, flags.Async)
 		}
 		if flags.TimeoutInSeconds != tc.timeoutExpected {
 			t.Errorf("%d: Invalid timeout set. %d (expected) != %d (actual)", i, tc.timeoutExpected, flags.TimeoutInSeconds)
@@ -64,19 +72,17 @@ func TestAddWaitForReadyDeprecatedFlags(t *testing.T) {
 }
 
 func TestAddWaitForReadyFlags(t *testing.T) {
-
 	for i, tc := range []waitTestCase{
-		{[]string{"--no-wait"}, 60, true, false},
-		{[]string{}, 60, false, false},
-		{[]string{"--wait-timeout=120"}, 120, false, false},
+		{[]string{}, 60, true, false},
+		{[]string{"--wait-timeout=120"}, 120, true, false},
 		// Can't be easily prevented, the timeout is just ignored in this case:
-		{[]string{"--no-wait", "--wait-timeout=120"}, 120, true, false},
+		{[]string{"--no-wait", "--wait-timeout=120"}, 120, false, false},
 		{[]string{"--wait-timeout=bla"}, 0, true, true},
 	} {
 
 		flags := &WaitFlags{}
 		cmd := cobra.Command{}
-		flags.AddConditionWaitFlags(&cmd, 60, "Create", "service", "ready")
+		flags.AddConditionWaitFlags(&cmd, 60, "create", "service", "ready")
 
 		err := cmd.ParseFlags(tc.args)
 		if err != nil && !tc.isParseErrorExpected {
@@ -88,8 +94,14 @@ func TestAddWaitForReadyFlags(t *testing.T) {
 		if tc.isParseErrorExpected {
 			continue
 		}
-		if flags.NoWait != tc.isNoWaitExpected {
-			t.Errorf("%d: wrong wait mode detected: %t (expected) != %t (actual)", i, tc.isNoWaitExpected, flags.NoWait)
+
+		//  reconcile to ensure wait, no-wait and async behaves as expected
+		err = knflags.ReconcileBoolFlags(cmd.Flags())
+		assert.NilError(t, err)
+		fmt.Println("wait value")
+		fmt.Println(flags.Wait)
+		if flags.Wait != tc.isWaitExpected {
+			t.Errorf("%d: wrong wait mode detected: %t (expected) != %t (actual)", i, tc.isWaitExpected, flags.Wait)
 		}
 		if flags.TimeoutInSeconds != tc.timeoutExpected {
 			t.Errorf("%d: Invalid timeout set. %d (expected) != %d (actual)", i, tc.timeoutExpected, flags.TimeoutInSeconds)
@@ -105,7 +117,7 @@ func TestAddWaitUsageMessage(t *testing.T) {
 	if !strings.Contains(cmd.UsageString(), "blub") {
 		t.Error("no type returned in usage")
 	}
-	if !strings.Contains(cmd.UsageString(), "don't wait") {
+	if !strings.Contains(cmd.UsageString(), "Do not wait") {
 		t.Error("wrong usage message")
 	}
 	if !strings.Contains(cmd.UsageString(), "60") {
@@ -119,8 +131,8 @@ func TestAddWaitUsageMessage(t *testing.T) {
 func TestAddWaitUsageDelete(t *testing.T) {
 	flags := &WaitFlags{}
 	cmd := cobra.Command{}
-	flags.AddConditionWaitFlags(&cmd, 60, "Delete", "blub", "deleted")
-	if !strings.Contains(cmd.UsageString(), "deleted. (default true)") {
+	flags.AddConditionWaitFlags(&cmd, 60, "delete", "blub", "deleted")
+	if !strings.Contains(cmd.UsageString(), "completed. (default true)") {
 		t.Error("Delete has wrong default value for --no-wait")
 	}
 }