Repository: kubernetes-sigs/krew
Pull request #512 (2020-02-25 04:34:37)

Initial receipt type
This refactors the receipts to use a new `Receipt` type. The tests for it are pretty much identical to the old ones but using the new type. Once the status object gets added to receipts the tests will need to be updated.

I decided to not do `type Receipt Plugin`, only because I figured this would be changed very soon and would save a bit of work down the line. If you'd like me to change it and start out without the embedded type just let me know.

Related issues: #483, #507 
<!-- For proposed features, make sure there's an issue it's discussed first -->


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

This pull-request has been approved by: *<a href="https://github.com/kubernetes-sigs/krew/pull/512#" title="Author self-approved">chriskim06</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **soltysh**
You can assign the PR to them by writing `/assign @soltysh` in a comment when ready.

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

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

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

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":["soltysh"]} -->
codecov-io: # [Codecov](https://codecov.io/gh/kubernetes-sigs/krew/pull/512?src=pr&el=h1) Report
> Merging [#512](https://codecov.io/gh/kubernetes-sigs/krew/pull/512?src=pr&el=desc) into [master](https://codecov.io/gh/kubernetes-sigs/krew/commit/4f1339fc3cce3917cc8323b1b6ebe583d583349d?src=pr&el=desc) will **increase** coverage by `0.72%`.
> The diff coverage is `80.43%`.

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

```diff
@@            Coverage Diff             @@
##           master     #512      +/-   ##
==========================================
+ Coverage   59.12%   59.84%   +0.72%     
==========================================
  Files          23       23              
  Lines        1003     1041      +38     
==========================================
+ Hits          593      623      +30     
- Misses        350      354       +4     
- Partials       60       64       +4
```


| [Impacted Files](https://codecov.io/gh/kubernetes-sigs/krew/pull/512?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [internal/installation/receipt/receipt.go](https://codecov.io/gh/kubernetes-sigs/krew/pull/512/diff?src=pr&el=tree#diff-aW50ZXJuYWwvaW5zdGFsbGF0aW9uL3JlY2VpcHQvcmVjZWlwdC5nbw==) | `75% <100%> (ø)` | :arrow_up: |
| [internal/index/indexscanner/scanner.go](https://codecov.io/gh/kubernetes-sigs/krew/pull/512/diff?src=pr&el=tree#diff-aW50ZXJuYWwvaW5kZXgvaW5kZXhzY2FubmVyL3NjYW5uZXIuZ28=) | `69.69% <66.66%> (-2.86%)` | :arrow_down: |
| [internal/index/validation/validate.go](https://codecov.io/gh/kubernetes-sigs/krew/pull/512/diff?src=pr&el=tree#diff-aW50ZXJuYWwvaW5kZXgvdmFsaWRhdGlvbi92YWxpZGF0ZS5nbw==) | `91.91% <91.3%> (-0.19%)` | :arrow_down: |

------

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

corneliusweig: Interesting. Travis builds seem to be broken right now. I guess we either need to disable travis or this test for now :)
ahmetb: @corneliusweig do you mind Making the error more Verbose in  Test_fetchLatestTag_GitHubAPI? I suspect it’s hitting API rate limits of travis servers. 
ahmetb: We can go ahead with this if you merge the small outstanding style comments. 
corneliusweig: /lgtm
/approve
Thanks, @chriskim06, for bearing with us. It always takes some time to get used to the coding style of a new project. But we're getting there! And having two reviewers is also challenging :)
k8s-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-sigs/krew/pull/512#" title="Author self-approved">chriskim06</a>*, *<a href="https://github.com/kubernetes-sigs/krew/pull/512#issuecomment-592164752" title="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":[]} -->
k8s-ci-robot: New changes are detected. LGTM label has been removed.
corneliusweig: /lgtm
:confetti_ball: 

Patch:
diff --git a/internal/index/indexscanner/scanner.go b/internal/index/indexscanner/scanner.go
index e8cec59..58684e4 100644
--- a/internal/index/indexscanner/scanner.go
+++ b/internal/index/indexscanner/scanner.go
@@ -86,31 +86,47 @@ func LoadPluginByName(pluginsDir, pluginName string) (index.Plugin, error) {
 // ReadPluginFromFile loads a file from the FS. When plugin file not found, it
 // returns an error that can be checked with os.IsNotExist.
 func ReadPluginFromFile(path string) (index.Plugin, error) {
-	f, err := os.Open(path)
-	if os.IsNotExist(err) {
-		// TODO(ahmetb): we should use go1.13+ errors.Is construct at call sites to evaluate if an error is os.IsNotExist
-		return index.Plugin{}, err
-	} else if err != nil {
-		return index.Plugin{}, errors.Wrap(err, "failed to open index file")
+	var plugin index.Plugin
+	err := readFromFile(path, &plugin)
+	if err != nil {
+		return plugin, err
 	}
-	return ReadPlugin(f)
+	return plugin, errors.Wrap(validation.ValidatePlugin(plugin.Name, plugin), "plugin manifest validation error")
 }
 
 func ReadPlugin(f io.ReadCloser) (index.Plugin, error) {
-	defer f.Close()
-	p, err := DecodePluginFile(f)
+	var plugin index.Plugin
+	err := decodeFile(f, &plugin)
 	if err != nil {
-		return p, errors.Wrap(err, "failed to decode plugin manifest")
+		return plugin, errors.Wrap(err, "failed to decode plugin manifest")
 	}
-	return p, errors.Wrap(validation.ValidatePlugin(p.Name, p), "plugin manifest validation error")
+	return plugin, errors.Wrap(validation.ValidatePlugin(plugin.Name, plugin), "plugin manifest validation error")
 }
 
-// DecodePluginFile tries to decodes a plugin manifest from r.
-func DecodePluginFile(r io.Reader) (index.Plugin, error) {
-	var plugin index.Plugin
+// ReadReceiptFromFile loads a file from the FS. When receipt file not found, it
+// returns an error that can be checked with os.IsNotExist.
+func ReadReceiptFromFile(path string) (index.Receipt, error) {
+	var receipt index.Receipt
+	err := readFromFile(path, &receipt)
+	return receipt, err
+}
+
+// readFromFile loads a file from the FS into the provided object.
+func readFromFile(path string, as interface{}) error {
+	f, err := os.Open(path)
+	if err != nil {
+		return err
+	}
+	err = decodeFile(f, &as)
+	return errors.Wrap(err, "failed to parse yaml file")
+}
+
+// decodeFile tries to decode a plugin/receipt
+func decodeFile(r io.ReadCloser, as interface{}) error {
+	defer r.Close()
 	b, err := ioutil.ReadAll(r)
 	if err != nil {
-		return plugin, err
+		return err
 	}
 
 	// TODO(ahmetb): when we have a stable API that won't add new fields,
@@ -118,6 +134,5 @@ func DecodePluginFile(r io.Reader) (index.Plugin, error) {
 	// incremental field additions to plugin manifests independently from the
 	// installed version of krew.
 	// yaml.UnmarshalStrict()
-	err = yaml.Unmarshal(b, &plugin)
-	return plugin, err
+	return yaml.Unmarshal(b, &as)
 }
diff --git a/internal/installation/receipt/receipt.go b/internal/installation/receipt/receipt.go
index e4495cf..6ebd5a3 100644
--- a/internal/installation/receipt/receipt.go
+++ b/internal/installation/receipt/receipt.go
@@ -24,7 +24,7 @@ import (
 	"sigs.k8s.io/krew/pkg/index"
 )
 
-// Store saves the given plugin at the destination.
+// Store saves the given plugin receipt at the destination.
 // The caller has to ensure that the destination directory exists.
 func Store(plugin index.Plugin, dest string) error {
 	yamlBytes, err := yaml.Marshal(plugin)
@@ -38,6 +38,6 @@ func Store(plugin index.Plugin, dest string) error {
 
 // Load reads the plugin receipt at the specified destination.
 // If not found, it returns os.IsNotExist error.
-func Load(path string) (index.Plugin, error) {
-	return indexscanner.ReadPluginFromFile(path)
+func Load(path string) (index.Receipt, error) {
+	return indexscanner.ReadReceiptFromFile(path)
 }
diff --git a/pkg/index/types.go b/pkg/index/types.go
index b326c17..791d946 100644
--- a/pkg/index/types.go
+++ b/pkg/index/types.go
@@ -58,3 +58,8 @@ type FileOperation struct {
 	From string `json:"from,omitempty"`
 	To   string `json:"to,omitempty"`
 }
+
+// Receipt describes a plugin receipt file.
+type Receipt struct {
+	Plugin `json:",inline" yaml:",inline"`
+}


Test patch:
diff --git a/internal/installation/receipt/receipt_test.go b/internal/installation/receipt/receipt_test.go
index f0adddc..658ebf1 100644
--- a/internal/installation/receipt/receipt_test.go
+++ b/internal/installation/receipt/receipt_test.go
@@ -22,6 +22,7 @@ import (
 
 	"sigs.k8s.io/krew/internal/index/indexscanner"
 	"sigs.k8s.io/krew/internal/testutil"
+	"sigs.k8s.io/krew/pkg/index"
 )
 
 func TestStore(t *testing.T) {
@@ -58,7 +59,8 @@ func TestLoad(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	if diff := cmp.Diff(&gotPlugin, &testPlugin); diff != "" {
+	testPluginReceipt := index.Receipt{Plugin: testPlugin}
+	if diff := cmp.Diff(&gotPlugin, &testPluginReceipt); diff != "" {
 		t.Fatal(diff)
 	}
 }
diff --git a/internal/installation/util_test.go b/internal/installation/util_test.go
index 4b3d490..7d6bd4d 100644
--- a/internal/installation/util_test.go
+++ b/internal/installation/util_test.go
@@ -65,7 +65,7 @@ func TestListInstalledPlugins(t *testing.T) {
 			defer cleanup()
 
 			for _, plugin := range test.plugins {
-				tempDir.WritePlugin(plugin.Name+constants.ManifestExtension, plugin)
+				tempDir.WriteYAML(plugin.Name+constants.ManifestExtension, plugin)
 			}
 
 			actual, err := ListInstalledPlugins(tempDir.Root())
diff --git a/internal/testutil/tempdir.go b/internal/testutil/tempdir.go
index 92ef417..9b28fee 100644
--- a/internal/testutil/tempdir.go
+++ b/internal/testutil/tempdir.go
@@ -23,8 +23,6 @@ import (
 	"testing"
 
 	"sigs.k8s.io/yaml"
-
-	"sigs.k8s.io/krew/pkg/index"
 )
 
 type TempDir struct {
@@ -77,11 +75,11 @@ func (td *TempDir) Write(file string, content []byte) *TempDir {
 	return td
 }
 
-func (td *TempDir) WritePlugin(file string, plugin index.Plugin) *TempDir {
+func (td *TempDir) WriteYAML(file string, obj interface{}) *TempDir {
 	td.t.Helper()
-	content, err := yaml.Marshal(plugin)
+	content, err := yaml.Marshal(obj)
 	if err != nil {
-		td.t.Fatalf("cannot marshal plugin: %s", err)
+		td.t.Fatalf("cannot marshal obj: %s", err)
 	}
 	return td.Write(file, content)
 }