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

cmd: Update each index instead of just default #588

Merged
merged 8 commits into from
Apr 13, 2020
Merged
Show file tree
Hide file tree
Changes from 6 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
2 changes: 1 addition & 1 deletion cmd/krew/cmd/install.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
},
}

Expand Down
16 changes: 3 additions & 13 deletions cmd/krew/cmd/search.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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))
Expand Down
2 changes: 1 addition & 1 deletion cmd/krew/cmd/system.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down
26 changes: 21 additions & 5 deletions cmd/krew/cmd/update.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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) {
Expand Down Expand Up @@ -102,13 +104,24 @@ 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
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.V(1).Infof("error updating index %s: %s", idx.Name, err)
Copy link
Member

@ahmetb ahmetb Apr 11, 2020

Choose a reason for hiding this comment

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

you should probably do this as klog.Warningf otherwise the user will never know (unless they use -v) what the error is.

See how we handle multiple plugin install failures in install.go.

klog.Warningf("failed to install plugin %q: %v", plugin.Name, err)

failed = append(failed, idx.Name)
}
}

fmt.Fprintln(os.Stderr, "Updated the local copy of plugin index.")

if len(preUpdateIndex) == 0 {
Expand All @@ -132,6 +145,9 @@ func ensureIndexUpdated(_ *cobra.Command, _ []string) error {
// TODO(chriskim06) consider commenting this out when refactoring for custom indexes
showUpdatedPlugins(os.Stderr, preUpdateIndex, posUpdateIndex, installedPlugins)

Copy link
Member

Choose a reason for hiding this comment

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

I’ve realized this if (+counting) isnt necessary since errors.Wrap already handles nil err. Maybe remove?

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 do that. Do you think it will look weird if this ends with the return errors.Wrapf(...)?

Copy link
Member

Choose a reason for hiding this comment

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

I think it’s fine. I just realized the check is redundant.

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 remove the check then.

if len(failed) != 0 {
return errors.Errorf("failed to update the following indexes: %s\n", strings.Join(failed, ", "))
Copy link
Member

Choose a reason for hiding this comment

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

Something else we do in install.go is that we collect the first error as returnErr and wrap+return it.

if returnErr == nil {
returnErr = err
}

if len(failed) > 0 {
return errors.Wrapf(returnErr, "failed to install some plugins: %+v", failed)
}

The reason for this is that when you wrap+return errors, when executed with -v 1 or above, we the retain ability to print full stack trace (in this case, at least for 1 error which is good enough).

}
return nil
}

Expand Down
2 changes: 1 addition & 1 deletion cmd/krew/cmd/upgrade.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
},
}

Expand Down
2 changes: 1 addition & 1 deletion integration_test/index_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
44 changes: 44 additions & 0 deletions integration_test/update_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,50 @@ 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()
os.Setenv(constants.EnableMultiIndexSwitch, "1")
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a comment like

// to enable new paths in environment.NewPaths()

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, err := ioutil.ReadDir(paths.IndexPluginsPath("foo"))
if err != nil {
t.Errorf("error reading plugins directory: %s", err)
}
if len(out) == 0 {
t.Error("no plugins in index foo after update")
}
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't you be also checking what's in default index now?
also you can use something like krew search here to find plugins in all indexes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ya good call 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should be enough to look for two plugin names in the output. For example, ctx and foo/ctx.

}

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")
}
if !strings.Contains(string(out), "failed to update the following indexes: foo") {
t.Error("expected index update to fail for foo")
Copy link
Member

Choose a reason for hiding this comment

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

Try doing this for debuggability:

%q doesn’t contain msg=%q

Copy link
Member Author

Choose a reason for hiding this comment

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

What are the two strings in the example you gave? I'm guessing the output of krew update and foo but want to make sure.

Copy link
Member

Choose a reason for hiding this comment

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

Out & “failed to ...foo”

You can export latter into a variable.

}
}

func TestKrewUpdateListsNewPlugins(t *testing.T) {
skipShort(t)
test, cleanup := NewTest(t)
Expand Down
10 changes: 10 additions & 0 deletions internal/index/indexoperations/index.go
Original file line number Diff line number Diff line change
Expand Up @@ -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_-]+$`)
Expand All @@ -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")
Expand Down
23 changes: 4 additions & 19 deletions internal/index/indexoperations/index_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand All @@ -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)
Expand All @@ -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 {
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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
Expand Down
16 changes: 16 additions & 0 deletions internal/testutil/tempdir.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ import (
"testing"

"sigs.k8s.io/yaml"

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

type TempDir struct {
Expand Down Expand Up @@ -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) {
ahmetb marked this conversation as resolved.
Show resolved Hide resolved
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)
}
}