cmd: Update each index instead of just default (#588)

* Update each index instead of just default * Fix linting issue * Split into two tests * Code review changes * Wrap error in search * Add integration tests Switch back to sequential updates. The test where gitutil.EnsureUpdated fails seems to hang when it happens in a goroutine for some reason. * Code review changes * Code review changes
kubernetes-sigs · Apr 13, 2020 · c99749f · c99749f
1 parent b2b8a40
commit c99749f
Show file tree

Hide file tree

Showing 10 changed files with 106 additions and 42 deletions.
diff --git a/cmd/krew/cmd/install.go b/cmd/krew/cmd/install.go
@@ -189,7 +189,7 @@ Remarks:
 				klog.V(4).Infof("--no-update-index specified, skipping updating local copy of plugin index")
 				return nil
 			}
-			return ensureIndexUpdated(cmd, args)
+			return ensureIndexesUpdated(cmd, args)
 		},
 	}
 

diff --git a/cmd/krew/cmd/search.go b/cmd/krew/cmd/search.go
@@ -27,7 +27,6 @@ import (
 	"sigs.k8s.io/krew/internal/index/indexoperations"
 	"sigs.k8s.io/krew/internal/index/indexscanner"
 	"sigs.k8s.io/krew/internal/installation"
-	"sigs.k8s.io/krew/pkg/constants"
 )
 
 // searchCmd represents the search command
@@ -44,18 +43,9 @@ Examples:
   To fuzzy search plugins with a keyword:
     kubectl krew search KEYWORD`,
 	RunE: func(cmd *cobra.Command, args []string) error {
-		indexes := []indexoperations.Index{
-			{
-				Name: constants.DefaultIndexName,
-				URL:  constants.DefaultIndexURI, // unused here but providing for completeness
-			},
-		}
-		if os.Getenv(constants.EnableMultiIndexSwitch) != "" {
-			out, err := indexoperations.ListIndexes(paths)
-			if err != nil {
-				return errors.Wrapf(err, "failed to list plugin indexes available")
-			}
-			indexes = out
+		indexes, err := indexoperations.ListIndexes(paths)
+		if err != nil {
+			return errors.Wrap(err, "failed to list indexes")
 		}
 
 		klog.V(3).Infof("found %d indexes", len(indexes))

diff --git a/cmd/krew/cmd/system.go b/cmd/krew/cmd/system.go
@@ -51,7 +51,7 @@ This command will be removed without further notice from future versions of krew
 	RunE: func(cmd *cobra.Command, args []string) error {
 		return receiptsmigration.Migrate(paths)
 	},
-	PreRunE: ensureIndexUpdated,
+	PreRunE: ensureIndexesUpdated,
 }
 
 var indexUpgradeCmd = &cobra.Command{

diff --git a/cmd/krew/cmd/update.go b/cmd/krew/cmd/update.go
@@ -19,12 +19,14 @@ import (
 	"fmt"
 	"io"
 	"os"
+	"strings"
 
 	"github.com/pkg/errors"
 	"github.com/spf13/cobra"
 	"k8s.io/klog"
 
 	"sigs.k8s.io/krew/internal/gitutil"
+	"sigs.k8s.io/krew/internal/index/indexoperations"
 	"sigs.k8s.io/krew/internal/index/indexscanner"
 	"sigs.k8s.io/krew/internal/installation"
 	"sigs.k8s.io/krew/pkg/constants"
@@ -43,7 +45,7 @@ plugin index from the internet.
 Remarks:
   You don't need to run this command: Running "krew update" or "krew upgrade"
   will silently run this command.`,
-	RunE: ensureIndexUpdated,
+	RunE: ensureIndexesUpdated,
 }
 
 func showFormattedPluginsInfo(out io.Writer, header string, plugins []string) {
@@ -102,13 +104,28 @@ func showUpdatedPlugins(out io.Writer, preUpdate, posUpdate []index.Plugin, inst
 	}
 }
 
