Repository: kubernetes-sigs/krew
Pull request #555 (2020-03-21 19:41:48)

Populate receipt status on install/upgrade
Related issue: #483 
<!-- 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/555#" 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"]} -->
ahmetb: Right now we lack any integration tests for the changes.

Note that you're changing criticial path in this PR. So we need to make sure newly added behavior works. We can merge, but the very next PR we need would be the integration tests.

Some basic cases I'm thinking easily:

- add index + install with its prefix
- install plugins with `default/` prefix
chriskim06: I can add the integration tests here or in a separate one if that'll make it easier. Let me know which one you'd prefer.
ahmetb: > I can add the integration tests here or in a separate one if that'll make it easier. Let me know which one you'd prefer.

I'm curious how you tested this currently. I'm trying to get it to work locally and tried a fake local index at a directory, and wasn't able to install. So the integration tests can actually verify it works.
chriskim06: That's strange. I pretty much did what you mentioned and what's in the integration test: add index then install and it worked.
ahmetb: /lgtm
/approve

nice and small!
k8s-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-sigs/krew/pull/555#issuecomment-604832987" title="Approved">ahmetb</a>*, *<a href="https://github.com/kubernetes-sigs/krew/pull/555#" title="Author self-approved">chriskim06</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]

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":[]} -->

Patch:
diff --git a/cmd/krew/cmd/install.go b/cmd/krew/cmd/install.go
index 4f5f2af..4dd1e66 100644
--- a/cmd/krew/cmd/install.go
+++ b/cmd/krew/cmd/install.go
@@ -27,10 +27,16 @@ import (
 	"sigs.k8s.io/krew/cmd/krew/cmd/internal"
 	"sigs.k8s.io/krew/internal/index/indexscanner"
 	"sigs.k8s.io/krew/internal/installation"
+	"sigs.k8s.io/krew/internal/pathutil"
 	"sigs.k8s.io/krew/pkg/constants"
 	"sigs.k8s.io/krew/pkg/index"
 )
 
