Repository: kubernetes-sigs/krew
Pull request #545 (2020-03-10 22:47:35)

Add index add command
Relatively small change. The majority of it is the test I added. I wasn't sure how to test the case where an index is added since it relies on git to clone the repo, so the only test cases I have are negative ones where the index with a name has already been configured and an invalid url is provided for the clone operation. I was thinking of using `constants.IndexURI` in a test case but didn't like that the unit test would need to be able to reach github so I didn't add it. 

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/545#" 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: Please add CLI integration tests to ./integration_test. 
ahmetb: @chriskim06 does the code handle this case?

    krew index add ../../../usr/bin [REPO]

we need sanity checks around names to ensure they don't serve path escapes.
chriskim06: I didn't think about that, I'll add those checks.
ahmetb: /lgtm
/approve

Thanks @chriskim06 for pushing it through.
Thanks @corneliusweig for thorough reviews!

I'll be making several followup PRs to clean up a few things! 
I also have a patch ready for `krew index delete`.
k8s-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-sigs/krew/pull/545#issuecomment-599808995" title="Approved">ahmetb</a>*, *<a href="https://github.com/kubernetes-sigs/krew/pull/545#" 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/index.go b/cmd/krew/cmd/index.go
index fe70f30..d401dce 100644
--- a/cmd/krew/cmd/index.go
+++ b/cmd/krew/cmd/index.go
@@ -46,6 +46,7 @@ each configured index in table format.`,
 		if err != nil {
 			return errors.Wrap(err, "failed to list indexes")
 		}
+
 		var rows [][]string
 		for _, index := range indexes {
 			rows = append(rows, []string{index.Name, index.URL})
@@ -54,9 +55,21 @@ each configured index in table format.`,
 	},
 }
 
+var indexAddCmd = &cobra.Command{
+	Use:     "add",
+	Short:   "Add a new index",
+	Long:    "Configure a new index to install plugins from.",
+	Example: "kubectl krew index add default " + constants.IndexURI,
+	Args:    cobra.ExactArgs(2),
+	RunE: func(_ *cobra.Command, args []string) error {
+		return indexoperations.AddIndex(paths.IndexBase(), args[0], args[1])
+	},
+}
+
 func init() {
 	if _, ok := os.LookupEnv(constants.EnableMultiIndexSwitch); ok {
 		indexCmd.AddCommand(indexListCmd)
+		indexCmd.AddCommand(indexAddCmd)
 		rootCmd.AddCommand(indexCmd)
 	}
 }
