Repository: kubernetes-sigs/krew
Pull request #361 (2019-10-29 21:44:46)

Improve error messages when installing from manifest+archive
Related #358

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

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

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

```diff
@@           Coverage Diff           @@
##           master     #361   +/-   ##
=======================================
  Coverage   56.78%   56.78%           
=======================================
  Files          19       19           
  Lines         907      907           
=======================================
  Hits          515      515           
  Misses        339      339           
  Partials       53       53
```


| [Impacted Files](https://codecov.io/gh/kubernetes-sigs/krew/pull/361?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/download/fetch.go](https://codecov.io/gh/kubernetes-sigs/krew/pull/361/diff?src=pr&el=tree#diff-cGtnL2Rvd25sb2FkL2ZldGNoLmdv) | `50% <0%> (ø)` | :arrow_up: |
| [pkg/download/downloader.go](https://codecov.io/gh/kubernetes-sigs/krew/pull/361/diff?src=pr&el=tree#diff-cGtnL2Rvd25sb2FkL2Rvd25sb2FkZXIuZ28=) | `65.71% <100%> (ø)` | :arrow_up: |
| [pkg/installation/install.go](https://codecov.io/gh/kubernetes-sigs/krew/pull/361/diff?src=pr&el=tree#diff-cGtnL2luc3RhbGxhdGlvbi9pbnN0YWxsLmdv) | `42.42% <33.33%> (ø)` | :arrow_up: |

------

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

ahmetb: /lgtm
/approve
/hold 
have only 1 comment.
k8s-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-sigs/krew/pull/361#issuecomment-547649759" title="Approved">ahmetb</a>*, *<a href="https://github.com/kubernetes-sigs/krew/pull/361#" 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)~~ [ahmetb,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":[]} -->
k8s-ci-robot: New changes are detected. LGTM label has been removed.
corneliusweig: /hold cancel

Patch:
diff --git a/pkg/download/downloader.go b/pkg/download/downloader.go
index 19448f1..1cbf2d0 100644
--- a/pkg/download/downloader.go
+++ b/pkg/download/downloader.go
@@ -209,7 +209,7 @@ func NewDownloader(v Verifier, f Fetcher) Downloader {
 func (d Downloader) Get(uri, dst string) error {
 	body, size, err := download(uri, d.verifier, d.fetcher)
 	if err != nil {
-		return errors.Wrapf(err, "failed to get the uri %q", uri)
+		return err
 	}
 	return extractArchive(dst, body, size)
 }
diff --git a/pkg/download/fetch.go b/pkg/download/fetch.go
index 1b7ab99..e320302 100644
--- a/pkg/download/fetch.go
+++ b/pkg/download/fetch.go
@@ -37,7 +37,7 @@ type HTTPFetcher struct{}
 func (HTTPFetcher) Get(uri string) (io.ReadCloser, error) {
 	resp, err := http.Get(uri)
 	if err != nil {
-		return nil, err
+		return nil, errors.Wrapf(err, "failed to get the uri %q", uri)
 	}
 	return resp.Body, nil
 }
@@ -47,7 +47,8 @@ var _ Fetcher = fileFetcher{}
 type fileFetcher struct{ f string }
 
 func (f fileFetcher) Get(_ string) (io.ReadCloser, error) {
-	return os.Open(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.
diff --git a/pkg/download/verifier.go b/pkg/download/verifier.go
index 459e50a..1e8fb5a 100644
--- a/pkg/download/verifier.go
+++ b/pkg/download/verifier.go
@@ -20,7 +20,6 @@ import (
 	"encoding/hex"
 	"hash"
 	"io"
-	"io/ioutil"
 
 	"github.com/golang/glog"
 	"github.com/pkg/errors"
@@ -55,11 +54,3 @@ func (v sha256Verifier) Verify() error {
 	}
 	return errors.Errorf("checksum does not match, want: %x, got %x", v.wantedHash, v.Sum(nil))
 }
-
-var _ Verifier = trueVerifier{}
-
-type trueVerifier struct{ io.Writer }
-
-// NewInsecureVerifier returns a Verifier that always verifies to true.
-func NewInsecureVerifier() Verifier { return trueVerifier{ioutil.Discard} }
-func (trueVerifier) Verify() error  { return nil }
diff --git a/pkg/installation/install.go b/pkg/installation/install.go
index 1d6a29b..43692d5 100644
--- a/pkg/installation/install.go
+++ b/pkg/installation/install.go
@@ -95,7 +95,7 @@ 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)
+		return errors.Wrapf(err, "could not create staging dir %q", op.downloadStagingDir)
 	}
 	defer func() {
 		glog.V(3).Infof("Deleting the download staging directory %s", op.downloadStagingDir)
@@ -104,7 +104,7 @@ func install(op installOperation, opts InstallOpts) error {
 		}
 	}()
 	if err := downloadAndExtract(op.downloadStagingDir, op.platform.URI, op.platform.Sha256, opts.ArchiveFileOverride); err != nil {
-		return errors.Wrap(err, "failed to download and extract")
+		return errors.Wrap(err, "failed to unpack into staging dir")
 	}
 
 	applyDefaults(&op.platform)
@@ -146,7 +146,7 @@ func downloadAndExtract(extractDir, uri, sha256sum, overrideFile string) error {
 
 	verifier := download.NewSha256Verifier(sha256sum)
 	err := download.NewDownloader(verifier, fetcher).Get(uri, extractDir)
-	return errors.Wrap(err, "failed to download and verify file")
+	return errors.Wrap(err, "failed to unpack the plugin archive")
 }
 
 // Uninstall will uninstall a plugin.


Test patch:
diff --git a/pkg/download/downloader_test.go b/pkg/download/downloader_test.go
index b8960db..6b9da84 100644
--- a/pkg/download/downloader_test.go
+++ b/pkg/download/downloader_test.go
@@ -165,7 +165,7 @@ func TestDownloader_Get(t *testing.T) {
 		{
 			name: "successful get",
 			fields: fields{
-				verifier: NewInsecureVerifier(),
+				verifier: newTrueVerifier(),
 				fetcher:  NewFileFetcher(filepath.Join(testdataPath(), "test-with-directory.zip")),
 			},
 			uri:     "foo/bar/test-with-directory.zip",
@@ -174,7 +174,7 @@ func TestDownloader_Get(t *testing.T) {
 		{
 			name: "fail get by fetching",
 			fields: fields{
-				verifier: NewInsecureVerifier(),
+				verifier: newTrueVerifier(),
 				fetcher:  errorFetcher{},
 			},
 			uri:     "foo/bar/test-with-directory.zip",
@@ -217,7 +217,7 @@ func Test_download(t *testing.T) {
 			name: "successful fetch",
 			args: args{
 				url:      filePath,
-				verifier: NewInsecureVerifier(),
+				verifier: newTrueVerifier(),
 				fetcher:  NewFileFetcher(filePath),
 			},
 			wantReader: bytes.NewReader(downloadOriginal),
@@ -267,6 +267,14 @@ func Test_download(t *testing.T) {
 	}
 }
 
+var _ Verifier = trueVerifier{}
+
+type trueVerifier struct{ io.Writer }
+
+// newTrueVerifier returns a Verifier that always verifies to true.
+func newTrueVerifier() Verifier    { return trueVerifier{ioutil.Discard} }
+func (trueVerifier) Verify() error { return nil }
+
 var _ Verifier = falseVerifier{}
 
 type falseVerifier struct{ io.Writer }
diff --git a/pkg/download/verifier_test.go b/pkg/download/verifier_test.go
index 6b9a82f..c978435 100644
--- a/pkg/download/verifier_test.go
+++ b/pkg/download/verifier_test.go
@@ -58,37 +58,3 @@ func TestSha256Verifier(t *testing.T) {
 		})
 	}
 }
-
-func TestTrueVerifier(t *testing.T) {
-	tests := []struct {
-		name      string
-		write     []byte
-		wantError bool
-	}{
-		{
-			name:      "test okay hash",
-			write:     []byte("hello world"),
-			wantError: false,
-		},
-		{
-			name:      "test wrong hash",
-			write:     []byte("HELLO WORLD"),
-			wantError: false,
-		},
-		{
-			name:      "test empty hash",
-			write:     []byte{},
-			wantError: false,
-		},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			v := NewInsecureVerifier()
-			_, _ = io.Copy(v, bytes.NewReader(tt.write))
-			if err := v.Verify(); (err != nil) != tt.wantError {
-				t.Errorf("NewInsecureVerifier().Write(%x).Verify() = %v, wantReader %v", tt.write, err, tt.wantError)
-				return
-			}
-		})
-	}
-}