+type pluginEntry struct {
+	p         index.Plugin
+	indexName string
+}
+
 func init() {
 	var (
 		manifest, manifestURL, archiveFileOverride *string
@@ -90,16 +96,20 @@ Remarks:
 				return errors.New("--archive can be specified only with --manifest or --manifest-url")
 			}
 
-			var install []index.Plugin
+			var install []pluginEntry
 			for _, name := range pluginNames {
-				plugin, err := indexscanner.LoadPluginByName(paths.IndexPluginsPath(constants.DefaultIndexName), name)
+				indexName, pluginName := pathutil.CanonicalPluginName(name)
+				plugin, err := indexscanner.LoadPluginByName(paths.IndexPluginsPath(indexName), pluginName)
 				if err != nil {
 					if os.IsNotExist(err) {
 						return errors.Errorf("plugin %q does not exist in the plugin index", name)
 					}
 					return errors.Wrapf(err, "failed to load plugin %q from the index", name)
 				}
-				install = append(install, plugin)
+				install = append(install, pluginEntry{
+					p:         plugin,
+					indexName: indexName,
+				})
 			}
 
 			if *manifest != "" {
@@ -107,28 +117,35 @@ Remarks:
 				if err != nil {
 					return errors.Wrap(err, "failed to load plugin manifest from file")
 				}
-				install = append(install, plugin)
+				install = append(install, pluginEntry{
+					p:         plugin,
+					indexName: constants.DefaultIndexName,
+				})
 			} else if *manifestURL != "" {
 				plugin, err := readPluginFromURL(*manifestURL)
 				if err != nil {
 					return errors.Wrap(err, "failed to read plugin manifest file from url")
 				}
-				install = append(install, plugin)
+				install = append(install, pluginEntry{
+					p:         plugin,
+					indexName: constants.DefaultIndexName,
+				})
 			}
 
 			if len(install) == 0 {
 				return cmd.Help()
 			}
 
-			for _, plugin := range install {
-				klog.V(2).Infof("Will install plugin: %s\n", plugin.Name)
+			for _, pluginEntry := range install {
+				klog.V(2).Infof("Will install plugin: %s/%s\n", pluginEntry.indexName, pluginEntry.p.Name)
 			}
 
 			var failed []string
 			var returnErr error
-			for _, plugin := range install {
+			for _, entry := range install {
+				plugin := entry.p
 				fmt.Fprintf(os.Stderr, "Installing plugin: %s\n", plugin.Name)
-				err := installation.Install(paths, plugin, installation.InstallOpts{
+				err := installation.Install(paths, plugin, entry.indexName, installation.InstallOpts{
 					ArchiveFileOverride: *archiveFileOverride,
 				})
 				if err == installation.ErrIsAlreadyInstalled {
diff --git a/cmd/krew/cmd/upgrade.go b/cmd/krew/cmd/upgrade.go
index 74dbbd7..8df9806 100644
--- a/cmd/krew/cmd/upgrade.go
+++ b/cmd/krew/cmd/upgrade.go
@@ -25,7 +25,7 @@ import (
 	"sigs.k8s.io/krew/cmd/krew/cmd/internal"
 	"sigs.k8s.io/krew/internal/index/indexscanner"
 	"sigs.k8s.io/krew/internal/installation"
-	"sigs.k8s.io/krew/pkg/constants"
+	"sigs.k8s.io/krew/internal/pathutil"
 )
 
 func init() {
@@ -63,7 +63,8 @@ kubectl krew upgrade foo bar"`,
 
 			var nErrors int
 			for _, name := range pluginNames {
-				plugin, err := indexscanner.LoadPluginByName(paths.IndexPluginsPath(constants.DefaultIndexName), name)
+				indexName, pluginName := pathutil.CanonicalPluginName(name)
+				plugin, err := indexscanner.LoadPluginByName(paths.IndexPluginsPath(indexName), pluginName)
 				if err != nil {
 					if !os.IsNotExist(err) {
 						return errors.Wrapf(err, "failed to load the plugin manifest for plugin %s", name)
@@ -74,7 +75,7 @@ kubectl krew upgrade foo bar"`,
 
 				if err == nil {
 					fmt.Fprintf(os.Stderr, "Upgrading plugin: %s\n", name)
-					err = installation.Upgrade(paths, plugin)
+					err = installation.Upgrade(paths, plugin, indexName)
 					if ignoreUpgraded && err == installation.ErrIsAlreadyUpgraded {
 						fmt.Fprintf(os.Stderr, "Skipping plugin %s, it is already on the newest version\n", name)
 						continue
diff --git a/internal/installation/install.go b/internal/installation/install.go
index 4d7ab8c..c7c6197 100644
--- a/internal/installation/install.go
+++ b/internal/installation/install.go
@@ -45,6 +45,12 @@ type installOperation struct {
 	binDir     string
 }
 
+// PluginEntry describes a plugin and the index it comes from.
+type PluginEntry struct {
+	Plugin    index.Plugin
+	IndexName string
+}
+
 // Plugin lifecycle errors
 var (
 	ErrIsAlreadyInstalled = errors.New("can't install, the newest version is already installed")
@@ -54,7 +60,7 @@ var (
 
 // Install will download and install a plugin. The operation tries
 // to not get the plugin dir in a bad state if it fails during the process.
-func Install(p environment.Paths, plugin index.Plugin, opts InstallOpts) error {
+func Install(p environment.Paths, plugin index.Plugin, indexName string, opts InstallOpts) error {
 	klog.V(2).Infof("Looking for installed versions")
 	_, err := receipt.Load(p.PluginInstallReceiptPath(plugin.Name))
 	if err == nil {
@@ -85,7 +91,7 @@ func Install(p environment.Paths, plugin index.Plugin, opts InstallOpts) error {
 		return errors.Wrap(err, "install failed")
 	}
 	klog.V(3).Infof("Storing install receipt for plugin %s", plugin.Name)
-	err = receipt.Store(plugin, p.PluginInstallReceiptPath(plugin.Name))
+	err = receipt.Store(receipt.New(plugin, indexName), p.PluginInstallReceiptPath(plugin.Name))
 	return errors.Wrap(err, "installation receipt could not be stored, uninstall may fail")
 }
 
diff --git a/internal/installation/receipt/receipt.go b/internal/installation/receipt/receipt.go
index 6ebd5a3..83a2735 100644
--- a/internal/installation/receipt/receipt.go
+++ b/internal/installation/receipt/receipt.go
@@ -24,10 +24,10 @@ import (
 	"sigs.k8s.io/krew/pkg/index"
 )
 
-// Store saves the given plugin receipt at the destination.
+// Store saves the given 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)
+func Store(receipt index.Receipt, dest string) error {
+	yamlBytes, err := yaml.Marshal(receipt)
 	if err != nil {
 		return errors.Wrapf(err, "convert to yaml")
 	}
@@ -41,3 +41,15 @@ func Store(plugin index.Plugin, dest string) error {
 func Load(path string) (index.Receipt, error) {
 	return indexscanner.ReadReceiptFromFile(path)
 }
+
+// New returns a new receipt with the given plugin and index name.
+func New(plugin index.Plugin, indexName string) index.Receipt {
+	return index.Receipt{
+		Plugin: plugin,
+		Status: index.ReceiptStatus{
+			Source: index.SourceIndex{
+				Name: indexName,
+			},
+		},
+	}
+}
diff --git a/internal/installation/upgrade.go b/internal/installation/upgrade.go
index 6d18426..bac6d32 100644
--- a/internal/installation/upgrade.go
+++ b/internal/installation/upgrade.go
@@ -29,7 +29,7 @@ import (
 
 // Upgrade will reinstall and delete the old plugin. The operation tries
 // to not get the plugin dir in a bad state if it fails during the process.
-func Upgrade(p environment.Paths, plugin index.Plugin) error {
+func Upgrade(p environment.Paths, plugin index.Plugin, indexName string) error {
 	installReceipt, err := receipt.Load(p.PluginInstallReceiptPath(plugin.Name))
 	if err != nil {
 		return errors.Wrapf(err, "failed to load install receipt for plugin %q", plugin.Name)
@@ -78,7 +78,7 @@ func Upgrade(p environment.Paths, plugin index.Plugin) error {
 	}
 
 	klog.V(2).Infof("Upgrading install receipt for plugin %s", plugin.Name)
-	if err = receipt.Store(plugin, p.PluginInstallReceiptPath(plugin.Name)); err != nil {
+	if err = receipt.Store(receipt.New(plugin, indexName), p.PluginInstallReceiptPath(plugin.Name)); err != nil {
 		return errors.Wrap(err, "installation receipt could not be stored, uninstall may fail")
 	}
 


Test patch:
diff --git a/integration_test/install_test.go b/integration_test/install_test.go
index 21c7589..06d9b9f 100644
--- a/integration_test/install_test.go
+++ b/integration_test/install_test.go
@@ -95,6 +95,33 @@ func TestKrewInstall_StdinAndPositionalArguments(t *testing.T) {
 	test.AssertExecutableNotInPATH("kubectl-" + validPlugin2)
 }
 
+func TestKrewInstall_ExplicitDefaultIndex(t *testing.T) {
+	skipShort(t)
+
+	test, cleanup := NewTest(t)
+	defer cleanup()
+
+	test.Krew("install", "default/"+validPlugin).RunOrFail()
+	test.AssertExecutableInPATH("kubectl-" + validPlugin)
+}
+
+func TestKrewInstall_CustomIndex(t *testing.T) {
+	skipShort(t)
+
+	test, cleanup := NewTest(t)
+	defer cleanup()
+
+	test.WithEnv(constants.EnableMultiIndexSwitch, 1).WithIndex()
+	test.Krew("index", "add", "foo", test.TempDir().Path("index/"+constants.DefaultIndexName)).RunOrFail()
+	test.Krew("install", "foo/"+validPlugin).RunOrFail()
+	test.AssertExecutableInPATH("kubectl-" + validPlugin)
+
+	if err := test.Krew("install", "invalid/"+validPlugin2).Run(); err == nil {
+		t.Fatal("expected install from invalid index to fail")
+	}
+	test.AssertExecutableNotInPATH("kubectl-" + validPlugin2)
+}
+
 func TestKrewInstall_Manifest(t *testing.T) {
 	skipShort(t)
 
diff --git a/internal/installation/receipt/receipt_test.go b/internal/installation/receipt/receipt_test.go
index 01f98da..6c44bae 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/constants"
 )
 
 func TestStore(t *testing.T) {
@@ -32,7 +33,7 @@ func TestStore(t *testing.T) {
 	testReceipt := testutil.NewReceipt().WithPlugin(testPlugin).V()
 	dest := tmpDir.Path("some-plugin.yaml")
 
-	if err := Store(testPlugin, dest); err != nil {
+	if err := Store(testReceipt, dest); err != nil {
 		t.Fatal(err)
 	}
 
@@ -51,7 +52,8 @@ func TestLoad(t *testing.T) {
 	defer cleanup()
 
 	testPlugin := testutil.NewPlugin().WithName("foo").WithPlatforms(testutil.NewPlatform().V()).V()
-	if err := Store(testPlugin, tmpDir.Path("foo.yaml")); err != nil {
+	testPluginReceipt := testutil.NewReceipt().WithPlugin(testPlugin).V()
+	if err := Store(testPluginReceipt, tmpDir.Path("foo.yaml")); err != nil {
 		t.Fatal(err)
 	}
 
@@ -59,7 +61,6 @@ func TestLoad(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	testPluginReceipt := testutil.NewReceipt().WithPlugin(testPlugin).V()
 	if diff := cmp.Diff(&gotPlugin, &testPluginReceipt); diff != "" {
 		t.Fatal(diff)
 	}
@@ -71,3 +72,13 @@ func TestLoad_preservesNonExistsError(t *testing.T) {
 		t.Fatalf("returned error is not ENOENT: %+v", err)
 	}
 }
+
+func TestNew(t *testing.T) {
+	testPlugin := testutil.NewPlugin().WithName("foo").WithPlatforms(testutil.NewPlatform().V()).V()
+	wantReceipt := testutil.NewReceipt().WithPlugin(testPlugin).V()
+
+	gotReceipt := New(testPlugin, constants.DefaultIndexName)
+	if diff := cmp.Diff(gotReceipt, wantReceipt); diff != "" {
+		t.Fatalf("expected receipts to match: %s", diff)
+	}
+}