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

Add index add command #545

Merged
merged 12 commits into from
Mar 16, 2020
13 changes: 13 additions & 0 deletions cmd/krew/cmd/index.go
Original file line number Diff line number Diff line change
Expand Up @@ -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})
Expand All @@ -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)
}
}
1 change: 1 addition & 0 deletions cmd/krew/cmd/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
86 changes: 86 additions & 0 deletions integration_test/index_test.go
Original file line number Diff line number Diff line change
@@ -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")
}
corneliusweig marked this conversation as resolved.
Show resolved Hide resolved
if err := test.Krew("index", "add", "foo", test.TempDir().Path("index/"+constants.DefaultIndexName)).Run(); err != nil {
t.Fatalf("error adding new index: %v", err)
}
corneliusweig marked this conversation as resolved.
Show resolved Hide resolved
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()
corneliusweig marked this conversation as resolved.
Show resolved Hide resolved
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)
}

ahmetb marked this conversation as resolved.
Show resolved Hide resolved
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))
}
}
11 changes: 11 additions & 0 deletions integration_test/testutil_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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) {
corneliusweig marked this conversation as resolved.
Show resolved Hide resolved
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) {
Expand Down
24 changes: 24 additions & 0 deletions internal/index/indexoperations/index.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
corneliusweig marked this conversation as resolved.
Show resolved Hide resolved
if _, err := os.Stat(dir); os.IsNotExist(err) {
return gitutil.EnsureCloned(url, dir)
ahmetb marked this conversation as resolved.
Show resolved Hide resolved
} else if err != nil {
return err
}
return errors.New("index already exists")
ahmetb marked this conversation as resolved.
Show resolved Hide resolved
}

// IsValidIndexName validates if an index name contains invalid characters
func IsValidIndexName(name string) bool {
return validNamePattern.MatchString(name)
}
117 changes: 106 additions & 11 deletions internal/index/indexoperations/index_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand All @@ -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)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO, you should also test that AddIndex has actually done something.

Copy link
Member

Choose a reason for hiding this comment

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

To do that, you can have a combined Add+List scenario.

Copy link
Member

Choose a reason for hiding this comment

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

That way you can also move away from doing manual mkdir/clone in the test code.

Copy link
Contributor

Choose a reason for hiding this comment

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

@chriskim06 What's your opinion about using the ListIndex machinery for the assertion?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure I can change it to use that.

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,
},
}
ahmetb marked this conversation as resolved.
Show resolved Hide resolved
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) {
ahmetb marked this conversation as resolved.
Show resolved Hide resolved
t.Helper()

if err := os.MkdirAll(path, os.ModePerm); err != nil {
t.Fatalf("cannot create directory %q: %s", filepath.Dir(path), err)
}
ahmetb marked this conversation as resolved.
Show resolved Hide resolved
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)
}
}