From d7cc48739bcb3731d97794ac858e76322a1b8925 Mon Sep 17 00:00:00 2001 From: Ahmet Alp Balkan Date: Mon, 22 Jul 2019 14:39:10 -0700 Subject: [PATCH] pkg/installation: cleanup refactoring - `getDownloadTarget()`: removed, it didn't do anything interesting. - `downloadAndMove()`: - renamed to `downloadAndExtract()` which better reflects the functionality - reduced methods from 7 to 4 - refactored the "move" operation out of this method (decouples "move" from "download+extract") and we now call "move" from install(). - added unit tests (both for downloading a file, and --archive override) - introduced `installOperation` which contains the minimal information to perform installation operation. - introduced `InstallOpts` type to scale out (in the future) - `install()`: - accepts fewer arguments (2) than before (8) - no longer knows about index.Plugin, or environment.Paths - cmd/install: removed a redundant check detecting specifying plugin names along with --manifest option. Signed-off-by: Ahmet Alp Balkan --- cmd/krew/cmd/install.go | 20 +++--- pkg/installation/install.go | 112 ++++++++++++++++++++----------- pkg/installation/install_test.go | 45 +++++++++++++ pkg/installation/move.go | 40 +++++------ pkg/installation/upgrade.go | 24 +++++-- pkg/installation/util.go | 20 ------ pkg/installation/util_test.go | 70 ------------------- 7 files changed, 164 insertions(+), 167 deletions(-) diff --git a/cmd/krew/cmd/install.go b/cmd/krew/cmd/install.go index 8a7ac0e4..7afb05cd 100644 --- a/cmd/krew/cmd/install.go +++ b/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) diff --git a/pkg/installation/install.go b/pkg/installation/install.go index 426cba71..85bab4a6 100644 --- a/pkg/installation/install.go +++ b/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,16 +66,26 @@ 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) @@ -88,25 +93,52 @@ func Install(p environment.Paths, plugin index.Plugin, forceDownloadFile string) 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) } diff --git a/pkg/installation/install_test.go b/pkg/installation/install_test.go index 53870114..7a7966fb 100644 --- a/pkg/installation/install_test.go +++ b/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") + } +} diff --git a/pkg/installation/move.go b/pkg/installation/move.go index 8021f2b6..9fc1bdf1 100644 --- a/pkg/installation/move.go +++ b/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) diff --git a/pkg/installation/upgrade.go b/pkg/installation/upgrade.go index 5a5b3257..a7ec43b0 100644 --- a/pkg/installation/upgrade.go +++ b/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. diff --git a/pkg/installation/util.go b/pkg/installation/util.go index 22b34449..4410e95f 100644 --- a/pkg/installation/util.go +++ b/pkg/installation/util.go @@ -21,29 +21,9 @@ import ( "github.com/pkg/errors" "sigs.k8s.io/krew/pkg/constants" - "sigs.k8s.io/krew/pkg/index" "sigs.k8s.io/krew/pkg/installation/receipt" ) -func getDownloadTarget(index index.Plugin) (version, sha256sum, uri string, fos []index.FileOperation, bin string, err error) { - // TODO(ahmetb): We have many return values from this method, indicating - // code smell. More specifically we return all-or-nothing, so ideally this - // should be converted into a struct, like InstallOperation{} contains all - // the data needed to install a plugin. - p, ok, err := GetMatchingPlatform(index.Spec.Platforms) - if err != nil { - return "", "", "", nil, p.Bin, errors.Wrap(err, "failed to get matching platforms") - } - if !ok { - return "", "", "", nil, p.Bin, errors.New("no matching platform found") - } - version = index.Spec.Version - uri = p.URI - sha256sum = p.Sha256 - glog.V(4).Infof("found a matching platform, version=%s checksum=%s", version, sha256sum) - return version, sha256sum, uri, p.Files, p.Bin, nil -} - // ListInstalledPlugins returns a list of all install plugins in a // name:version format based on the install receipts at the specified dir. func ListInstalledPlugins(receiptsDir string) (map[string]string, error) { diff --git a/pkg/installation/util_test.go b/pkg/installation/util_test.go index f1395c9c..cc749e44 100644 --- a/pkg/installation/util_test.go +++ b/pkg/installation/util_test.go @@ -16,79 +16,9 @@ package installation import ( "path/filepath" - "reflect" - "runtime" "testing" - - "sigs.k8s.io/krew/pkg/index" - "sigs.k8s.io/krew/pkg/testutil" ) -func Test_getDownloadTarget(t *testing.T) { - tests := []struct { - name string - plugin index.Plugin - wantVersion string - wantSHA256Sum string - wantURI string - wantFos []index.FileOperation - wantBin string - wantErr bool - }{ - { - name: "matches to a platform in the list", - plugin: testutil.NewPlugin(). - WithVersion("v1.0.1").WithPlatforms( - testutil.NewPlatform().WithOS("none").V(), - testutil.NewPlatform().WithBin("kubectl-foo"). - WithOS(runtime.GOOS). - WithFiles([]index.FileOperation{{From: "a", To: "b"}}). - WithSHA256("f0f0f0"). - WithURI("http://localhost").V()).V(), - wantVersion: "v1.0.1", - wantSHA256Sum: "f0f0f0", - wantURI: "http://localhost", - wantFos: []index.FileOperation{{From: "a", To: "b"}}, - wantBin: "kubectl-foo", - wantErr: false, - }, - { - name: "does not match to a platform", - plugin: testutil.NewPlugin(). - WithVersion("v1.0.1"). - WithPlatforms( - testutil.NewPlatform().WithOS("foo").V(), - testutil.NewPlatform().WithOS("bar").V(), - ).V(), - wantErr: true, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - gotVersion, gotSHA256Sum, gotURI, gotFos, bin, err := getDownloadTarget(tt.plugin) - if (err != nil) != tt.wantErr { - t.Errorf("getDownloadTarget() error = %v, wantErr %v", err, tt.wantErr) - return - } - if gotVersion != tt.wantVersion { - t.Errorf("getDownloadTarget() gotVersion = %v, want %v", gotVersion, tt.wantVersion) - } - if gotSHA256Sum != tt.wantSHA256Sum { - t.Errorf("getDownloadTarget() gotSHA256Sum = %v, want %v", gotSHA256Sum, tt.wantSHA256Sum) - } - if bin != tt.wantBin { - t.Errorf("getDownloadTarget() bin = %v, want %v", bin, tt.wantBin) - } - if gotURI != tt.wantURI { - t.Errorf("getDownloadTarget() gotURI = %v, want %v", gotURI, tt.wantURI) - } - if !reflect.DeepEqual(gotFos, tt.wantFos) { - t.Errorf("getDownloadTarget() gotFos = %v, want %v", gotFos, tt.wantFos) - } - }) - } -} - func testdataPath(t *testing.T) string { pwd, err := filepath.Abs(".") if err != nil {