Repository: kubernetes-sigs/krew
Pull request #386 (2019-11-14 20:11:37)

Make logging more consistent for downloaded vs. local archives
This is a follow-up for #361 . I didn't realize that the logging was equally misleading.

Also see https://github.com/kubernetes-sigs/krew/pull/375#issuecomment-553653013

k8s-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-sigs/krew/pull/386#" title="Author self-approved">corneliusweig</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-sigs%2Fkrew).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/kubernetes-sigs/krew/blob/master/OWNERS)~~ [corneliusweig]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
codecov-io: # [Codecov](https://codecov.io/gh/kubernetes-sigs/krew/pull/386?src=pr&el=h1) Report
> Merging [#386](https://codecov.io/gh/kubernetes-sigs/krew/pull/386?src=pr&el=desc) into [master](https://codecov.io/gh/kubernetes-sigs/krew/commit/b9a0e88ef1d1a7122b99f29b61d050d6e01562a5?src=pr&el=desc) will **decrease** coverage by `0.06%`.
> The diff coverage is `66.66%`.

[![Impacted file tree graph](https://codecov.io/gh/kubernetes-sigs/krew/pull/386/graphs/tree.svg?width=650&token=J3nFtARI2m&height=150&src=pr)](https://codecov.io/gh/kubernetes-sigs/krew/pull/386?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #386      +/-   ##
==========================================
- Coverage   57.11%   57.04%   -0.07%     
==========================================
  Files          19       19              
  Lines         914      915       +1     
==========================================
  Hits          522      522              
- Misses        339      340       +1     
  Partials       53       53
```


| [Impacted Files](https://codecov.io/gh/kubernetes-sigs/krew/pull/386?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/download/fetch.go](https://codecov.io/gh/kubernetes-sigs/krew/pull/386/diff?src=pr&el=tree#diff-cGtnL2Rvd25sb2FkL2ZldGNoLmdv) | `53.84% <33.33%> (-0.7%)` | :arrow_down: |
| [pkg/download/downloader.go](https://codecov.io/gh/kubernetes-sigs/krew/pull/386/diff?src=pr&el=tree#diff-cGtnL2Rvd25sb2FkL2Rvd25sb2FkZXIuZ28=) | `68.42% <83.33%> (-0.28%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/kubernetes-sigs/krew/pull/386?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/kubernetes-sigs/krew/pull/386?src=pr&el=footer). Last update [b9a0e88...7778375](https://codecov.io/gh/kubernetes-sigs/krew/pull/386?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/.travis.yml b/.travis.yml
index 23bb835..e4de39e 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -25,7 +25,6 @@ after_success:
 env:
   global:
   - GO111MODULE=on
-  - GOPROXY=https://proxy.golang.org
   - secure: Ly5oLE9sC7NDvwyZEzOi8gPeHdu3NTP2OuDa8BOUngAanXhaEInzBJ+Pod+TIKFzpope4/z8idMN3rZfPV5LcXzhufk82LSpYlGhZgFDYajvv3eNN20arZQf4kjohd5U2aY1XrYgrgBbodv7dCLnitxotO65tTzQN5xh6evwW6Ji2Em7t42kvz9VHj2nptWKsciQEr6KNb2xDJ7PSvFKdPKq0q3Vuy7PFToZvNbsk+FBLSjYpAHG1KrlyX/WiM1mLu7jCJ4yuPx4n9+CM5Zzycbc/6mrs8ZiR+myyaCMzh6pEltdR0vzSlFIl2TJvUwzTUyTXcZsQ/ENa1ZdOkQwI4jpV3IDTQ6AlMRtBSbL5/uSOvHYs1HBYTbJJeZLiMjru9SM6D9AUW9HRr0JQGeQFsPE/4mzl39Xgl+3GE4+6sfNiaurpvgNArQNBAs2nZ0V3myiDoSG/sAL/2YzXHiG0HVhVk65P33d9SevJjBJRTobhRf4Lt0lT2IO6XF/4kQIvMnaCPQRS1X5tMIHFeTzn1dwrFiZqd1Sbwq5cd8H25nt1XnXsCTgRegJPNUxqRHnDpR5M3HRiW+TmjJIte7Y6exCihonM3afj3rsu3Q/ePwj7ckzgq8CP2yJ//Sxu4FH65LGQ/2bHJUneKL33Uq1W68c+qtKWMGLB4LNRGJVZkc=
   - secure: o6ZTdC1H4elXUrqt8XFfV7zuN9mWjQcV5iMicyG7TU/Fn8HIOluzRYPNgmdtuUpLYaQ7KD1t79+QNwuWtQOOHO/eQJ2/LCwO6cSVfxa6sM560r6qOtXraJugq0sCjOY8mNcwo7toY8vtf8yHa1an4L8CSm0SkH0h2EaBE0SE+Vfmb0T0RD/5f2i0Ni8Vn9QLe2P32CRT8VWhFNs+rqj1VwpspDjwa/X+R44RBgLmf61bZQpW5bKauLTR/JrXejppkSXRRqYFDokYTbAVOZbTacwqj7b4D+H6tUb8PaG6wMu5GLe1/rLwREn49cWA3LLX/U7wcUqbIW9jYYFRjt4Cs4YFZXIsaqhAcBcitajFXiZsOdYgHnIHNR4C86oH/dM5UjVSXcpuAw8xcJ4AAEaF3EE3C+bfWZZBHbmfdF1mDbezuTKGynzsQs6i9tlOlrgL9KVsj4YThPqk8Mfa3UOHSG6dOKVqxECPb5voSzKQvRyrzPVjONgmuHf4r/7E8ur1hsXuZGnaVphWTG+fPX4Sdiuo2kn1pOCLt1C8aXqsDpKcnzBjm1yeV8m7jIOBDhPm0mrGHjiCix7NqgxUPuAwLC1zIAXIKyAIpJzig+WlQC6gFu/7zlm0qrowxLImX4seb4rWp90OFcutek0NEVy1BO2LNn4TspaeTWkqD9vR4yQ=
 deploy:
diff --git a/cmd/krew/cmd/install.go b/cmd/krew/cmd/install.go
index 69f45b9..66b1393 100644
--- a/cmd/krew/cmd/install.go
+++ b/cmd/krew/cmd/install.go
@@ -114,6 +114,7 @@ Remarks:
 			}
 
 			var failed []string
+			var returnErr error
 			for _, plugin := range install {
 				fmt.Fprintf(os.Stderr, "Installing plugin: %s\n", plugin.Name)
 				err := installation.Install(paths, plugin, installation.InstallOpts{
@@ -125,6 +126,9 @@ Remarks:
 				}
 				if err != nil {
 					glog.Warningf("failed to install plugin %q: %v", plugin.Name, err)
+					if returnErr == nil {
+						returnErr = err
+					}
 					failed = append(failed, plugin.Name)
 					continue
 				}
@@ -135,7 +139,7 @@ Remarks:
 				internal.PrintSecurityNotice()
 			}
 			if len(failed) > 0 {
-				return errors.Errorf("failed to install some plugins: %+v", failed)
+				return errors.Wrapf(returnErr, "failed to install some plugins: %+v", failed)
 			}
 			return nil
 		},
diff --git a/pkg/download/downloader.go b/pkg/download/downloader.go
index 55b8a04..c63cb10 100644
--- a/pkg/download/downloader.go
+++ b/pkg/download/downloader.go
@@ -33,26 +33,25 @@ import (
 // download gets a file from the internet in memory and writes it content
 // to a Verifier.
 func download(url string, verifier Verifier, fetcher Fetcher) (io.ReaderAt, int64, error) {
-	glog.V(2).Infof("Fetching %q", url)
 	body, err := fetcher.Get(url)
 	if err != nil {
-		return nil, 0, errors.Wrapf(err, "could not download %q", url)
+		return nil, 0, errors.Wrapf(err, "failed to obtain plugin archive")
 	}
 	defer body.Close()
 
-	glog.V(3).Infof("Reading download data into memory")
+	glog.V(3).Infof("Reading archive file into memory")
 	data, err := ioutil.ReadAll(io.TeeReader(body, verifier))
 	if err != nil {
-		return nil, 0, errors.Wrap(err, "could not read download content")
+		return nil, 0, errors.Wrap(err, "could not read archive")
 	}
-	glog.V(2).Infof("Read %d bytes of download data into memory", len(data))
+	glog.V(2).Infof("Read %d bytes from archive into memory", len(data))
 
 	return bytes.NewReader(data), int64(len(data)), verifier.Verify()
 }
 
 // extractZIP extracts a zip file into the target directory.
 func extractZIP(targetDir string, read io.ReaderAt, size int64) error {
-	glog.V(4).Infof("Extracting download zip to %q", targetDir)
+	glog.V(4).Infof("Extracting zip archive to %q", targetDir)
 	zipReader, err := zip.NewReader(read, size)
 	if err != nil {
 		return err
@@ -204,7 +203,7 @@ func extractArchive(dst string, at io.ReaderAt, size int64) error {
 	glog.V(4).Infof("detected %q file type", t)
 	exf, ok := defaultExtractors[t]
 	if !ok {
-		return errors.Errorf("mime type %q for downloaded file is not a supported archive format", t)
+		return errors.Errorf("mime type %q for archive file is not a supported archive format", t)
 	}
 	return errors.Wrap(exf(dst, at, size), "failed to extract file")
 
diff --git a/pkg/download/fetch.go b/pkg/download/fetch.go
index e320302..6aea40d 100644
--- a/pkg/download/fetch.go
+++ b/pkg/download/fetch.go
@@ -19,6 +19,7 @@ import (
 	"net/http"
 	"os"
 
+	"github.com/golang/glog"
 	"github.com/pkg/errors"
 )
 
@@ -35,9 +36,10 @@ type HTTPFetcher struct{}
 
 // Get gets the file and returns an stream to read the file.
 func (HTTPFetcher) Get(uri string) (io.ReadCloser, error) {
+	glog.V(2).Infof("Fetching %q", uri)
 	resp, err := http.Get(uri)
 	if err != nil {
-		return nil, errors.Wrapf(err, "failed to get the uri %q", uri)
+		return nil, errors.Wrapf(err, "failed to download %q", uri)
 	}
 	return resp.Body, nil
 }
@@ -47,17 +49,10 @@ var _ Fetcher = fileFetcher{}
 type fileFetcher struct{ f string }
 
 func (f fileFetcher) Get(_ string) (io.ReadCloser, error) {
+	glog.V(2).Infof("Reading %q", f.f)
 	file, err := os.Open(f.f)
 	return file, errors.Wrapf(err, "failed to open archive file %q for reading", f.f)
 }
 
 // NewFileFetcher returns a local file reader.
 func NewFileFetcher(path string) Fetcher { return fileFetcher{f: path} }
-
-var _ Fetcher = errorFetcher{}
-
-type errorFetcher struct{}
-
-func (f errorFetcher) Get(_ string) (io.ReadCloser, error) {
-	return nil, errors.New("test fail")
-}
diff --git a/pkg/installation/install.go b/pkg/installation/install.go
index 43692d5..7afe7fc 100644
--- a/pkg/installation/install.go
+++ b/pkg/installation/install.go
@@ -199,7 +199,7 @@ func createOrUpdateLink(binDir string, binary string, plugin string) error {
 	// Create new
 	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)
+		return errors.Wrapf(err, "failed to create a symlink from %q to %q", binary, dst)
 	}
 	glog.V(2).Infof("Created symlink at %q", dst)
 
diff --git a/pkg/installation/move.go b/pkg/installation/move.go
index 049857a..fe08237 100644
--- a/pkg/installation/move.go
+++ b/pkg/installation/move.go
@@ -134,8 +134,8 @@ func moveFiles(fromDir, toDir string, fo index.FileOperation) error {
 			return errors.Wrapf(err, "failed to create move path %q", filepath.Dir(m.to))
 		}
 
-		if err = os.Rename(m.from, m.to); err != nil {
-			return errors.Wrapf(err, "could not rename file from %q to %q", m.from, m.to)
+		if err = renameOrCopy(m.from, m.to); err != nil {
+			return errors.Wrapf(err, "could not rename/copy file from %q to %q", m.from, m.to)
 		}
 	}
 	glog.V(4).Infoln("Move operations are complete")
@@ -170,19 +170,19 @@ func moveToInstallDir(srcDir, installDir string, fos []index.FileOperation) erro
 	}
 
 	glog.V(2).Infof("Move directory %q to %q", tmp, installDir)
-	if err = moveOrCopyDir(tmp, installDir); err != nil {
+	if err = renameOrCopy(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 errors.Wrapf(err, "could not rename/copy directory %q to %q", tmp, installDir)
 	}
 	return nil
 }
 
-// moveOrCopyDir will try to rename a dir or file. If rename is not supported, a manual copy will be performed.
+// renameOrCopy 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 {
+func renameOrCopy(from, to string) error {
 	// Try atomic rename (does not work cross partition).
 	fi, err := os.Stat(to)
 	if err != nil && !os.IsNotExist(err) {
@@ -198,16 +198,15 @@ func moveOrCopyDir(from, to string) error {
 
 	err = os.Rename(from, to)
 	// Fallback for invalid cross-device link (errno:18).
-	if le, ok := err.(*os.LinkError); err != nil && ok {
-		if errno, ok := le.Err.(syscall.Errno); ok && errno == 18 {
-			glog.V(4).Infof("Cross-device link error (ERRNO=18), fallback to manual copy")
-			return copyDir(from, to)
-		}
+	if isCrossDeviceRenameErr(err) {
+		glog.V(2).Infof("Cross-device link error while copying, fallback to manual copy")
+		return errors.Wrap(copyTree(from, to), "failed to copy directory tree as a fallback")
 	}
 	return err
 }
 
-func copyDir(from string, to string) (err error) {
+// copyTree copies files or directories, recursively.
+func copyTree(from, to string) (err error) {
 	return filepath.Walk(from, func(path string, info os.FileInfo, err error) error {
 		if err != nil {
 			return err
@@ -224,7 +223,7 @@ func copyDir(from string, to string) (err error) {
 	})
 }
 
-func copyFile(source string, dst string, mode os.FileMode) (err error) {
+func copyFile(source, dst string, mode os.FileMode) (err error) {
 	sf, err := os.Open(source)
 	if err != nil {
 		return err
@@ -243,3 +242,17 @@ func copyFile(source string, dst string, mode os.FileMode) (err error) {
 	}
 	return os.Chmod(dst, mode)
 }
+
+// isCrossDeviceRenameErr determines if a os.Rename error is due to cross-fs/drive/volume copying.
+func isCrossDeviceRenameErr(err error) bool {
+	le, ok := err.(*os.LinkError)
+	if !ok {
+		return false
+	}
+	errno, ok := le.Err.(syscall.Errno)
+	if !ok {
+		return false
+	}
+	return (IsWindows() && errno == 17) || // syscall.ERROR_NOT_SAME_DEVICE
+		(!IsWindows() && errno == 18) // syscall.EXDEV
+}
diff --git a/pkg/receiptsmigration/migration.go b/pkg/receiptsmigration/migration.go
index d6df854..b29fbda 100644
--- a/pkg/receiptsmigration/migration.go
+++ b/pkg/receiptsmigration/migration.go
@@ -44,19 +44,12 @@ func Done(newPaths environment.Paths) (bool, error) {
 	if err != nil {
 		return false, err
 	}
-	store, err := ioutil.ReadDir(newPaths.InstallPath())
+	plugins, err := ioutil.ReadDir(newPaths.BinPath())
 	if err != nil {
 		return false, err
 	}
 
-	var hasInstalledPlugins bool
-	for _, entry := range store {
-		if entry.IsDir() {
-			hasInstalledPlugins = true
-			break
-		}
-	}
-
+	hasInstalledPlugins := len(plugins) > 0
 	hasNoReceipts := len(receipts) == 0
 
 	return !(hasInstalledPlugins && hasNoReceipts), nil


Test patch:
diff --git a/integration_test/testutil_test.go b/integration_test/testutil_test.go
index da2ba7a..c4e5fa3 100644
--- a/integration_test/testutil_test.go
+++ b/integration_test/testutil_test.go
@@ -84,6 +84,10 @@ func setupKrewBin(t *testing.T, tempDir *testutil.TempDir) string {
 	if err := os.MkdirAll(binPath, 0755); err != nil {
 		t.Fatal(err)
 	}
+
+	// todo(corneliusweig): when the receipts migration logic is removed, this becomes obsolete
+	tempDir.Write("receipts/krew.notyaml", []byte("must be present for receipts migration check"))
+
 	if err := os.Symlink(krewBinary, filepath.Join(binPath, "kubectl-krew")); err != nil {
 		t.Fatalf("cannot link krew binary: %s", err)
 	}
diff --git a/pkg/download/downloader_test.go b/pkg/download/downloader_test.go
index 0469a2c..8a9848d 100644
--- a/pkg/download/downloader_test.go
+++ b/pkg/download/downloader_test.go
@@ -154,6 +154,12 @@ func collectFiles(t *testing.T, scanPath string) []string {
 	return outFiles
 }
 
+var _ Fetcher = errorFetcher{}
+
+type errorFetcher struct{}
+
+func (f errorFetcher) Get(_ string) (io.ReadCloser, error) { return nil, errors.New("test fail") }
+
 func TestDownloader_Get(t *testing.T) {
 	type fields struct {
 		verifier Verifier
diff --git a/pkg/installation/move_test.go b/pkg/installation/move_test.go
index 8980855..dfd7c9e 100644
--- a/pkg/installation/move_test.go
+++ b/pkg/installation/move_test.go
@@ -196,7 +196,7 @@ func Test_moveOrCopyDir_canMoveToNonExistingDir(t *testing.T) {
 
 	dst := dstDir.Path("non-existing-dir")
 
-	if err := moveOrCopyDir(srcDir.Root(), dst); err != nil {
+	if err := renameOrCopy(srcDir.Root(), dst); err != nil {
 		t.Fatalf("move failed: %+v", err)
 	}
 
@@ -225,7 +225,7 @@ func Test_moveOrCopyDir_removesExistingTarget(t *testing.T) {
 		dstDir.Write(fmt.Sprintf("file-%d", i), nil)
 	}
 
-	if err := moveOrCopyDir(srcDir.Root(), dstDir.Root()); err != nil {
+	if err := renameOrCopy(srcDir.Root(), dstDir.Root()); err != nil {
 		t.Fatalf("move failed: %+v", err)
 	}
 
diff --git a/pkg/receiptsmigration/migration_test.go b/pkg/receiptsmigration/migration_test.go
index 4742b9e..f8e3fc9 100644
--- a/pkg/receiptsmigration/migration_test.go
+++ b/pkg/receiptsmigration/migration_test.go
@@ -39,7 +39,7 @@ func TestIsMigrated(t *testing.T) {
 	}{
 		{
 			name:         "One plugin and receipts",
-			filesPresent: []string{"store/konfig/konfig.sh", "receipts/present"},
+			filesPresent: []string{"bin/foo", "receipts/present"},
 			expected:     true,
 		},
 		{
@@ -48,7 +48,7 @@ func TestIsMigrated(t *testing.T) {
 		},
 		{
 			name:         "When a plugin is installed but no receipts",
-			filesPresent: []string{"store/konfig/konfig.sh"},
+			filesPresent: []string{"bin/foo"},
 			expected:     false,
 		},
 		{
@@ -69,7 +69,7 @@ func TestIsMigrated(t *testing.T) {
 			newPaths := environment.MustGetKrewPaths()
 
 			_ = os.MkdirAll(tmpDir.Path("receipts"), os.ModePerm)
-			_ = os.MkdirAll(tmpDir.Path("store"), os.ModePerm)
+			_ = os.MkdirAll(tmpDir.Path("bin"), os.ModePerm)
 			for _, name := range test.filesPresent {
 				touch(tmpDir, name)
 			}