pkg/installation: cleanup refactoring

cmd/krew/cmd/install.go

@@ -31,8 +31,8 @@ import (
 
 func init() {
 	var (
-		manifest, forceDownloadFile *string
-		noUpdateIndex               *bool
+		manifest, archiveFileOverride *string
+		noUpdateIndex                 *bool
 	)
 
 	// installCmd represents the install command
@@ -79,10 +79,10 @@ Remarks:
 			}
 
 			if len(pluginNames) != 0 && *manifest != "" {
-				return errors.New("must specify either specify stdin or --manifest or args")
+				return errors.New("must specify either specify either plugin names (via positional arguments or STDIN), or --manifest; not both")
 			}
 
-			if *forceDownloadFile != "" && *manifest == "" {
+			if *archiveFileOverride != "" && *manifest == "" {
 				return errors.New("--archive can be specified only with --manifest")
 			}
 
@@ -106,24 +106,20 @@ Remarks:
 				install = append(install, plugin)
 			}
 
-			if len(install) > 1 && *manifest != "" {
-				return errors.New("can't use --manifest option with multiple plugins")
-			}
-
 			if len(install) == 0 {
 				return cmd.Help()
 			}
 
-			// Print plugin namesFromFile
 			for _, plugin := range install {
 				glog.V(2).Infof("Will install plugin: %s\n", plugin.Name)
 			}
 
 			var failed []string
-			// Do install
 			for _, plugin := range install {
 				fmt.Fprintf(os.Stderr, "Installing plugin: %s\n", plugin.Name)
-				err := installation.Install(paths, plugin, *forceDownloadFile)
+				err := installation.Install(paths, plugin, installation.InstallOpts{
+					ArchiveFileOverride: *archiveFileOverride,
+				})
 				if err == installation.ErrIsAlreadyInstalled {
 					glog.Warningf("Skipping plugin %q, it is already installed", plugin.Name)
 					continue
@@ -157,7 +153,7 @@ Remarks:
 	}
 
 	manifest = installCmd.Flags().String("manifest", "", "(Development-only) specify plugin manifest directly.")
-	forceDownloadFile = installCmd.Flags().String("archive", "", "(Development-only) force all downloads to use the specified file")
+	archiveFileOverride = installCmd.Flags().String("archive", "", "(Development-only) force all downloads to use the specified file")
 	noUpdateIndex = installCmd.Flags().Bool("no-update-index", false, "(Experimental) do not update local copy of plugin index before installing")
 
 	rootCmd.AddCommand(installCmd)

pkg/installation/install.go

@@ -30,39 +30,34 @@ import (
 	"sigs.k8s.io/krew/pkg/pathutil"
 )
 
-// Plugin Lifecycle Errors
-var (
-	ErrIsAlreadyInstalled = errors.New("can't install, the newest version is already installed")
-	ErrIsNotInstalled     = errors.New("plugin is not installed")
-	ErrIsAlreadyUpgraded  = errors.New("can't upgrade, the newest version is already installed")
-)
+// InstallOpts specifies options for plugin installation operation.
+type InstallOpts struct {
+	ArchiveFileOverride string
+}
+
+type installOperation struct {
+	pluginName string
+	platform   index.Platform
+
+	downloadStagingDir string
+	installDir         string
+	binDir             string
+}
 
 const (
 	krewPluginName = "krew"
 )
 
-func downloadAndMove(version, sha256sum, uri string, fos []index.FileOperation, downloadPath, installPath, forceDownloadFile string) (dst string, err error) {
-	glog.V(3).Infof("Creating download dir %q", downloadPath)
-	if err = os.MkdirAll(downloadPath, 0755); err != nil {
-		return "", errors.Wrapf(err, "could not create download path %q", downloadPath)
-	}
-	defer os.RemoveAll(downloadPath)
-
-	var fetcher download.Fetcher = download.HTTPFetcher{}
-	if forceDownloadFile != "" {
-		fetcher = download.NewFileFetcher(forceDownloadFile)
-	}
-
-	verifier := download.NewSha256Verifier(sha256sum)
-	if err := download.NewDownloader(verifier, fetcher).Get(uri, downloadPath); err != nil {
-		return "", errors.Wrap(err, "failed to download and verify file")
-	}
-	return moveToInstallDir(downloadPath, installPath, version, fos)
-}
+// Plugin lifecycle errors
+var (
+	ErrIsAlreadyInstalled = errors.New("can't install, the newest version is already installed")
+	ErrIsNotInstalled     = errors.New("plugin is not installed")
+	ErrIsAlreadyUpgraded  = errors.New("can't upgrade, the newest version is already installed")
+)
 
 // Install will download and install a plugin. The operation tries
 // to not get the plugin dir in a bad state if it fails during the process.
-func Install(p environment.Paths, plugin index.Plugin, forceDownloadFile string) error {
+func Install(p environment.Paths, plugin index.Plugin, opts InstallOpts) error {
 	glog.V(2).Infof("Looking for installed versions")
 	_, err := receipt.Load(p.PluginInstallReceiptPath(plugin.Name))
 	if err == nil {
@@ -71,42 +66,79 @@ func Install(p environment.Paths, plugin index.Plugin, forceDownloadFile string)
 		return errors.Wrap(err, "failed to look up plugin receipt")
 	}
 
-	glog.V(1).Infof("Finding download target for plugin %s", plugin.Name)
-	version, sha256, uri, fos, bin, err := getDownloadTarget(plugin)
+	// Find available installation candidate
+	candidate, ok, err := GetMatchingPlatform(plugin.Spec.Platforms)
 	if err != nil {
-		return err
+		return errors.Wrap(err, "failed trying to find a matching platform in plugin spec")
+	}
+	if !ok {
+		return errors.Wrapf(err, "plugin %q does not offer installation for this platform", plugin.Name)
 	}
 
 	// The actual install should be the last action so that a failure during receipt
 	// saving does not result in an installed plugin without receipt.
-	glog.V(3).Infof("Install plugin %s", plugin.Name)
-	if err := install(plugin.Name, version, sha256, uri, bin, p, fos, forceDownloadFile); err != nil {
+	glog.V(3).Infof("Install plugin %s at version=%s", plugin.Name, plugin.Spec.Version)
+	if err := install(installOperation{
+		pluginName: plugin.Name,
+		platform:   candidate,
+
+		downloadStagingDir: filepath.Join(p.DownloadPath(), plugin.Name),
+		binDir:             p.BinPath(),
+		installDir:         p.PluginVersionInstallPath(plugin.Name, plugin.Spec.Version),
+	}, opts); err != nil {
 		return errors.Wrap(err, "install failed")
 	}
 	glog.V(3).Infof("Storing install receipt for plugin %s", plugin.Name)
 	err = receipt.Store(plugin, p.PluginInstallReceiptPath(plugin.Name))
 	return errors.Wrap(err, "installation receipt could not be stored, uninstall may fail")
 }
 
-func install(plugin, version, sha256sum, uri, bin string, p environment.Paths, fos []index.FileOperation, forceDownloadFile string) error {
-	dst, err := downloadAndMove(version, sha256sum, uri, fos, filepath.Join(p.DownloadPath(), plugin), p.PluginInstallPath(plugin), forceDownloadFile)
-	if err != nil {
-		return errors.Wrap(err, "failed to download and move during installation")
+func install(op installOperation, opts InstallOpts) error {
+	// Download and extract
+	glog.V(3).Infof("Creating download staging directory %q", op.downloadStagingDir)
+	if err := os.MkdirAll(op.downloadStagingDir, 0755); err != nil {
+		return errors.Wrapf(err, "could not create download path %q", op.downloadStagingDir)
+	}
+	defer func() {
+		glog.V(3).Infof("Deleting the download staging directory %s", op.downloadStagingDir)
+		_ = os.RemoveAll(op.downloadStagingDir)
+	}()
+	if err := downloadAndExtract(op.downloadStagingDir, op.platform.URI, op.platform.Sha256, opts.ArchiveFileOverride); err != nil {
+		return errors.Wrap(err, "failed to download and extract")
 	}
 
-	subPathAbs, err := filepath.Abs(dst)
+	if err := moveToInstallDir(op.downloadStagingDir, op.installDir, op.platform.Files); err != nil {
+		return errors.Wrap(err, "failed while moving files to the installation directory")
+	}
+
+	subPathAbs, err := filepath.Abs(op.installDir)
 	if err != nil {
-		return errors.Wrapf(err, "failed to get the absolute fullPath of %q", dst)
+		return errors.Wrapf(err, "failed to get the absolute fullPath of %q", op.installDir)
 	}
-	fullPath := filepath.Join(dst, filepath.FromSlash(bin))
+	fullPath := filepath.Join(op.installDir, filepath.FromSlash(op.platform.Bin))
 	pathAbs, err := filepath.Abs(fullPath)
 	if err != nil {
 		return errors.Wrapf(err, "failed to get the absolute fullPath of %q", fullPath)
 	}
 	if _, ok := pathutil.IsSubPath(subPathAbs, pathAbs); !ok {
-		return errors.Wrapf(err, "the fullPath %q does not extend the sub-fullPath %q", fullPath, dst)
+		return errors.Wrapf(err, "the fullPath %q does not extend the sub-fullPath %q", fullPath, op.installDir)
+	}
+	err = createOrUpdateLink(op.binDir, fullPath, op.pluginName)
+	return errors.Wrap(err, "failed to link installed plugin")
+}
+
+// downloadAndExtract downloads the specified archive uri (or uses the provided overrideFile, if a non-empty value)
+// while validating its checksum with the provided sha256sum, and extracts its contents to extractDir that must be.
+// created.
+func downloadAndExtract(extractDir, uri, sha256sum, overrideFile string) error {
+	var fetcher download.Fetcher = download.HTTPFetcher{}
+	if overrideFile != "" {
+		fetcher = download.NewFileFetcher(overrideFile)
 	}
-	return createOrUpdateLink(p.BinPath(), filepath.Join(dst, filepath.FromSlash(bin)), plugin)
+
+	verifier := download.NewSha256Verifier(sha256sum)
+	err := download.NewDownloader(verifier, fetcher).Get(uri, extractDir)
+	return errors.Wrap(err, "failed to download and verify file")
 }
 
 // Uninstall will uninstall a plugin.
@@ -157,7 +189,7 @@ func createOrUpdateLink(binDir string, binary string, plugin string) error {
 	}
 
 	// Create new
-	glog.V(2).Infof("Creating symlink from %q to %q", binary, dst)
+	glog.V(2).Infof("Creating symlink to %q at %q", binary, dst)
 	if err := os.Symlink(binary, dst); err != nil {
 		return errors.Wrapf(err, "failed to create a symlink form %q to %q", binDir, dst)
 	}

pkg/installation/install_test.go

@@ -16,6 +16,8 @@ package installation
 
 import (
 	"io/ioutil"
+	"net/http"
+	"net/http/httptest"
 	"os"
 	"path/filepath"
 	"reflect"
@@ -212,3 +214,46 @@ func Test_isWindows_envOverride(t *testing.T) {
 		t.Fatalf("isWindows()=true when KREW_OS != windows")
 	}
 }
+
+func Test_downloadAndExtract(t *testing.T) {
+	tmpDir, cleanup := testutil.NewTempDir(t)
+	defer cleanup()
+
+	// start a local http server to serve the test archive from pkg/download/testdata
+	testdataDir := filepath.Join(testdataPath(t), "..", "..", "download", "testdata")
+	server := httptest.NewServer(http.FileServer(http.Dir(testdataDir)))
+	defer server.Close()
+
+	url := server.URL + "/test-without-directory.tar.gz"
+	checksum := "433b9e0b6cb9f064548f451150799daadcc70a3496953490c5148c8e550d2f4e"
+
+	if err := downloadAndExtract(tmpDir.Root(), url, checksum, ""); err != nil {
+		t.Fatal(err)
+	}
+	files, err := ioutil.ReadDir(tmpDir.Root())
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(files) == 0 {
+		t.Fatal("no files found in the extract output directory")
+	}
+}
+
+func Test_downloadAndExtract_fileOverride(t *testing.T) {
+	tmpDir, cleanup := testutil.NewTempDir(t)
+	defer cleanup()
+
+	testFile := filepath.Join(testdataPath(t), "..", "..", "download", "testdata", "test-without-directory.tar.gz")
+	checksum := "433b9e0b6cb9f064548f451150799daadcc70a3496953490c5148c8e550d2f4e"
+
+	if err := downloadAndExtract(tmpDir.Root(), "", checksum, testFile); err != nil {
+		t.Fatal(err)
+	}
+	files, err := ioutil.ReadDir(tmpDir.Root())
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(files) == 0 {
+		t.Fatal("no files found in the extract output directory")
+	}
+}

pkg/installation/move.go

@@ -151,35 +151,37 @@ func moveAllFiles(fromDir, toDir string, fos []index.FileOperation) error {
 	return nil
 }
 
-func moveToInstallDir(download, pluginDir, version string, fos []index.FileOperation) (string, error) {
-	glog.V(4).Infof("Creating plugin dir %q", pluginDir)
-	if err := os.MkdirAll(pluginDir, 0755); err != nil {
-		return "", errors.Wrapf(err, "error creating path to %q", pluginDir)
+// moveToInstallDir moves plugins from srcDir to dstDir (created in this method) with given FileOperation.
+func moveToInstallDir(srcDir, installDir string, fos []index.FileOperation) error {
+	glog.V(4).Infof("Creating plugin installation directory %q", installDir)
+	if err := os.MkdirAll(installDir, 0755); err != nil {
+		return errors.Wrapf(err, "error creating installation directory at %q", installDir)
 	}
 
-	tempdir, err := ioutil.TempDir("", "krew-temp-move")
-	glog.V(4).Infof("Creating temp plugin move operations dir %q", tempdir)
+	tmp, err := ioutil.TempDir("", "krew-temp-move")
+	glog.V(4).Infof("Creating temp plugin move operations dir %q", tmp)
 	if err != nil {
-		return "", errors.Wrap(err, "failed to find a temporary director")
+		return errors.Wrap(err, "failed to find a temporary director")
 	}
-	defer os.RemoveAll(tempdir)
+	defer os.RemoveAll(tmp)
 
-	if err = moveAllFiles(download, tempdir, fos); err != nil {
-		return "", errors.Wrap(err, "failed to move files")
+	if err = moveAllFiles(srcDir, tmp, fos); err != nil {
+		return errors.Wrap(err, "failed to move files")
 	}
 
-	installPath := filepath.Join(pluginDir, version)
-	glog.V(2).Infof("Move directory %q to %q", tempdir, installPath)
-	if err = moveOrCopyDir(tempdir, installPath); err != nil {
-		defer os.Remove(installPath)
-		return "", errors.Wrapf(err, "could not rename file from %q to %q", tempdir, installPath)
+	glog.V(2).Infof("Move directory %q to %q", tmp, installDir)
+	if err = moveOrCopyDir(tmp, installDir); err != nil {
+		defer func() {
+			glog.V(3).Info("Cleaning up installation directory due to error during copying files")
+			os.Remove(installDir)
+		}()
+		return errors.Wrapf(err, "could not rename file from %q to %q", tmp, installDir)
 	}
-
-	return installPath, nil
+	return nil
 }
 
-// moveOrCopyDir will try to rename a dir or file. If rename is not supported a
-// manual copy will be performed. Existing files at "to" will be deleted
+// moveOrCopyDir will try to rename a dir or file. If rename is not supported, a manual copy will be performed.
+// Existing files at "to" will be deleted.
 func moveOrCopyDir(from, to string) error {
 	// Try atomic rename (does not work cross partition).
 	fi, err := os.Stat(to)

pkg/installation/upgrade.go

@@ -17,6 +17,7 @@ package installation
 import (
 	"io/ioutil"
 	"os"
+	"path/filepath"
 
 	"github.com/golang/glog"
 	"github.com/pkg/errors"
@@ -38,17 +39,22 @@ func Upgrade(p environment.Paths, plugin index.Plugin) error {
 	curVersion := installReceipt.Spec.Version
 	curv, err := semver.Parse(curVersion)
 	if err != nil {
-		return errors.Wrapf(err, "failed to parse installed version (%q) plugin %q as semantic version", curVersion, plugin.Name)
+		return errors.Wrapf(err, "failed to parse installed plugin version (%q) as a semver value", curVersion)
 	}
 
 	// Find available installation candidate
-	newVersion, sha256sum, uri, fos, binName, err := getDownloadTarget(plugin)
+	candidate, ok, err := GetMatchingPlatform(plugin.Spec.Platforms)
 	if err != nil {
-		return errors.Wrap(err, "failed to get the current download target")
+		return errors.Wrap(err, "failed trying to find a matching platform in plugin spec")
 	}
+	if !ok {
+		return errors.Wrapf(err, "plugin %q does not offer installation for this platform", plugin.Name)
+	}
+
+	newVersion := plugin.Spec.Version
 	newv, err := semver.Parse(newVersion)
 	if err != nil {
-		return errors.Wrapf(err, "failed to parse installation candidate version spec (%q) for plugin %q", newVersion, plugin.Name)
+		return errors.Wrapf(err, "failed to parse candidate version spec (%q)", newVersion)
 	}
 	glog.V(2).Infof("Comparing versions: current=%s target=%s", curv, newv)
 
@@ -66,7 +72,13 @@ func Upgrade(p environment.Paths, plugin index.Plugin) error {
 
 	// Re-Install
 	glog.V(1).Infof("Installing new version %s", newVersion)
-	if err := install(plugin.Name, newVersion, sha256sum, uri, binName, p, fos, ""); err != nil {
+	if err := install(installOperation{
+		pluginName:         plugin.Name,
+		platform:           candidate,
+		downloadStagingDir: filepath.Join(p.DownloadPath(), plugin.Name),
+		installDir:         p.PluginVersionInstallPath(plugin.Name, newVersion),
+		binDir:             p.BinPath(),
+	}, InstallOpts{}); err != nil {
 		return errors.Wrap(err, "failed to install new version")
 	}
 
@@ -76,7 +88,7 @@ func Upgrade(p environment.Paths, plugin index.Plugin) error {
 }
 
 // removePluginVersionFromFS will remove a plugin directly if it not krew.
-
+//
 // Krew on Windows needs special care because active directories can't be
 // deleted. This method will unlink old krew versions and during next run clean
 // the directory.