From b8417a3f8888e92b9bdb51c2491571de7b6e5285 Mon Sep 17 00:00:00 2001 From: Navid Shaikh Date: Tue, 14 Apr 2020 23:34:30 +0530 Subject: [PATCH] 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 --- docs/cmd/kn_revision_delete.md | 5 ++- docs/cmd/kn_service_create.md | 5 ++- docs/cmd/kn_service_delete.md | 5 ++- docs/cmd/kn_service_update.md | 5 ++- pkg/kn/commands/revision/delete.go | 4 +- pkg/kn/commands/service/create.go | 4 +- pkg/kn/commands/service/delete.go | 2 +- pkg/kn/commands/service/service_test.go | 6 +++ pkg/kn/commands/service/update.go | 4 +- pkg/kn/commands/wait_flags.go | 23 ++++++----- pkg/kn/commands/wait_flags_test.go | 52 +++++++++++++++---------- pkg/kn/flags/bool.go | 35 +++++++++++++++-- 12 files changed, 100 insertions(+), 50 deletions(-) diff --git a/docs/cmd/kn_revision_delete.md b/docs/cmd/kn_revision_delete.md index 1337acb4c4..f12e129ef4 100644 --- a/docs/cmd/kn_revision_delete.md +++ b/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) ``` diff --git a/docs/cmd/kn_service_create.md b/docs/cmd/kn_service_create.md index ec8822d424..1ff4f6ac94 100644 --- a/docs/cmd/kn_service_create.md +++ b/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) ``` diff --git a/docs/cmd/kn_service_delete.md b/docs/cmd/kn_service_delete.md index 6a2aa9f720..b3f4f56cd1 100644 --- a/docs/cmd/kn_service_delete.md +++ b/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. -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. + --wait Wait for 'service Delete' operation to be completed. (default true) --wait-timeout int Seconds to wait before giving up on waiting for service to be deleted. (default 600) ``` diff --git a/docs/cmd/kn_service_update.md b/docs/cmd/kn_service_update.md index 255ae185c7..4a4da61b19 100644 --- a/docs/cmd/kn_service_update.md +++ b/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) ``` diff --git a/pkg/kn/commands/revision/delete.go b/pkg/kn/commands/revision/delete.go index 109448e8a5..e3c3ad7b1e 100644 --- a/pkg/kn/commands/revision/delete.go +++ b/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 } diff --git a/pkg/kn/commands/service/create.go b/pkg/kn/commands/service/create.go index 33900354df..ea1df02f85 100644 --- a/pkg/kn/commands/service/create.go +++ b/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 } diff --git a/pkg/kn/commands/service/delete.go b/pkg/kn/commands/service/delete.go index 413c7aea55..d63cebd2db 100644 --- a/pkg/kn/commands/service/delete.go +++ b/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) diff --git a/pkg/kn/commands/service/service_test.go b/pkg/kn/commands/service/service_test.go index 8d77089510..22561a6ec8 100644 --- a/pkg/kn/commands/service/service_test.go +++ b/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 } diff --git a/pkg/kn/commands/service/update.go b/pkg/kn/commands/service/update.go index fe82ce232d..1d72dc040f 100644 --- a/pkg/kn/commands/service/update.go +++ b/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 } diff --git a/pkg/kn/commands/wait_flags.go b/pkg/kn/commands/wait_flags.go index d92ee6f13f..32e0e07c00 100644 --- a/pkg/kn/commands/wait_flags.go +++ b/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) } diff --git a/pkg/kn/commands/wait_flags_test.go b/pkg/kn/commands/wait_flags_test.go index ce6ad0004f..923c0d882d 100644 --- a/pkg/kn/commands/wait_flags_test.go +++ b/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") } } diff --git a/pkg/kn/flags/bool.go b/pkg/kn/flags/bool.go index 026638b822..06049f31f5 100644 --- a/pkg/kn/flags/bool.go +++ b/pkg/kn/flags/bool.go @@ -24,7 +24,10 @@ import ( "github.com/spf13/pflag" ) -var negPrefix = "no-" +var ( + negPrefix = "no-" + deprecatedPrefix = "DEPRECATED:" +) // AddBothBoolFlagsUnhidden is just like AddBothBoolFlags but shows both flags. func AddBothBoolFlagsUnhidden(f *pflag.FlagSet, p *bool, name, short string, value bool, usage string) { @@ -32,7 +35,7 @@ func AddBothBoolFlagsUnhidden(f *pflag.FlagSet, p *bool, name, short string, val negativeName := negPrefix + name f.BoolVarP(p, name, short, value, usage) - f.Bool(negativeName, !value, "Do not "+firstCharToLower(usage)) + f.Bool(negativeName, !value, InvertUsage(usage)) } // AddBothBoolFlags adds the given flag in both `--foo` and `--no-foo` variants. @@ -65,6 +68,25 @@ func ReconcileBoolFlags(f *pflag.FlagSet) error { if err != nil { return } + + // handle DEPRECATD flags + if strings.HasPrefix(flag.Usage, deprecatedPrefix) { + if flag.Changed { + // handle async flag + if flag.Name == "async" { + if f.Lookup("wait").Changed || f.Lookup("no-wait").Changed { + err = fmt.Errorf("only one of (DEPRECATED) --%s, --wait and --no-wait may be specified", flag.Name) + return + } + err = checkExplicitFalse(flag, "wait") + if err != nil { + return + } + f.Lookup("no-wait").Value.Set("true") + } + } + } + // Walk the "no-" versions of the flags. Make sure we didn't set // both, and set the positive value to the opposite of the "no-" // value if it exists. @@ -90,6 +112,7 @@ func ReconcileBoolFlags(f *pflag.FlagSet) error { err = checkExplicitFalse(positive, flag.Name) } } + }) return err } @@ -110,7 +133,13 @@ func checkExplicitFalse(f *pflag.Flag, betterFlag string) error { return nil } -func firstCharToLower(s string) string { +// FirstCharToLower converts first char in given string to lowercase +func FirstCharToLower(s string) string { r, n := utf8.DecodeRuneInString(s) return string(unicode.ToLower(r)) + s[n:] } + +// InvertUsage inverts the usage string with prefix "Do not" +func InvertUsage(usage string) string { + return "Do not " + FirstCharToLower(usage) +}