Make CLI functions return error instead of fatal out (#1379)

Before this PR, CLI functions fatal out when encountering error
instead of returning it. Such behavior makes it hard to reuse 
the functions nor writing unit tests. This PR aims to make functions 
return errors instead of error out.

pkg/fission-cli/cmd/package/create.go

@@ -23,16 +23,14 @@ import (
 	"time"
 
 	"github.com/dchest/uniuri"
+	"github.com/pkg/errors"
 	uuid "github.com/satori/go.uuid"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	fv1 "github.com/fission/fission/pkg/apis/fission.io/v1"
 	"github.com/fission/fission/pkg/controller/client"
 	"github.com/fission/fission/pkg/fission-cli/cliwrapper/cli"
-	"github.com/fission/fission/pkg/fission-cli/cmd"
-	cmdutils "github.com/fission/fission/pkg/fission-cli/cmd"
 	"github.com/fission/fission/pkg/fission-cli/cmd/spec"
-	"github.com/fission/fission/pkg/fission-cli/log"
 	"github.com/fission/fission/pkg/fission-cli/util"
 )
 
@@ -41,8 +39,12 @@ type CreateSubCommand struct {
 }
 
 func Create(flags cli.Input) error {
+	c, err := util.GetServer(flags)
+	if err != nil {
+		return err
+	}
 	opts := CreateSubCommand{
-		client: cmd.GetServer(flags),
+		client: c,
 	}
 	return opts.do(flags)
 }
@@ -60,7 +62,7 @@ func (opts *CreateSubCommand) complete(flags cli.Input) error {
 	pkgNamespace := flags.String("pkgNamespace")
 	envName := flags.String("env")
 	if len(envName) == 0 {
-		log.Fatal("Need --env argument.")
+		return errors.New("Need --env argument.")
 	}
 	envNamespace := flags.String("envNamespace")
 	srcArchiveFiles := flags.StringSlice("src")
@@ -69,7 +71,7 @@ func (opts *CreateSubCommand) complete(flags cli.Input) error {
 	keepURL := flags.Bool("keepurl")
 
 	if len(srcArchiveFiles) == 0 && len(deployArchiveFiles) == 0 {
-		log.Fatal("Need --src to specify source archive, or use --deploy to specify deployment archive.")
+		return errors.New("Need --src to specify source archive, or use --deploy to specify deployment archive.")
 	}
 
 	_, err := CreatePackage(flags, opts.client, pkgNamespace, envName, envNamespace,
@@ -132,19 +134,25 @@ func CreatePackage(flags cli.Input, client *client.Client, pkgNamespace string,
 
 	if len(specFile) > 0 {
 		// if a package sith the same spec exists, don't create a new spec file
-		fr, err := spec.ReadSpecs(cmdutils.GetSpecDir(flags))
-		util.CheckErr(err, "read specs")
+		fr, err := spec.ReadSpecs(util.GetSpecDir(flags))
+		if err != nil {
+			return nil, errors.Wrap(err, "error reading specs")
+		}
 		if m := fr.SpecExists(pkg, false, true); m != nil {
 			fmt.Printf("Re-using previously created package %v\n", m.Name)
 			return m, nil
 		}
 
 		err = spec.SpecSave(*pkg, specFile)
-		util.CheckErr(err, "save package spec")
+		if err != nil {
+			return nil, errors.Wrap(err, "error saving package spec")
+		}
 		return &pkg.Metadata, nil
 	} else {
 		pkgMetadata, err := client.PackageCreate(pkg)
-		util.CheckErr(err, "create package")
+		if err != nil {
+			return nil, errors.Wrap(err, "error creating package")
+		}
 		fmt.Printf("Package '%v' created\n", pkgMetadata.GetName())
 		return pkgMetadata, nil
 	}

pkg/fission-cli/cmd/package/delete.go

@@ -24,7 +24,7 @@ import (
 
 	"github.com/fission/fission/pkg/controller/client"
 	"github.com/fission/fission/pkg/fission-cli/cliwrapper/cli"
-	cmdutils "github.com/fission/fission/pkg/fission-cli/cmd"
+	"github.com/fission/fission/pkg/fission-cli/util"
 )
 
 type DeleteSubCommand struct {
@@ -36,8 +36,12 @@ type DeleteSubCommand struct {
 }
 
 func Delete(flags cli.Input) error {
+	c, err := util.GetServer(flags)
+	if err != nil {
+		return err
+	}
 	opts := DeleteSubCommand{
-		client: cmdutils.GetServer(flags),
+		client: c,
 	}
 	return opts.do(flags)
 }

pkg/fission-cli/cmd/package/get.go

@@ -27,8 +27,8 @@ import (
 	fv1 "github.com/fission/fission/pkg/apis/fission.io/v1"
 	"github.com/fission/fission/pkg/controller/client"
 	"github.com/fission/fission/pkg/fission-cli/cliwrapper/cli"
-	cmdutils "github.com/fission/fission/pkg/fission-cli/cmd"
 	pkgutil "github.com/fission/fission/pkg/fission-cli/cmd/package/util"
+	"github.com/fission/fission/pkg/fission-cli/util"
 )
 
 const (
@@ -45,16 +45,24 @@ type GetSubCommand struct {
 }
 
 func GetSrc(flags cli.Input) error {
+	c, err := util.GetServer(flags)
+	if err != nil {
+		return err
+	}
 	opts := GetSubCommand{
-		client:      cmdutils.GetServer(flags),
+		client:      c,
 		archiveType: sourceArchive,
 	}
 	return opts.do(flags)
 }
 
 func GetDeploy(flags cli.Input) error {
+	c, err := util.GetServer(flags)
+	if err != nil {
+		return err
+	}
 	opts := GetSubCommand{
-		client:      cmdutils.GetServer(flags),
+		client:      c,
 		archiveType: deployArchive,
 	}
 	return opts.do(flags)
@@ -96,7 +104,10 @@ func (opts *GetSubCommand) run(flags cli.Input) error {
 	if pkg.Spec.Deployment.Type == fv1.ArchiveTypeLiteral {
 		reader = bytes.NewReader(archive.Literal)
 	} else if pkg.Spec.Deployment.Type == fv1.ArchiveTypeUrl {
-		readCloser := pkgutil.DownloadStoragesvcURL(opts.client, archive.URL)
+		readCloser, err := pkgutil.DownloadStoragesvcURL(opts.client, archive.URL)
+		if err != nil {
+			return err
+		}
 		defer readCloser.Close()
 		reader = readCloser
 	}

pkg/fission-cli/cmd/package/info.go

@@ -21,12 +21,11 @@ import (
 	"os"
 	"text/tabwriter"
 
+	"github.com/pkg/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	"github.com/fission/fission/pkg/controller/client"
 	"github.com/fission/fission/pkg/fission-cli/cliwrapper/cli"
-	cmdutils "github.com/fission/fission/pkg/fission-cli/cmd"
-	"github.com/fission/fission/pkg/fission-cli/log"
 	"github.com/fission/fission/pkg/fission-cli/util"
 )
 
@@ -37,8 +36,12 @@ type InfoSubCommand struct {
 }
 
 func Info(flags cli.Input) error {
+	c, err := util.GetServer(flags)
+	if err != nil {
+		return err
+	}
 	opts := InfoSubCommand{
-		client: cmdutils.GetServer(flags),
+		client: c,
 	}
 	return opts.do(flags)
 }
@@ -54,7 +57,7 @@ func (opts *InfoSubCommand) do(flags cli.Input) error {
 func (opts *InfoSubCommand) complete(flags cli.Input) error {
 	opts.name = flags.String("name")
 	if len(opts.name) == 0 {
-		log.Fatal("Need name of package, use --name")
+		return errors.New("Need name of package, use --name")
 	}
 	opts.namespace = flags.String("pkgNamespace")
 	return nil
@@ -66,7 +69,7 @@ func (opts *InfoSubCommand) run(flags cli.Input) error {
 		Name:      opts.name,
 	})
 	if err != nil {
-		util.CheckErr(err, fmt.Sprintf("find package %s", opts.name))
+		return errors.Wrapf(err, "error finding package %s", opts.name)
 	}
 
 	w := tabwriter.NewWriter(os.Stdout, 0, 0, 1, ' ', 0)

pkg/fission-cli/cmd/package/list.go

@@ -27,7 +27,7 @@ import (
 
 	"github.com/fission/fission/pkg/controller/client"
 	"github.com/fission/fission/pkg/fission-cli/cliwrapper/cli"
-	cmdutils "github.com/fission/fission/pkg/fission-cli/cmd"
+	"github.com/fission/fission/pkg/fission-cli/util"
 )
 
 type ListSubCommand struct {
@@ -38,8 +38,12 @@ type ListSubCommand struct {
 }
 
 func List(flags cli.Input) error {
+	c, err := util.GetServer(flags)
+	if err != nil {
+		return err
+	}
 	opts := ListSubCommand{
-		client: cmdutils.GetServer(flags),
+		client: c,
 	}
 	return opts.do(flags)
 }

pkg/fission-cli/cmd/package/package.go

@@ -61,7 +61,7 @@ func CreateArchive(client *client.Client, includeFiles []string, noZip bool, kee
 		// Get files from inputs as number of files decide next steps
 		files, err := utils.FindAllGlobs([]string{path})
 		if err != nil {
-			util.CheckErr(err, "finding all globs")
+			return nil, errors.Wrap(err, "error finding all globs")
 		}
 
 		if len(files) == 0 {
@@ -90,14 +90,18 @@ func CreateArchive(client *client.Client, includeFiles []string, noZip bool, kee
 
 			// check if this AUS exists in the specs; if so, don't create a new one
 			fr, err := spec.ReadSpecs(specDir)
-			util.CheckErr(err, "read specs")
+			if err != nil {
+				return nil, errors.Wrap(err, "error reading specs")
+			}
 			if m := fr.SpecExists(aus, false, true); m != nil {
 				fmt.Printf("Re-using previously created archive %v\n", m.Name)
 				aus.Name = m.Name
 			} else {
 				// save the uploadspec
 				err := spec.SpecSave(*aus, specFile)
-				util.CheckErr(err, fmt.Sprintf("write spec file %v", specFile))
+				if err != nil {
+					return nil, errors.Wrapf(err, "write spec file %v", specFile)
+				}
 			}
 
 			// create the archive object
@@ -118,11 +122,18 @@ func CreateArchive(client *client.Client, includeFiles []string, noZip bool, kee
 			}, nil
 		}
 		// download the file before we archive it
-		dst := pkgutil.DownloadToTempFile(fileURL)
+		dst, err := pkgutil.DownloadToTempFile(fileURL)
+		if err != nil {
+			return nil, err
+		}
 		includeFiles = []string{dst}
 	}
 
-	archivePath := makeArchiveFile("", includeFiles, noZip)
+	archivePath, err := makeArchiveFile("", includeFiles, noZip)
+	if err != nil {
+		return nil, err
+	}
+
 	ctx := context.Background()
 	return pkgutil.UploadArchiveFile(ctx, client, archivePath)
 }
@@ -134,7 +145,7 @@ func CreateArchive(client *client.Client, includeFiles []string, noZip bool, kee
 // returned as-is with no zipping.  (This is used for compatibility
 // with v1 envs.)  noZip is IGNORED if there is more than one input
 // file.
-func makeArchiveFile(archiveNameHint string, archiveInput []string, noZip bool) string {
+func makeArchiveFile(archiveNameHint string, archiveInput []string, noZip bool) (string, error) {
 
 	// Unique name for the archive
 	archiveName := archiveName(archiveNameHint, archiveInput)
@@ -142,34 +153,34 @@ func makeArchiveFile(archiveNameHint string, archiveInput []string, noZip bool)
 	// Get files from inputs as number of files decide next steps
 	files, err := utils.FindAllGlobs(archiveInput)
 	if err != nil {
-		util.CheckErr(err, "finding all globs")
+		return "", errors.Wrap(err, "error finding all globs")
 	}
 
 	// We have one file; if it's a zip file, no need to archive it
 	if len(files) == 1 {
 		// make sure it exists
 		if _, err := os.Stat(files[0]); err != nil {
-			util.CheckErr(err, fmt.Sprintf("open input file %v", files[0]))
+			return "", errors.Wrapf(err, "open input file %v", files[0])
 		}
 
 		// if it's an existing zip file OR we're not supposed to zip it, don't do anything
 		if archiver.Zip.Match(files[0]) || noZip {
-			return files[0]
+			return files[0], nil
 		}
 	}
 
 	// For anything else, create a new archive
 	tmpDir, err := utils.GetTempDir()
 	if err != nil {
-		util.CheckErr(err, "create temporary archive directory")
+		return "", errors.Wrap(err, "error create temporary archive directory")
 	}
 
 	archivePath, err := utils.MakeZipArchive(filepath.Join(tmpDir, archiveName), archiveInput...)
 	if err != nil {
-		util.CheckErr(err, "create archive file")
+		return "", errors.Wrap(err, "create archive file")
 	}
 
-	return archivePath
+	return archivePath, nil
 }
 
 // Name an archive

pkg/fission-cli/cmd/package/rebuild.go

@@ -25,7 +25,7 @@ import (
 	fv1 "github.com/fission/fission/pkg/apis/fission.io/v1"
 	"github.com/fission/fission/pkg/controller/client"
 	"github.com/fission/fission/pkg/fission-cli/cliwrapper/cli"
-	cmdutils "github.com/fission/fission/pkg/fission-cli/cmd"
+	"github.com/fission/fission/pkg/fission-cli/util"
 )
 
 type RebuildSubCommand struct {
@@ -35,8 +35,12 @@ type RebuildSubCommand struct {
 }
 
 func Rebuild(flags cli.Input) error {
+	c, err := util.GetServer(flags)
+	if err != nil {
+		return err
+	}
 	opts := RebuildSubCommand{
-		client: cmdutils.GetServer(flags),
+		client: c,
 	}
 	return opts.do(flags)
 }

pkg/fission-cli/cmd/package/update.go

@@ -26,7 +26,7 @@ import (
 	fv1 "github.com/fission/fission/pkg/apis/fission.io/v1"
 	"github.com/fission/fission/pkg/controller/client"
 	"github.com/fission/fission/pkg/fission-cli/cliwrapper/cli"
-	"github.com/fission/fission/pkg/fission-cli/cmd"
+	"github.com/fission/fission/pkg/fission-cli/util"
 )
 
 type UpdateSubCommand struct {
@@ -43,8 +43,12 @@ type UpdateSubCommand struct {
 }
 
 func Update(flags cli.Input) error {
+	c, err := util.GetServer(flags)
+	if err != nil {
+		return err
+	}
 	opts := UpdateSubCommand{
-		client: cmd.GetServer(flags),
+		client: c,
 	}
 	return opts.do(flags)
 }

pkg/fission-cli/cmd/package/util/util.go

@@ -26,11 +26,11 @@ import (
 	"path/filepath"
 	"strings"
 
+	"github.com/pkg/errors"
 	uuid "github.com/satori/go.uuid"
 
 	fv1 "github.com/fission/fission/pkg/apis/fission.io/v1"
 	"github.com/fission/fission/pkg/controller/client"
-	"github.com/fission/fission/pkg/fission-cli/util"
 	storageSvcClient "github.com/fission/fission/pkg/storagesvc/client"
 	"github.com/fission/fission/pkg/types"
 	"github.com/fission/fission/pkg/utils"
@@ -46,18 +46,25 @@ func UploadArchiveFile(ctx context.Context, client *client.Client, fileName stri
 
 	if size < types.ArchiveLiteralSizeLimit {
 		archive.Type = fv1.ArchiveTypeLiteral
-		archive.Literal = GetContents(fileName)
+		archive.Literal, err = GetContents(fileName)
+		if err != nil {
+			return nil, err
+		}
 	} else {
 		u := strings.TrimSuffix(client.Url, "/") + "/proxy/storage"
 		ssClient := storageSvcClient.MakeClient(u)
 
 		// TODO add a progress bar
 		id, err := ssClient.Upload(ctx, fileName, nil)
-		util.CheckErr(err, fmt.Sprintf("upload file %v", fileName))
+		if err != nil {
+			return nil, errors.Wrapf(err, "error uploading file %v", fileName)
+		}
 
 		storageSvc, err := client.GetSvcURL("application=fission-storage")
 		storageSvcURL := "http://" + storageSvc
-		util.CheckErr(err, "get fission storage service name")
+		if err != nil {
+			return nil, errors.Wrapf(err, "error getting fission storage service name")
+		}
 
 		// We make a new client with actual URL of Storage service so that the URL is not
 		// pointing to 127.0.0.1 i.e. proxy. DON'T reuse previous ssClient
@@ -68,7 +75,9 @@ func UploadArchiveFile(ctx context.Context, client *client.Client, fileName stri
 		archive.URL = archiveURL
 
 		csum, err := utils.GetFileChecksum(fileName)
-		util.CheckErr(err, fmt.Sprintf("calculate checksum for file %v", fileName))
+		if err != nil {
+			return nil, errors.Wrapf(err, "calculate checksum for file %v", fileName)
+		}
 
 		archive.Checksum = *csum
 	}
@@ -76,32 +85,37 @@ func UploadArchiveFile(ctx context.Context, client *client.Client, fileName stri
 	return &archive, nil
 }
 
-func GetContents(filePath string) []byte {
-	var code []byte
-	var err error
-
-	code, err = ioutil.ReadFile(filePath)
-	util.CheckErr(err, fmt.Sprintf("read %v", filePath))
-	return code
+func GetContents(filePath string) ([]byte, error) {
+	code, err := ioutil.ReadFile(filePath)
+	if err != nil {
+		return nil, errors.Wrapf(err, "error reading %v", filePath)
+	}
+	return code, nil
 }
 
 // DownloadToTempFile fetches archive file from arbitrary url
 // and write it to temp file for further usage
-func DownloadToTempFile(fileUrl string) string {
+func DownloadToTempFile(fileUrl string) (string, error) {
 	reader, err := DownloadURL(fileUrl)
-	util.CheckErr(err, fmt.Sprintf("download from url: %v", fileUrl))
+	if err != nil {
+		return "", errors.Wrapf(err, "error downloading from url: %v", fileUrl)
+	}
 	defer reader.Close()
 
 	tmpDir, err := utils.GetTempDir()
-	util.CheckErr(err, "create temp directory")
+	if err != nil {
+		return "", errors.Wrapf(err, "error creating temp directory %v", tmpDir)
+	}
 
 	tmpFilename := uuid.NewV4().String()
 	destination := filepath.Join(tmpDir, tmpFilename)
 
 	err = WriteArchiveToFile(destination, reader)
-	util.CheckErr(err, "write archive to file")
+	if err != nil {
+		return "", errors.Wrapf(err, "error writing archive to file %v", destination)
+	}
 
-	return destination
+	return destination, nil
 }
 
 // DownloadURL downloads file from given url
@@ -146,16 +160,18 @@ func WriteArchiveToFile(fileName string, reader io.Reader) error {
 }
 
 // DownloadStoragesvcURL downloads and return archive content with given storage service url
-func DownloadStoragesvcURL(client *client.Client, fileUrl string) io.ReadCloser {
+func DownloadStoragesvcURL(client *client.Client, fileUrl string) (io.ReadCloser, error) {
 	u, err := url.ParseRequestURI(fileUrl)
 	if err != nil {
-		return nil
+		return nil, err
 	}
 
 	// replace in-cluster storage service host with controller server url
 	fileDownloadUrl := strings.TrimSuffix(client.Url, "/") + "/proxy/storage/" + u.RequestURI()
 	reader, err := DownloadURL(fileDownloadUrl)
+	if err != nil {
+		return nil, errors.Wrapf(err, fmt.Sprintf("error downloading from storage service url: %v", fileUrl))
+	}
 
-	util.CheckErr(err, fmt.Sprintf("download from storage service url: %v", fileUrl))
-	return reader
+	return reader, nil
 }