diff --git a/cmd/krew/cmd/root.go b/cmd/krew/cmd/root.go
index 0fc8421..4dc76f9 100644
--- a/cmd/krew/cmd/root.go
+++ b/cmd/krew/cmd/root.go
@@ -117,6 +117,7 @@ func preRun(cmd *cobra.Command, _ []string) error {
 	if err := ensureDirs(paths.BasePath(),
 		paths.InstallPath(),
 		paths.BinPath(),
+		paths.IndexBase(),
 		paths.InstallReceiptsPath()); err != nil {
 		klog.Fatal(err)
 	}
diff --git a/internal/index/indexoperations/index.go b/internal/index/indexoperations/index.go
index c66b988..4c21b35 100644
--- a/internal/index/indexoperations/index.go
+++ b/internal/index/indexoperations/index.go
@@ -16,13 +16,17 @@ package indexoperations
 
 import (
 	"io/ioutil"
+	"os"
 	"path/filepath"
+	"regexp"
 
 	"github.com/pkg/errors"
 
 	"sigs.k8s.io/krew/internal/gitutil"
 )
 
+var validNamePattern = regexp.MustCompile(`^[A-Za-z0-9_-]+$`)
+
 // Index describes the name and URL of a configured index.
 type Index struct {
 	Name string
@@ -52,3 +56,23 @@ func ListIndexes(path string) ([]Index, error) {
 	}
 	return indexes, nil
 }
+
+// AddIndex initializes a new index to install plugins from.
+func AddIndex(path, name, url string) error {
+	if !IsValidIndexName(name) {
+		return errors.New("invalid index name")
+	}
+
+	dir := filepath.Join(path, name)
+	if _, err := os.Stat(dir); os.IsNotExist(err) {
+		return gitutil.EnsureCloned(url, dir)
+	} else if err != nil {
+		return err
+	}
+	return errors.New("index already exists")
+}
+
+// IsValidIndexName validates if an index name contains invalid characters
+func IsValidIndexName(name string) bool {
+	return validNamePattern.MatchString(name)
+}


Test patch:
diff --git a/integration_test/index_test.go b/integration_test/index_test.go
new file mode 100644
index 0000000..3151419
--- /dev/null
+++ b/integration_test/index_test.go
@@ -0,0 +1,86 @@
+// Copyright 2019 The Kubernetes Authors.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package integrationtest
+
+import (
+	"os"
+	"testing"
+
+	"sigs.k8s.io/krew/pkg/constants"
+)
+
+func TestKrewIndexAdd(t *testing.T) {
+	skipShort(t)
+
+	test, cleanup := NewTest(t)
+	defer cleanup()
+
+	test.WithEnv(constants.EnableMultiIndexSwitch, 1).WithIndex()
+	if err := test.Krew("index", "add").Run(); err == nil {
+		t.Fatal("expected index add with no args to fail")
+	}
+	if err := test.Krew("index", "add", "foo", "https://invalid").Run(); err == nil {
+		t.Fatal("expected index add with invalid URL to fail")
+	}
+	if err := test.Krew("index", "add", "../../usr/bin", constants.IndexURI); err == nil {
+		t.Fatal("expected index add with path characters to fail")
+	}
+	if err := test.Krew("index", "add", "foo", test.TempDir().Path("index/"+constants.DefaultIndexName)).Run(); err != nil {
+		t.Fatalf("error adding new index: %v", err)
+	}
+	if err := test.Krew("index", "add", "foo", test.TempDir().Path("index/"+constants.DefaultIndexName)).Run(); err == nil {
+		t.Fatal("expected adding same index to fail")
+	}
+}
+
+func TestKrewIndexList(t *testing.T) {
+	skipShort(t)
+
+	test, cleanup := NewTest(t)
+	defer cleanup()
+
+	test.WithEnv(constants.EnableMultiIndexSwitch, 1).WithIndex()
+	out := test.Krew("index", "list").RunOrFailOutput()
+	if indexes := lines(out); len(indexes) < 2 {
+		// the first line is the header
+		t.Fatal("expected at least 1 index in output")
+	}
+
+	test.Krew("index", "add", "foo", test.TempDir().Path("index/"+constants.DefaultIndexName)).RunOrFail()
+	out = test.Krew("index", "list").RunOrFailOutput()
+	if indexes := lines(out); len(indexes) < 3 {
+		// the first line is the header
+		t.Fatal("expected 2 indexes in output")
+	}
+}
+
+func TestKrewIndexList_NoIndexes(t *testing.T) {
+	skipShort(t)
+
+	test, cleanup := NewTest(t)
+	defer cleanup()
+
+	test.WithEnv(constants.EnableMultiIndexSwitch, 1).WithIndex()
+	index := test.TempDir().Path("index")
+	if err := os.RemoveAll(index); err != nil {
+		t.Fatalf("error removing default index: %v", err)
+	}
+
+	out := test.Krew("index", "list").RunOrFailOutput()
+	if indexes := lines(out); len(indexes) > 1 {
+		// the first line is the header
+		t.Fatalf("expected index list to be empty:\n%s", string(out))
+	}
+}
diff --git a/integration_test/testutil_test.go b/integration_test/testutil_test.go
index 48dcabb..924eb6c 100644
--- a/integration_test/testutil_test.go
+++ b/integration_test/testutil_test.go
@@ -30,6 +30,8 @@ import (
 
 	"github.com/pkg/errors"
 
+	"sigs.k8s.io/krew/internal/environment"
+	"sigs.k8s.io/krew/internal/indexmigration"
 	"sigs.k8s.io/krew/internal/testutil"
 	"sigs.k8s.io/krew/pkg/constants"
 )
@@ -283,6 +285,15 @@ func (it *ITest) initializeIndex() {
 	if err := cmd.Run(); err != nil {
 		it.t.Fatalf("cannot restore index from cache: %s", err)
 	}
+
+	// TODO(chriskim06) simplify once multi-index is enabled
+	for _, e := range it.env {
+		if strings.Contains(e, constants.EnableMultiIndexSwitch) {
+			if err := indexmigration.Migrate(environment.NewPaths(it.Root())); err != nil {
+				it.t.Fatalf("error migrating index: %s", err)
+			}
+		}
+	}
 }
 
 func initFromGitClone(t *testing.T) ([]byte, error) {
diff --git a/internal/index/indexoperations/index_test.go b/internal/index/indexoperations/index_test.go
index 7fd07cd..a6e09cf 100644
--- a/internal/index/indexoperations/index_test.go
+++ b/internal/index/indexoperations/index_test.go
@@ -43,17 +43,7 @@ func TestListIndexes(t *testing.T) {
 
 	for _, index := range wantIndexes {
 		path := tmpDir.Path("index/" + index.Name)
-		if err := os.MkdirAll(path, os.ModePerm); err != nil {
-			t.Fatalf("cannot create directory %q: %s", filepath.Dir(path), err)
-		}
-		_, err := gitutil.Exec(path, "init")
-		if err != nil {
-			t.Fatalf("error initializing git repo: %s", err)
-		}
-		_, err = gitutil.Exec(path, "remote", "add", "origin", index.URL)
-		if err != nil {
-			t.Fatalf("error setting remote origin: %s", err)
-		}
+		initEmptyGitRepo(t, path, index.URL)
 	}
 
 	gotIndexes, err := ListIndexes(environment.NewPaths(tmpDir.Root()).IndexBase())
@@ -64,3 +54,108 @@ func TestListIndexes(t *testing.T) {
 		t.Errorf("output does not match: %s", diff)
 	}
 }
+
+func TestAddIndexSuccess(t *testing.T) {
+	tmpDir, cleanup := testutil.NewTempDir(t)
+	defer cleanup()
+
+	indexName := "foo"
+	localRepo := tmpDir.Path("local/" + indexName)
+	initEmptyGitRepo(t, localRepo, "")
+
+	indexRoot := tmpDir.Path("index")
+	if err := AddIndex(indexRoot, indexName, localRepo); err != nil {
+		t.Errorf("error adding index: %v", err)
+	}
+	gotIndexes, err := ListIndexes(indexRoot)
+	if err != nil {
+		t.Errorf("error listing indexes: %s", err)
+	}
+	wantIndexes := []Index{
+		{
+			Name: indexName,
+			URL:  localRepo,
+		},
+	}
+	if diff := cmp.Diff(wantIndexes, gotIndexes); diff != "" {
+		t.Errorf("expected index %s in list: %s", indexName, diff)
+	}
+}
+
+func TestAddIndexFailure(t *testing.T) {
+	tmpDir, cleanup := testutil.NewTempDir(t)
+	defer cleanup()
+
+	indexName := "foo"
+	indexRoot := tmpDir.Path("index")
+	if err := AddIndex(indexRoot, indexName, tmpDir.Path("invalid/repo")); err == nil {
+		t.Error("expected error when adding index with invalid URL")
+	}
+
+	localRepo := tmpDir.Path("local/" + indexName)
+	initEmptyGitRepo(t, tmpDir.Path("index/"+indexName), "")
+	initEmptyGitRepo(t, localRepo, "")
+
+	if err := AddIndex(indexRoot, indexName, localRepo); err == nil {
+		t.Error("expected error when adding an index that already exists")
+	}
+
+	if err := AddIndex(indexRoot, "foo/bar", ""); err == nil {
+		t.Error("expected error with invalid index name")
+	}
+}
+
+func TestIsValidIndexName(t *testing.T) {
+	tests := []struct {
+		name  string
+		index string
+		want  bool
+	}{
+		{
+			name:  "with space",
+			index: "foo bar",
+			want:  false,
+		},
+		{
+			name:  "with forward slash",
+			index: "foo/bar",
+			want:  false,
+		},
+		{
+			name:  "with back slash",
+			index: "foo\\bar",
+			want:  false,
+		},
+		{
+			name:  "with period",
+			index: "foo.bar",
+			want:  false,
+		},
+		{
+			name:  "valid name",
+			index: "foo",
+			want:  true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := IsValidIndexName(tt.index); tt.want != got {
+				t.Errorf("IsValidIndexName(%s), got = %t, want = %t", tt.index, got, tt.want)
+			}
+		})
+	}
+}
+
+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)
+	}
+}