Repository: knative/client (go)

Issue:
Avoid `os.Exit(...)` calls other than in top level command handling in `main.go`
### Bug report

This is not technically a bug but rather a "code smell". Came from [discussions](https://github.com/knative/client/pull/834#discussion_r427206020) in PR #834 with @rhuss 

### Expected behavior

No change except we should not be exiting using `os.Exit(...)` anywhere but top level calls ideally in `main.go`

### Steps to reproduce the problem

N/A

### kn version

```
➜  client git:(issue827) ./kn version
Version:      v20200521-local-9803cd6e
Build Date:   2020-05-21 22:17:42
Git Revision: 9803cd6e
Supported APIs:
* Serving
  - serving.knative.dev/v1 (knative-serving v0.14.0)
* Eventing
  - sources.knative.dev/v1alpha2 (knative-eventing v0.14.1)
  - eventing.knative.dev/v1alpha1 (knative-eventing v0.14.1)
```

### Knative (serving/eventing) version

> 0.14.x

> /kind good-first-issue
> /kind cleanup

Pull request description:
Remove Use of os.Exit() and Return Error Instead
## Description

This pull request removes the use of `os.Exit(1)` from areas of `kn` with the exception of `main.go` and `generate-docs.go`. Instead of using `os.Exit(1)` an error is returned now and handled in `main.go`.

Can also work of removing from `generate-docs.go`, but my thought was the issue was meant to address this in `kn`.

Can wait for #834 and rebase if that is preferred.

## Reference

Fixes #850 

/lint


Patch:
diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc
index d98cb9d0..9728988b 100644
--- a/CHANGELOG.adoc
+++ b/CHANGELOG.adoc
@@ -16,6 +16,10 @@
 |===
 | | Description | PR
 
+| 🐣
+| Replaced non-standard errors package with standard library functions
+| https://github.com/knative/client/pull/853[#853]
+
 | 🐛
 | Fix Panic for kn source ping describe with Sink URI
 | https://github.com/knative/client/pull/848[#848]
diff --git a/cmd/kn/main.go b/cmd/kn/main.go
index b9b502af..b7e88936 100644
--- a/cmd/kn/main.go
+++ b/cmd/kn/main.go
@@ -33,8 +33,13 @@ var err error
 func main() {
 	defer cleanup()
 	rand.Seed(time.Now().UnixNano())
-	err = core.NewDefaultKnCommand().Execute()
+	kn, err := core.NewDefaultKnCommand()
 	if err != nil {
+		fmt.Fprintln(os.Stderr, err)
+		os.Exit(1)
+	}
+
+	if err := kn.Execute(); err != nil {
 		if err.Error() != "subcommand is required" {
 			fmt.Fprintln(os.Stderr, err)
 		}
diff --git a/go.mod b/go.mod
index 8fc3cc98..244cd04c 100644
--- a/go.mod
+++ b/go.mod
@@ -7,7 +7,6 @@ require (
 	github.com/google/go-containerregistry v0.0.0-20200413145205-82d30a103c0a // indirect
 	github.com/mitchellh/go-homedir v1.1.0
 	github.com/openzipkin/zipkin-go v0.2.2 // indirect
-	github.com/pkg/errors v0.8.1
 	github.com/robfig/cron v1.2.0 // indirect
 	github.com/spf13/cobra v0.0.5
 	github.com/spf13/pflag v1.0.5
diff --git a/pkg/kn/commands/plugin/handler.go b/pkg/kn/commands/plugin/handler.go
index 5f8b86a0..918765d3 100644
--- a/pkg/kn/commands/plugin/handler.go
+++ b/pkg/kn/commands/plugin/handler.go
@@ -113,10 +113,10 @@ func (h *DefaultPluginHandler) Execute(executablePath string, cmdArgs, environme
 		cmd.Stdin = os.Stdin
 		cmd.Env = environment
 		err := cmd.Run()
-		if err == nil {
-			os.Exit(0)
+		if err != nil {
+			return err
 		}
-		return err
+		return nil
 	}
 	return syscall.Exec(executablePath, cmdArgs, environment)
 }
diff --git a/pkg/kn/commands/plugin/verifier.go b/pkg/kn/commands/plugin/verifier.go
index 8ed479f9..a2187814 100644
--- a/pkg/kn/commands/plugin/verifier.go
+++ b/pkg/kn/commands/plugin/verifier.go
@@ -21,6 +21,7 @@
 package plugin
 
 import (
+	"errors"
 	"fmt"
 	"io"
 	"os"
@@ -29,7 +30,6 @@ import (
 	"strings"
 	"syscall"
 
-	"github.com/pkg/errors"
 	"github.com/spf13/cobra"
 )
 
diff --git a/pkg/kn/commands/plugin/verifier_windows.go b/pkg/kn/commands/plugin/verifier_windows.go
index a2f75e53..76e98026 100644
--- a/pkg/kn/commands/plugin/verifier_windows.go
+++ b/pkg/kn/commands/plugin/verifier_windows.go
@@ -18,6 +18,7 @@
 package plugin
 
 import (
+	"errors"
 	"fmt"
 	"io"
 	"os"
@@ -25,7 +26,6 @@ import (
 	"runtime"
 	"strings"
 
-	"github.com/pkg/errors"
 	"github.com/spf13/cobra"
 )
 
diff --git a/pkg/kn/commands/service/configuration_edit_flags.go b/pkg/kn/commands/service/configuration_edit_flags.go
index 404eedc4..a2d101d6 100644
--- a/pkg/kn/commands/service/configuration_edit_flags.go
+++ b/pkg/kn/commands/service/configuration_edit_flags.go
@@ -15,10 +15,10 @@
 package service
 
 import (
+	"errors"
 	"fmt"
 	"strings"
 
-	"github.com/pkg/errors"
 	"github.com/spf13/cobra"
 
 	corev1 "k8s.io/api/core/v1"
@@ -263,7 +263,7 @@ func (p *ConfigurationEditFlags) Apply(
 	if cmd.Flags().Changed("env") {
 		envMap, err := util.MapFromArrayAllowingSingles(p.Env, "=")
 		if err != nil {
-			return errors.Wrap(err, "Invalid --env")
+			return fmt.Errorf("Invalid --env: %w", err)
 		}
 
 		envToRemove := util.ParseMinusSuffix(envMap)
@@ -295,12 +295,12 @@ func (p *ConfigurationEditFlags) Apply(
 	if cmd.Flags().Changed("mount") || cmd.Flags().Changed("volume") {
 		mountsToUpdate, mountsToRemove, err := util.OrderedMapAndRemovalListFromArray(p.Mount, "=")
 		if err != nil {
-			return errors.Wrap(err, "Invalid --mount")
+			return fmt.Errorf("Invalid --mount: %w", err)
 		}
 
 		volumesToUpdate, volumesToRemove, err := util.OrderedMapAndRemovalListFromArray(p.Volume, "=")
 		if err != nil {
-			return errors.Wrap(err, "Invalid --volume")
+			return fmt.Errorf("Invalid --volume: %w", err)
 		}
 
 		err = servinglib.UpdateVolumeMountsAndVolumes(template, mountsToUpdate, mountsToRemove, volumesToUpdate, volumesToRemove)
@@ -429,24 +429,24 @@ func (p *ConfigurationEditFlags) Apply(
 	if cmd.Flags().Changed("label") || cmd.Flags().Changed("label-service") || cmd.Flags().Changed("label-revision") {
 		labelsAllMap, err := util.MapFromArrayAllowingSingles(p.Labels, "=")
 		if err != nil {
-			return errors.Wrap(err, "Invalid --label")
+			return fmt.Errorf("Invalid --label: %w", err)
 		}
 
 		err = p.updateLabels(&service.ObjectMeta, p.LabelsService, labelsAllMap)
 		if err != nil {
-			return errors.Wrap(err, "Invalid --label-service")
+			return fmt.Errorf("Invalid --label-service: %w", err)
 		}
 
 		err = p.updateLabels(&template.ObjectMeta, p.LabelsRevision, labelsAllMap)
 		if err != nil {
-			return errors.Wrap(err, "Invalid --label-revision")
+			return fmt.Errorf("Invalid --label-revision: %w", err)
 		}
 	}
 
 	if cmd.Flags().Changed("annotation") {
 		annotationsMap, err := util.MapFromArrayAllowingSingles(p.Annotations, "=")
 		if err != nil {
-			return errors.Wrap(err, "Invalid --annotation")
+			return fmt.Errorf("Invalid --annotation: %w", err)
 		}
 
 		annotationsToRemove := util.ParseMinusSuffix(annotationsMap)
@@ -477,7 +477,7 @@ func (p *ConfigurationEditFlags) Apply(
 func (p *ConfigurationEditFlags) updateLabels(obj *metav1.ObjectMeta, flagLabels []string, labelsAllMap map[string]string) error {
 	labelFlagMap, err := util.MapFromArrayAllowingSingles(flagLabels, "=")
 	if err != nil {
-		return errors.Wrap(err, "Unable to parse label flags")
+		return fmt.Errorf("Unable to parse label flags: %w", err)
 	}
 	labelsMap := make(util.StringMap)
 	labelsMap.Merge(labelsAllMap)
@@ -495,7 +495,7 @@ func (p *ConfigurationEditFlags) computeResources(resourceFlags ResourceFlags) (
 		cpuQuantity, err := resource.ParseQuantity(resourceFlags.CPU)
 		if err != nil {
 			return corev1.ResourceList{},
-				errors.Wrapf(err, "Error parsing %q", resourceFlags.CPU)
+				fmt.Errorf("Error parsing %q: %w", resourceFlags.CPU, err)
 		}
 
 		resourceList[corev1.ResourceCPU] = cpuQuantity
@@ -505,7 +505,7 @@ func (p *ConfigurationEditFlags) computeResources(resourceFlags ResourceFlags) (
 		memoryQuantity, err := resource.ParseQuantity(resourceFlags.Memory)
 		if err != nil {
 			return corev1.ResourceList{},
-				errors.Wrapf(err, "Error parsing %q", resourceFlags.Memory)
+				fmt.Errorf("Error parsing %q: %w", resourceFlags.Memory, err)
 		}
 
 		resourceList[corev1.ResourceMemory] = memoryQuantity
diff --git a/pkg/kn/commands/trigger/update_flags.go b/pkg/kn/commands/trigger/update_flags.go
index 0825c487..31f48f60 100644
--- a/pkg/kn/commands/trigger/update_flags.go
+++ b/pkg/kn/commands/trigger/update_flags.go
@@ -15,7 +15,8 @@
 package trigger
 
 import (
-	"github.com/pkg/errors"
+	"fmt"
+
 	"github.com/spf13/cobra"
 
 	"knative.dev/client/pkg/util"
@@ -32,7 +33,7 @@ type TriggerUpdateFlags struct {
 func (f *TriggerUpdateFlags) GetFilters() (map[string]string, error) {
 	filters, err := util.MapFromArray(f.Filters, "=")
 	if err != nil {
-		return nil, errors.Wrap(err, "Invalid --filter")
+		return nil, fmt.Errorf("Invalid --filter: %w", err)
 	}
 	return filters, nil
 }
@@ -41,7 +42,7 @@ func (f *TriggerUpdateFlags) GetFilters() (map[string]string, error) {
 func (f *TriggerUpdateFlags) GetUpdateFilters() (map[string]string, []string, error) {
 	filters, err := util.MapFromArrayAllowingSingles(f.Filters, "=")
 	if err != nil {
-		return nil, nil, errors.Wrap(err, "Invalid --filter")
+		return nil, nil, fmt.Errorf("Invalid --filter: %w", err)
 	}
 	removes := util.ParseMinusSuffix(filters)
 	return filters, removes, nil
diff --git a/pkg/kn/core/root.go b/pkg/kn/core/root.go
index 1693fcef..f3a17d92 100644
--- a/pkg/kn/core/root.go
+++ b/pkg/kn/core/root.go
@@ -44,7 +44,7 @@ import (
 )
 
 // NewDefaultKnCommand creates the default `kn` command with a default plugin handler
-func NewDefaultKnCommand() *cobra.Command {
+func NewDefaultKnCommand() (*cobra.Command, error) {
 	rootCmd := NewKnCommand()
 
 	// Needed since otherwise --plugins-dir and --lookup-plugins
@@ -53,7 +53,7 @@ func NewDefaultKnCommand() *cobra.Command {
 	pluginsDir, lookupPluginsInPath, err := extractKnPluginFlags(os.Args)
 	if err != nil {
 		fmt.Fprintf(os.Stderr, "%v\n", err)
-		os.Exit(1)
+		return &cobra.Command{}, fmt.Errorf("%v", err)
 	}
 
 	pluginHandler := plugin.NewDefaultPluginHandler(plugin.ValidPluginFilenamePrefixes,
@@ -70,9 +70,9 @@ func NewDefaultKnCommandWithArgs(rootCmd *cobra.Command,
 	args []string,
 	in io.Reader,
 	out,
-	errOut io.Writer) *cobra.Command {
+	errOut io.Writer) (*cobra.Command, error) {
 	if pluginHandler == nil {
-		return rootCmd
+		return rootCmd, nil
 	}
 	if len(args) > 1 {
 		cmdPathPieces := args[1:]
@@ -84,18 +84,16 @@ func NewDefaultKnCommandWithArgs(rootCmd *cobra.Command,
 		if err != nil || plugin.InAllowedExtensibleCommandGroups(foundCmd.Name()) {
 			err := plugin.HandlePluginCommand(pluginHandler, cmdPathPieces)
 			if err != nil {
-				fmt.Fprintf(rootCmd.OutOrStderr(), "Error: unknown command '%s' \nRun 'kn --help' for usage.\n", args[1])
-				os.Exit(1)
+				return &cobra.Command{}, fmt.Errorf("unknown command '%s' \nRun 'kn --help' for usage", args[1])
 			}
 		} else if foundCmd.HasSubCommands() {
 			if _, _, err := rootCmd.Find(innerArgs); err != nil {
-				fmt.Fprintf(rootCmd.OutOrStderr(), showSubcommands(foundCmd, cmdPathPieces, innerArgs[0]))
-				os.Exit(1)
+				return &cobra.Command{}, fmt.Errorf(showSubcommands(foundCmd, cmdPathPieces, innerArgs[0]))
 			}
 		}
 	}
 
-	return rootCmd
+	return rootCmd, nil
 }
 
 // NewKnCommand creates the rootCmd which is the base command when called without any subcommands
@@ -232,8 +230,7 @@ func initConfig() {
 func defaultConfigDir() (string, error) {
 	home, err := homedir.Dir()
 	if err != nil {
-		fmt.Fprintf(os.Stderr, "%v\n", err)
-		os.Exit(1)
+		return "", fmt.Errorf("%v", err)
 	}
 	// Check the deprecated path first and fallback to it, add warning to error message
 	if configHome := filepath.Join(home, ".kn"); dirExists(configHome) {
diff --git a/pkg/serving/v1/client.go b/pkg/serving/v1/client.go
index ca2d0f59..e2f9765a 100644
--- a/pkg/serving/v1/client.go
+++ b/pkg/serving/v1/client.go
@@ -18,7 +18,6 @@ import (
 	"fmt"
 	"time"
 
-	"github.com/pkg/errors"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/fields"
 	"knative.dev/pkg/apis"
@@ -254,7 +253,7 @@ func updateServiceWithRetry(cl KnServingClient, name string, updateFunc serviceU
 				time.Sleep(time.Second)
 				continue
 			}
-			return errors.Wrap(err, fmt.Sprintf("giving up after %d retries", nrRetries))
+			return fmt.Errorf("giving up after %d retries: %w", nrRetries, err)
 		}
 		return nil
 	}


Test patch:
diff --git a/lib/test/cli.go b/lib/test/cli.go
index 93f37d55..89a97285 100644
--- a/lib/test/cli.go
+++ b/lib/test/cli.go
@@ -19,8 +19,6 @@ import (
 	"fmt"
 	"os/exec"
 	"strings"
-
-	"github.com/pkg/errors"
 )
 
 const (
@@ -107,7 +105,7 @@ func RunKubectl(namespace string, args ...string) (string, error) {
 	}
 	stdout, stderr, err := runCli("kubectl", args)
 	if err != nil {
-		return stdout, errors.Wrap(err, fmt.Sprintf("stderr: %s", stderr))
+		return stdout, fmt.Errorf("stderr: %s: %w", stderr, err)
 	}
 	return stdout, nil
 }
diff --git a/lib/test/integration.go b/lib/test/integration.go
index a062774a..421920de 100644
--- a/lib/test/integration.go
+++ b/lib/test/integration.go
@@ -23,8 +23,6 @@ import (
 	"sync"
 	"testing"
 	"time"
-
-	"github.com/pkg/errors"
 )
 
 const (
@@ -121,7 +119,7 @@ func CreateNamespace(namespace string) error {
 	expectedOutputRegexp := fmt.Sprintf("namespace?.+%s.+created", namespace)
 	out, err := createNamespaceWithRetry(namespace, MaxRetries)
 	if err != nil {
-		return errors.Wrap(err, "could not create namespace "+namespace)
+		return fmt.Errorf("could not create namespace %s: %w", namespace, err)
 	}
 
 	// check that last output indeed show created namespace
@@ -140,7 +138,7 @@ func DeleteNamespace(namespace string) error {
 	kubectl := Kubectl{namespace}
 	out, err := kubectl.Run("delete", "namespace", namespace)
 	if err != nil {
-		return errors.Wrap(err, fmt.Sprintf("Cannot delete namespace %s", namespace))
+		return fmt.Errorf("Cannot delete namespace %s: %w", namespace, err)
 	}
 
 	expectedOutputRegexp := fmt.Sprintf("namespace?.+%s.+deleted", namespace)
@@ -226,7 +224,7 @@ func createNamespaceWithRetry(namespace string, maxRetries int) (string, error)
 func matchRegexp(matchingRegexp, actual string) (bool, error) {
 	matched, err := regexp.MatchString(matchingRegexp, actual)
 	if err != nil {
-		return false, errors.Wrap(err, fmt.Sprintf("failed to match regexp '%s'", matchingRegexp))
+		return false, fmt.Errorf("failed to match regexp %q: %w", matchingRegexp, err)
 	}
 	return matched, nil
 }
diff --git a/pkg/kn/commands/service/delete_test.go b/pkg/kn/commands/service/delete_test.go
index 3dba0e0f..803debdf 100644
--- a/pkg/kn/commands/service/delete_test.go
+++ b/pkg/kn/commands/service/delete_test.go
@@ -15,9 +15,9 @@
 package service
 
 import (
+	"errors"
 	"testing"
 
-	"github.com/pkg/errors"
 	"gotest.tools/assert"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/runtime"
diff --git a/pkg/kn/core/root_test.go b/pkg/kn/core/root_test.go
index 48c3ead5..78d8b015 100644
--- a/pkg/kn/core/root_test.go
+++ b/pkg/kn/core/root_test.go
@@ -30,7 +30,7 @@ func TestNewDefaultKnCommand(t *testing.T) {
 	var rootCmd *cobra.Command
 
 	setup := func(t *testing.T) {
-		rootCmd = NewDefaultKnCommand()
+		rootCmd, _ = NewDefaultKnCommand()
 	}
 
 	t.Run("returns a valid root command", func(t *testing.T) {
@@ -48,7 +48,7 @@ func TestNewDefaultKnCommandWithArgs(t *testing.T) {
 	)
 
 	setup := func(t *testing.T) {
-		rootCmd = NewDefaultKnCommandWithArgs(NewKnCommand(), pluginHandler, args, os.Stdin, os.Stdout, os.Stderr)
+		rootCmd, _ = NewDefaultKnCommandWithArgs(NewKnCommand(), pluginHandler, args, os.Stdin, os.Stdout, os.Stderr)
 	}
 
 	t.Run("when pluginHandler is nil", func(t *testing.T) {
diff --git a/test/e2e/basic_workflow_test.go b/test/e2e/basic_workflow_test.go
index 3bf7a5c3..479c7f9b 100644
--- a/test/e2e/basic_workflow_test.go
+++ b/test/e2e/basic_workflow_test.go
@@ -82,7 +82,7 @@ func TestWrongCommand(t *testing.T) {
 	r.AssertError(out)
 
 	out = test.Kn{}.Run("rev")
-	assert.Check(t, util.ContainsAll(out.Stderr, "Error", "unknown command", "rev"))
+	assert.Check(t, util.ContainsAll(out.Stderr, "unknown command", "rev"))
 	r.AssertError(out)
 
 }
diff --git a/test/e2e/source_apiserver_test.go b/test/e2e/source_apiserver_test.go
index 95035184..21d13795 100644
--- a/test/e2e/source_apiserver_test.go
+++ b/test/e2e/source_apiserver_test.go
@@ -22,7 +22,6 @@ import (
 	"strings"
 	"testing"
 
-	"github.com/pkg/errors"
 	"gotest.tools/assert"
 
 	"knative.dev/client/lib/test"
@@ -130,19 +129,19 @@ func tearDownForSourceAPIServer(t *testing.T, it *test.KnTest) error {
 	saCmd := []string{"delete", "serviceaccount", testServiceAccount}
 	_, err := test.NewKubectl(it.Kn().Namespace()).Run(saCmd...)
 	if err != nil {
-		return errors.Wrap(err, fmt.Sprintf("Error executing '%s'", strings.Join(saCmd, " ")))
+		return fmt.Errorf("Error executing %q: %w", strings.Join(saCmd, " "), err)
 	}
 
 	crCmd := []string{"delete", "clusterrole", clusterRolePrefix + it.Kn().Namespace()}
 	_, err = test.Kubectl{}.Run(crCmd...)
 	if err != nil {
-		return errors.Wrap(err, fmt.Sprintf("Error executing '%s'", strings.Join(saCmd, " ")))
+		return fmt.Errorf("Error executing %q: %w", strings.Join(saCmd, " "), err)
 	}
 
 	crbCmd := []string{"delete", "clusterrolebinding", clusterRoleBindingPrefix + it.Kn().Namespace()}
 	_, err = test.Kubectl{}.Run(crbCmd...)
 	if err != nil {
-		return errors.Wrap(err, fmt.Sprintf("Error executing '%s'", strings.Join(saCmd, " ")))
+		return fmt.Errorf("Error executing %q: %w", strings.Join(saCmd, " "), err)
 	}
 	return nil
 }