Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial receipt type #512

Merged
merged 6 commits into from
Feb 27, 2020
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 37 additions & 16 deletions internal/index/indexscanner/scanner.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,38 +86,59 @@ 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")
corneliusweig marked this conversation as resolved.
Show resolved Hide resolved
}

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
ahmetb marked this conversation as resolved.
Show resolved Hide resolved
}

// readFromFile loads a file from the FS into the provided object.
func readFromFile(path string, as interface{}) error {
f, err := os.Open(path)
if os.IsNotExist(err) {
return err
} else if err != nil {
return errors.Wrapf(err, "failed to open file %s", path)
corneliusweig marked this conversation as resolved.
Show resolved Hide resolved
}
defer f.Close()
err = decodeFile(f, &as)
if err != nil {
corneliusweig marked this conversation as resolved.
Show resolved Hide resolved
return errors.Wrap(err, "failed to decode plugin manifest")
corneliusweig marked this conversation as resolved.
Show resolved Hide resolved
}
return nil
}

// decodeFile tries to decode a plugin/receipt
func decodeFile(r io.Reader, as interface{}) error {
corneliusweig marked this conversation as resolved.
Show resolved Hide resolved
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,
// we can consider failing on unknown fields. Currently, disabling due to
// 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)
}
53 changes: 53 additions & 0 deletions internal/index/indexscanner/scanner_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,11 @@ import (
"path/filepath"
"testing"

"github.com/google/go-cmp/cmp"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"

"sigs.k8s.io/krew/internal/testutil"
)

func TestReadPluginFile(t *testing.T) {
Expand Down Expand Up @@ -90,6 +93,56 @@ func TestReadPluginFile(t *testing.T) {
}
}

func TestReadReceiptFile(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not particularly happy with the test below, as it exercises only a few things. For example, it doesn't check whether the status fields are parsed correctly.

We just need 1 test case that makes sure the fields are parsed decently. and the json:"foo,omitempty" field-tags are accurate.

So I suggest we do this:

  1. Add a few receipt test files in testdata/receipts/foo.yaml

  2. Parse the receipt file from disk.

  3. Build as NewReceipt().WithXxx(...)....V() and use cmp.Diff to do equality check.

@corneliusweig wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, so far Receipt does not have any extra fields beside the ones from Plugin. So we are cutting corners by parsing a plugin yaml as a receipt. I think that's ok for now, but we need to ensure that we extend this test as suggested by Ahmet.

And I'm in for the receipt builder pattern. The sooner we have it, the better.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah in the next PR, we should add the status field and start working with it to develop tests like this (where we can roundtrip the type i.e. write/parse a YAML file).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So is this ok as is for now while status doesn't exist?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah not so useful but helps us pass the tests.
feel free to remove it altogether or add TODO that captures comment above, but this should be the next PR anyway.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I'll just remove it from this PR and add the tests in the next one when they can be more meaningful with the new status field.

tests := []struct {
corneliusweig marked this conversation as resolved.
Show resolved Hide resolved
name string
receiptFileName string
wantErr bool
matchFirst labels.Set
}{
{
name: "read receipt file",
receiptFileName: "foo.yaml",
wantErr: false,
matchFirst: labels.Set{
"arch": "amd64",
"os": "linux",
},
},
}
neverMatch := labels.Set{}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
tmpDir, cleanup := testutil.NewTempDir(t)
defer cleanup()

receipt := testutil.NewPlugin().WithName("foo").V()
receiptFile := filepath.Join(tmpDir.Root(), "receipts", tt.receiptFileName)
corneliusweig marked this conversation as resolved.
Show resolved Hide resolved
tmpDir.WriteYaml(receiptFile, receipt)

got, err := ReadReceiptFromFile(receiptFile)
if (err != nil) != tt.wantErr {
t.Errorf("ReadReceiptFromFile() error = %v, wantErr %v", err, tt.wantErr)
}
if err != nil {
t.Error(err)
}
if cmp.Diff(got.Plugin, receipt) != "" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cmp.Diff already formats the differences in an easy to read way. You should do

if diff := cmp.Diff(got.Plugin, receipt); diff != "" {
   t.Errorf("Parsed receipt has unexpected content: %s", diff)
}

t.Errorf("ReadReceiptFromFile() has not parsed the receipt properly: %+v", got)
}

sel, err := metav1.LabelSelectorAsSelector(got.Spec.Platforms[0].Selector)
if err != nil {
t.Errorf("ReadReceiptFromFile() error parsing label err: %v", err)
}
if !sel.Matches(tt.matchFirst) || sel.Matches(neverMatch) {
t.Errorf("ReadReceiptFromFile() didn't parse label selector properly: %##v", sel)
}
})
}
}

func TestReadPluginFile_preservesNotFoundErr(t *testing.T) {
_, err := ReadPluginFromFile(filepath.Join(testdataPath(t), "does-not-exist.yaml"))
if err == nil {
Expand Down
6 changes: 3 additions & 3 deletions internal/installation/receipt/receipt.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
}
4 changes: 3 additions & 1 deletion internal/installation/receipt/receipt_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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)
}
}
Expand Down
2 changes: 1 addition & 1 deletion internal/installation/util_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand Down
8 changes: 3 additions & 5 deletions internal/testutil/tempdir.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@ import (
"testing"

"sigs.k8s.io/yaml"

"sigs.k8s.io/krew/pkg/index"
)

type TempDir struct {
Expand Down Expand Up @@ -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 {
corneliusweig marked this conversation as resolved.
Show resolved Hide resolved
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)
}
5 changes: 5 additions & 0 deletions pkg/index/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,3 +58,8 @@ type FileOperation struct {
From string `json:"from,omitempty"`
To string `json:"to,omitempty"`
}

// Receipt describes a plugin receipt file.
corneliusweig marked this conversation as resolved.
Show resolved Hide resolved
type Receipt struct {
Plugin `json:",inline" yaml:",inline"`
}