Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 4 commits into from
Apr 14, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b8417a3
Make wait, no-wait and async flags per bool var CLI convention
navidshaikh Apr 14, 2020
064a02f
Simplify conditionals
navidshaikh Apr 14, 2020
b79ddce
Add unit tests for deprecated flag async
navidshaikh Apr 14, 2020
4e845cd
Fix a typo
navidshaikh Apr 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions docs/cmd/kn_revision_delete.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
```

Expand Down
5 changes: 3 additions & 2 deletions docs/cmd/kn_service_create.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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).
Expand All @@ -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)
```

Expand Down
5 changes: 3 additions & 2 deletions docs/cmd/kn_service_delete.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
```

Expand Down
5 changes: 3 additions & 2 deletions docs/cmd/kn_service_update.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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).
Expand All @@ -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)
```

Expand Down
4 changes: 2 additions & 2 deletions pkg/kn/commands/revision/delete.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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
}
4 changes: 2 additions & 2 deletions pkg/kn/commands/service/create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down Expand Up @@ -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
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/kn/commands/service/delete.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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
}
6 changes: 6 additions & 0 deletions pkg/kn/commands/service/service_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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
}
4 changes: 2 additions & 2 deletions pkg/kn/commands/service/update.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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
}
Expand Down
23 changes: 11 additions & 12 deletions pkg/kn/commands/wait_flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
}
Expand All @@ -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)
}
52 changes: 32 additions & 20 deletions pkg/kn/commands/wait_flags_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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)
Expand All @@ -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 {
Expand All @@ -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)
Expand All @@ -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") {
Expand All @@ -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")
}
}
Loading