Proper io.Closing around Packages (#843)

pkg/kudoctl/cmd/package_test.go

@@ -16,8 +16,8 @@ var packageCmdArgs = []struct {
 	errorMessage string
 }{
 	{"expect exactly one argument", []string{}, "expecting exactly one argument - directory of the operator to package"}, // 1
-	{"empty string argument", []string{""}, "invalid operator in path: "},                                                // 2
-	{"invalid operator", []string{"foo"}, "invalid operator in path: foo"},                                               // 3
+	{"empty string argument", []string{""}, "invalid operator in path:  error: path must be specified"},                  // 2
+	{"invalid operator", []string{"foo"}, "invalid operator in path: foo error: open foo: file does not exist"},          // 3
 	{"valid operator", []string{"/opt/zk"}, ""},                                                                          // 4
 }
 

pkg/kudoctl/packages/finder/package_finder.go

@@ -1,11 +1,12 @@
 package finder
 
 import (
+	"bytes"
 	"fmt"
-	"io"
 
 	"github.com/kudobuilder/kudo/pkg/kudoctl/http"
 	"github.com/kudobuilder/kudo/pkg/kudoctl/packages"
+
 	"github.com/spf13/afero"
 )
 
@@ -71,14 +72,14 @@ func (f *URLFinder) GetPackage(name string, version string) (packages.Package, e
 	if !http.IsValidURL(name) {
 		return nil, fmt.Errorf("finder: url %v invalid", name)
 	}
-	reader, err := f.getPackageByURL(name)
+	buf, err := f.getPackageByURL(name)
 	if err != nil {
 		return nil, err
 	}
-	return packages.NewPackageFromReader(reader), nil
+	return packages.NewFromBytes(buf), nil
 }
 
-func (f *URLFinder) getPackageByURL(url string) (io.Reader, error) {
+func (f *URLFinder) getPackageByURL(url string) (*bytes.Buffer, error) {
 	resp, err := f.client.Get(url)
 	if err != nil {
 		return nil, fmt.Errorf("finder: unable to get get reader from url %v", url)

pkg/kudoctl/packages/package.go

@@ -1,8 +1,10 @@
 package packages
 
 import (
+	"bytes"
 	"fmt"
 	"io"
+	"io/ioutil"
 	"regexp"
 	"strconv"
 	"strings"
@@ -263,17 +265,21 @@ func pathToOperator(fs afero.Fs, path string) (pfd *PackageFilesDigest, err erro
 	if err != nil {
 		return nil, err
 	}
+	b, err := ioutil.ReadAll(reader)
+	if err != nil {
+		return nil, err
+	}
 
-	pkg, err := readerToOperator(reader)
+	pkg, err := bufferToPackageFiles(bytes.NewBuffer(b))
 	pfd = &PackageFilesDigest{
 		pkg,
 		digest,
 	}
 	return pfd, err
 }
 
-func readerToOperator(r io.Reader) (*PackageFiles, error) {
-	b := NewPackageFromReader(r)
+func bufferToPackageFiles(buf *bytes.Buffer) (*PackageFiles, error) {
+	b := NewFromBytes(buf)
 	pkg, err := b.GetPkgFiles()
 	if err != nil {
 		return nil, err

pkg/kudoctl/packages/package_manager.go

@@ -2,13 +2,15 @@ package packages
 
 import (
 	"archive/tar"
+	"bytes"
 	"compress/gzip"
 	"fmt"
 	"io"
 	"io/ioutil"
 	"os"
 	"strings"
 
+	"github.com/kudobuilder/kudo/pkg/kudoctl/clog"
 	"github.com/kudobuilder/kudo/pkg/kudoctl/files"
 
 	"github.com/pkg/errors"
@@ -26,9 +28,8 @@ type Package interface {
 	GetPkgFiles() (*PackageFiles, error)
 }
 
-// FIXME: Implement 'io.Closer' and make sure that it is called. 'tarPackage' will leak a file descriptor otherwise.
 type tarPackage struct {
-	reader io.Reader
+	buf *bytes.Buffer
 }
 
 type filePackage struct {
@@ -44,69 +45,62 @@ func ReadPackage(fs afero.Fs, path string) (Package, error) {
 	if err != nil {
 		return nil, err
 	}
+	clog.V(4).Printf("determining package type of %v", path)
 	// order of discovery
 	// 1. tarball
 	// 2. file based
 	if fi.Mode().IsRegular() && strings.HasSuffix(path, ".tgz") {
-		r, err := getFileReader(fs, path)
+		b, err := afero.ReadFile(fs, path)
 		if err != nil {
 			return nil, err
 		}
-
-		return tarPackage{r}, nil
+		buf := bytes.NewBuffer(b)
+		clog.V(4).Printf("%v is a tar package", path)
+		return tarPackage{buf}, nil
 	} else if fi.IsDir() {
+		clog.V(4).Printf("%v is a file package", path)
 		return filePackage{path, fs}, nil
 	} else {
 		return nil, fmt.Errorf("unsupported file system format %v. Expect either a *.tgz file or a folder", path)
 	}
 }
 
-func getFileReader(fs afero.Fs, path string) (io.Reader, error) {
-	f, err := fs.Open(path)
-	if err != nil {
-		return nil, err
-	}
-	return f, nil
-}
-
-// NewPackageFromReader is a package from a reader.  This should only be used when a file cache isn't used.
-func NewPackageFromReader(r io.Reader) Package {
-	return tarPackage{r}
+// NewFromBytes creates a package from a byte Buffer
+func NewFromBytes(buf *bytes.Buffer) Package {
+	return tarPackage{buf}
 }
 
 // GetPkgFiles returns the command side package files
-func (b tarPackage) GetPkgFiles() (*PackageFiles, error) {
-	return parseTarPackage(b.reader)
+func (p tarPackage) GetPkgFiles() (*PackageFiles, error) {
+	return parseTarPackage(p.buf)
 }
 
 // GetCRDs returns the server side CRDs
-func (b tarPackage) GetCRDs() (*PackageCRDs, error) {
-	p, err := b.GetPkgFiles()
+func (p tarPackage) GetCRDs() (*PackageCRDs, error) {
+	pf, err := p.GetPkgFiles()
 	if err != nil {
 		return nil, errors.Wrap(err, "while extracting package files")
 	}
-	return p.getCRDs()
+	return pf.getCRDs()
 }
 
-func (b filePackage) GetCRDs() (*PackageCRDs, error) {
-	p, err := b.GetPkgFiles()
+func (p filePackage) GetCRDs() (*PackageCRDs, error) {
+	pf, err := p.GetPkgFiles()
 	if err != nil {
 		return nil, errors.Wrap(err, "while reading package from the file system")
 	}
-	return p.getCRDs()
+	return pf.getCRDs()
 }
 
-func (b filePackage) GetPkgFiles() (*PackageFiles, error) {
-	return fromFolder(b.fs, b.path)
+func (p filePackage) GetPkgFiles() (*PackageFiles, error) {
+	return fromFolder(p.fs, p.path)
 }
 
 // CreateTarball takes a path to operator files and creates a tgz of those files with the destination and name provided
 func CreateTarball(fs afero.Fs, path string, destination string, overwrite bool) (target string, err error) {
 	pkg, err := fromFolder(fs, path)
 	if err != nil {
-		//TODO (kensipe): use wrapped err at high verbosity
-		//return "", fmt.Errorf("invalid operator in path: %v error: %v", path, err)
-		return "", fmt.Errorf("invalid operator in path: %v", path)
+		return "", fmt.Errorf("invalid operator in path: %v error: %w", path, err)
 	}
 
 	name := packageVersionedName(pkg)
@@ -139,6 +133,9 @@ func packageVersionedName(pkg *PackageFiles) string {
 
 // fromFolder walks the path provided and returns CRD package files or an error
 func fromFolder(fs afero.Fs, packagePath string) (*PackageFiles, error) {
+	if packagePath == "" {
+		return nil, errors.New("path must be specified")
+	}
 	result := newPackageFiles()
 
 	err := afero.Walk(fs, packagePath, func(path string, file os.FileInfo, err error) error {
@@ -147,25 +144,29 @@ func fromFolder(fs afero.Fs, packagePath string) (*PackageFiles, error) {
 		}
 		if file.IsDir() {
 			// skip directories
+			clog.V(6).Printf("folder walking skipping directory %v", file)
 			return nil
 		}
 		if path == packagePath {
 			// skip the root folder, as Walk always starts there
 			return nil
 		}
-		bytes, err := afero.ReadFile(fs, path)
+		buf, err := afero.ReadFile(fs, path)
 		if err != nil {
 			return err
 		}
 
-		return parsePackageFile(path, bytes, &result)
+		return parsePackageFile(path, buf, &result)
 	})
 	if err != nil {
 		return nil, err
 	}
 	// final check
-	if result.Operator == nil || result.Params == nil {
-		return nil, fmt.Errorf("incomplete operator package in path: %v", packagePath)
+	if result.Operator == nil {
+		return nil, errors.New("operator package missing operator.yaml")
+	}
+	if result.Params == nil {
+		return nil, errors.New("operator package missing params.yaml")
 	}
 	return &result, nil
 }
@@ -209,14 +210,13 @@ func parseTarPackage(r io.Reader) (*PackageFiles, error) {
 		case tar.TypeDir:
 			// we don't need to handle folders, files have folder name in their names and that should be enough
 
-		// if it's a file create it
 		case tar.TypeReg:
-			bytes, err := ioutil.ReadAll(tr)
+			buf, err := ioutil.ReadAll(tr)
 			if err != nil {
 				return nil, errors.Wrapf(err, "while reading file from package tarball %s", header.Name)
 			}
 
-			err = parsePackageFile(header.Name, bytes, &result)
+			err = parsePackageFile(header.Name, buf, &result)
 			if err != nil {
 				return nil, err
 			}

pkg/kudoctl/util/repo/repo_operator.go

@@ -1,13 +1,12 @@
 package repo
 
 import (
+	"bytes"
 	"fmt"
-	"io"
 	"io/ioutil"
 	"net/url"
 	"strings"
 
-	"github.com/kudobuilder/kudo/pkg/apis/kudo/v1alpha1"
 	"github.com/kudobuilder/kudo/pkg/kudoctl/clog"
 	"github.com/kudobuilder/kudo/pkg/kudoctl/http"
 	"github.com/kudobuilder/kudo/pkg/kudoctl/kudohome"
@@ -82,11 +81,10 @@ func (r *Client) DownloadIndexFile() (*IndexFile, error) {
 // The PackageVersion is a package configuration from the index file which has a list of urls where
 // the package can be pulled from.  This will cycle through the list of urls and will return the reader
 // from the first successful url.  If all urls fail, the last error will be returned.
-func (r *Client) getPackageReaderByAPackageURL(pkg *PackageVersion) (io.Reader, error) {
-
+func (r *Client) getPackageReaderByAPackageURL(pkg *PackageVersion) (*bytes.Buffer, error) {
 	var pkgErr error
 	for _, u := range pkg.URLs {
-		r, err := r.getPackageReaderByURL(u)
+		r, err := r.getPackageBytesByURL(u)
 		if err == nil {
 			return r, nil
 		}
@@ -97,7 +95,7 @@ func (r *Client) getPackageReaderByAPackageURL(pkg *PackageVersion) (io.Reader,
 	return nil, pkgErr
 }
 
-func (r *Client) getPackageReaderByURL(packageURL string) (io.Reader, error) {
+func (r *Client) getPackageBytesByURL(packageURL string) (*bytes.Buffer, error) {
 	clog.V(4).Printf("attempt to retrieve package from url: %v", packageURL)
 	resp, err := r.Client.Get(packageURL)
 	if err != nil {
@@ -107,8 +105,8 @@ func (r *Client) getPackageReaderByURL(packageURL string) (io.Reader, error) {
 	return resp, nil
 }
 
-// GetPackageReader provides an io.Reader for a provided package name and optional version
-func (r *Client) GetPackageReader(name string, version string) (io.Reader, error) {
+// GetPackageBytes provides an io.Reader for a provided package name and optional version
+func (r *Client) GetPackageBytes(name string, version string) (*bytes.Buffer, error) {
 	clog.V(4).Printf("getting package reader for %v, %v", name, version)
 	clog.V(5).Printf("repository using: %v", r.Config)
 	// Construct the package name and download the index file from the remote repo
@@ -127,21 +125,9 @@ func (r *Client) GetPackageReader(name string, version string) (io.Reader, error
 
 // GetPackage provides an Package for a provided package name and optional version
 func (r *Client) GetPackage(name string, version string) (packages.Package, error) {
-	reader, err := r.GetPackageReader(name, version)
+	reader, err := r.GetPackageBytes(name, version)
 	if err != nil {
 		return nil, err
 	}
-	return packages.NewPackageFromReader(reader), nil
-}
-
-// GetOperatorVersionDependencies helper method returns a slice of strings that contains the names of all
-// dependency Operators
-func GetOperatorVersionDependencies(ov *v1alpha1.OperatorVersion) ([]string, error) {
-	var dependencyOperators []string
-	if ov.Spec.Dependencies != nil {
-		for _, v := range ov.Spec.Dependencies {
-			dependencyOperators = append(dependencyOperators, v.Name)
-		}
-	}
-	return dependencyOperators, nil
+	return packages.NewFromBytes(reader), nil
 }