-func ensureIndexUpdated(_ *cobra.Command, _ []string) error {
+func ensureIndexesUpdated(_ *cobra.Command, _ []string) error {
 	preUpdateIndex, _ := indexscanner.LoadPluginListFromFS(paths.IndexPluginsPath(constants.DefaultIndexName))
 
-	klog.V(1).Infof("Updating the local copy of plugin index (%s)", paths.IndexPath(constants.DefaultIndexName))
-	if err := gitutil.EnsureUpdated(constants.DefaultIndexURI, paths.IndexPath(constants.DefaultIndexName)); err != nil {
-		return errors.Wrap(err, "failed to update the local index")
+	indexes, err := indexoperations.ListIndexes(paths)
+	if err != nil {
+		return errors.Wrap(err, "failed to list indexes")
 	}
+
+	var failed []string
+	var returnErr error
+	for _, idx := range indexes {
+		indexPath := paths.IndexPath(idx.Name)
+		klog.V(1).Infof("Updating the local copy of plugin index (%s)", indexPath)
+		if err := gitutil.EnsureUpdated(idx.URL, indexPath); err != nil {
+			klog.Warningf("failed to update index %q: %v", idx.Name, err)
+			failed = append(failed, idx.Name)
+			if returnErr == nil {
+				returnErr = err
+			}
+		}
+	}
+
 	fmt.Fprintln(os.Stderr, "Updated the local copy of plugin index.")
 
 	if len(preUpdateIndex) == 0 {
@@ -132,7 +149,7 @@ func ensureIndexUpdated(_ *cobra.Command, _ []string) error {
 	// TODO(chriskim06) consider commenting this out when refactoring for custom indexes
 	showUpdatedPlugins(os.Stderr, preUpdateIndex, posUpdateIndex, installedPlugins)
 
-	return nil
+	return errors.Wrapf(returnErr, "failed to update the following indexes: %s\n", strings.Join(failed, ", "))
 }
 
 func init() {

diff --git a/cmd/krew/cmd/upgrade.go b/cmd/krew/cmd/upgrade.go
@@ -106,7 +106,7 @@ kubectl krew upgrade foo bar"`,
 				klog.V(4).Infof("--no-update-index specified, skipping updating local copy of plugin index")
 				return nil
 			}
-			return ensureIndexUpdated(cmd, args)
+			return ensureIndexesUpdated(cmd, args)
 		},
 	}
 

diff --git a/integration_test/index_test.go b/integration_test/index_test.go
@@ -150,7 +150,7 @@ func TestKrewIndexRemove_unsafe(t *testing.T) {
 func TestKrewIndexRemoveFailsWhenPluginsInstalled(t *testing.T) {
 	skipShort(t)
 	test, cleanup := NewTest(t)
-	test = test.WithEnv(constants.EnableMultiIndexSwitch, 1)
+	test = test.WithEnv(constants.EnableMultiIndexSwitch, 1).WithIndex()
 	defer cleanup()
 
 	test.Krew("install", validPlugin).RunOrFailOutput()

diff --git a/integration_test/update_test.go b/integration_test/update_test.go
@@ -49,6 +49,52 @@ func TestKrewUpdate(t *testing.T) {
 	}
 }
 
+func TestKrewUpdateMultipleIndexes(t *testing.T) {
+	skipShort(t)
+	test, cleanup := NewTest(t)
+	defer cleanup()
+
+	test = test.WithEnv(constants.EnableMultiIndexSwitch, 1).WithIndex()
+	// to enable new paths in environment.NewPaths()
+	os.Setenv(constants.EnableMultiIndexSwitch, "1")
+	defer os.Unsetenv(constants.EnableMultiIndexSwitch)
+
+	paths := environment.NewPaths(test.Root())
+	test.Krew("index", "add", "foo", paths.IndexPath(constants.DefaultIndexName)).RunOrFail()
+	if err := os.RemoveAll(paths.IndexPluginsPath("foo")); err != nil {
+		t.Errorf("error removing plugins directory from index: %s", err)
+	}
+	test.Krew("update").RunOrFailOutput()
+	out := string(test.Krew("search").RunOrFailOutput())
+	if !strings.Contains(out, "\nctx") {
+		t.Error("expected plugin ctx in list of plugins")
+	}
+	if !strings.Contains(out, "foo/ctx") {
+		t.Error("expected plugin foo/ctx in list of plugins")
+	}
+}
+
+func TestKrewUpdateFailedIndex(t *testing.T) {
+	skipShort(t)
+	test, cleanup := NewTest(t)
+	defer cleanup()
+
+	test = test.WithEnv(constants.EnableMultiIndexSwitch, 1).WithIndex()
+	os.Setenv(constants.EnableMultiIndexSwitch, "1")
+	defer os.Unsetenv(constants.EnableMultiIndexSwitch)
+
+	paths := environment.NewPaths(test.Root())
+	test.TempDir().InitEmptyGitRepo(paths.IndexPath("foo"), "invalid-git")
+	out, err := test.Krew("update").Run()
+	if err == nil {
+		t.Error("expected update to fail")
+	}
+	msg := "failed to update the following indexes: foo"
+	if !strings.Contains(string(out), msg) {
+		t.Errorf("%q doesn't contain msg=%q", string(out), msg)
+	}
+}
+
 func TestKrewUpdateListsNewPlugins(t *testing.T) {
 	skipShort(t)
 	test, cleanup := NewTest(t)

diff --git a/internal/index/indexoperations/index.go b/internal/index/indexoperations/index.go
@@ -23,6 +23,7 @@ import (
 
 	"sigs.k8s.io/krew/internal/environment"
 	"sigs.k8s.io/krew/internal/gitutil"
+	"sigs.k8s.io/krew/pkg/constants"
 )
 
 var validNamePattern = regexp.MustCompile(`^[A-Za-z0-9_-]+$`)
@@ -36,6 +37,15 @@ type Index struct {
 // ListIndexes returns a slice of Index objects. The path argument is used as
 // the base path of the index.
 func ListIndexes(paths environment.Paths) ([]Index, error) {
+	if _, ok := os.LookupEnv(constants.EnableMultiIndexSwitch); !ok {
+		return []Index{
+			{
+				Name: constants.DefaultIndexName,
+				URL:  constants.DefaultIndexURI,
+			},
+		}, nil
+	}
+
 	dirs, err := ioutil.ReadDir(paths.IndexBase())
 	if err != nil {
 		return nil, errors.Wrap(err, "failed to list directory")

diff --git a/internal/index/indexoperations/index_test.go b/internal/index/indexoperations/index_test.go
@@ -22,7 +22,6 @@ import (
 	"github.com/google/go-cmp/cmp"
 
 	"sigs.k8s.io/krew/internal/environment"
-	"sigs.k8s.io/krew/internal/gitutil"
 	"sigs.k8s.io/krew/internal/testutil"
 	"sigs.k8s.io/krew/pkg/constants"
 )
@@ -49,7 +48,7 @@ func TestListIndexes(t *testing.T) {
 	paths := environment.NewPaths(tmpDir.Root())
 	for _, index := range wantIndexes {
 		path := paths.IndexPath(index.Name)
-		initEmptyGitRepo(t, path, index.URL)
+		tmpDir.InitEmptyGitRepo(path, index.URL)
 	}
 
 	gotIndexes, err := ListIndexes(paths)
@@ -71,7 +70,7 @@ func TestAddIndexSuccess(t *testing.T) {
 
 	indexName := "foo"
 	localRepo := tmpDir.Path("local/" + indexName)
-	initEmptyGitRepo(t, localRepo, "")
+	tmpDir.InitEmptyGitRepo(localRepo, "")
 
 	paths := environment.NewPaths(tmpDir.Root())
 	if err := AddIndex(paths, indexName, localRepo); err != nil {
@@ -107,8 +106,8 @@ func TestAddIndexFailure(t *testing.T) {
 	}
 
 	localRepo := tmpDir.Path("local/" + indexName)
-	initEmptyGitRepo(t, tmpDir.Path("index/"+indexName), "")
-	initEmptyGitRepo(t, localRepo, "")
+	tmpDir.InitEmptyGitRepo(tmpDir.Path("index/"+indexName), "")
+	tmpDir.InitEmptyGitRepo(localRepo, "")
 
 	if err := AddIndex(paths, indexName, localRepo); err == nil {
 		t.Error("expected error when adding an index that already exists")
@@ -151,20 +150,6 @@ func TestDeleteIndex(t *testing.T) {
 	}
 }
 
-func initEmptyGitRepo(t *testing.T, path, url string) {
-	t.Helper()
-
-	if err := os.MkdirAll(path, os.ModePerm); err != nil {
-		t.Fatalf("cannot create directory %q: %s", filepath.Dir(path), err)
-	}
-	if _, err := gitutil.Exec(path, "init"); err != nil {
-		t.Fatalf("error initializing git repo: %s", err)
-	}
-	if _, err := gitutil.Exec(path, "remote", "add", "origin", url); err != nil {
-		t.Fatalf("error setting remote origin: %s", err)
-	}
-}
-
 func TestIsValidIndexName(t *testing.T) {
 	tests := []struct {
 		name  string

diff --git a/internal/testutil/tempdir.go b/internal/testutil/tempdir.go
@@ -23,6 +23,8 @@ import (
 	"testing"
 
 	"sigs.k8s.io/yaml"
+
+	"sigs.k8s.io/krew/internal/gitutil"
 )
 
 type TempDir struct {
@@ -83,3 +85,17 @@ func (td *TempDir) WriteYAML(file string, obj interface{}) *TempDir {
 	}
 	return td.Write(file, content)
 }
+
+func (td *TempDir) InitEmptyGitRepo(path, url string) {
+	td.t.Helper()
+
+	if err := os.MkdirAll(path, os.ModePerm); err != nil {
+		td.t.Fatalf("cannot create directory %q: %s", filepath.Dir(path), err)
+	}
+	if _, err := gitutil.Exec(path, "init"); err != nil {
+		td.t.Fatalf("error initializing git repo: %s", err)
+	}
+	if _, err := gitutil.Exec(path, "remote", "add", "origin", url); err != nil {
+		td.t.Fatalf("error setting remote origin: %s", err)
+	}
+}