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

Refactor: move errorFetcher struct to test file
This fetcher was only used in its test.

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

This pull-request has been approved by: *<a href="https://github.com/kubernetes-sigs/krew/pull/387#" 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/387?src=pr&el=h1) Report
> Merging [#387](https://codecov.io/gh/kubernetes-sigs/krew/pull/387?src=pr&el=desc) into [master](https://codecov.io/gh/kubernetes-sigs/krew/commit/b9a0e88ef1d1a7122b99f29b61d050d6e01562a5?src=pr&el=desc) will **decrease** coverage by `0.53%`.
> The diff coverage is `n/a`.

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

```diff
@@            Coverage Diff             @@
##           master     #387      +/-   ##
==========================================
- Coverage   57.11%   56.57%   -0.54%     
==========================================
  Files          19       19              
  Lines         914      912       -2     
==========================================
- Hits          522      516       -6     
- Misses        339      341       +2     
- Partials       53       55       +2
```


| [Impacted Files](https://codecov.io/gh/kubernetes-sigs/krew/pull/387?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/download/fetch.go](https://codecov.io/gh/kubernetes-sigs/krew/pull/387/diff?src=pr&el=tree#diff-cGtnL2Rvd25sb2FkL2ZldGNoLmdv) | `44.44% <ø> (-10.11%)` | :arrow_down: |
| [pkg/download/downloader.go](https://codecov.io/gh/kubernetes-sigs/krew/pull/387/diff?src=pr&el=tree#diff-cGtnL2Rvd25sb2FkL2Rvd25sb2FkZXIuZ28=) | `65.21% <0%> (-3.48%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/kubernetes-sigs/krew/pull/387?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/387?src=pr&el=footer). Last update [b9a0e88...b52c135](https://codecov.io/gh/kubernetes-sigs/krew/pull/387?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

ahmetb: It actually helps us have code coverage around error cases in downloader.
That's why it's there. :)
/hold
ahmetb: In other words, we ensure the errors from downloader implementation are returned correctly.
corneliusweig: Whoops .. sorry I wasn't thinking straight. Thanks for catching this. I do think however, that it should have been in the test file.
/retitle Refactor: move errorFetcher struct to test file

Patch:
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/fetch.go b/pkg/download/fetch.go
index e320302..aea253d 100644
--- a/pkg/download/fetch.go
+++ b/pkg/download/fetch.go
@@ -53,11 +53,3 @@ func (f fileFetcher) Get(_ string) (io.ReadCloser, error) {
 
 // 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/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/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)
 			}