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

multi-index: Add default index if none exists #595

Merged
merged 2 commits into from
Apr 28, 2020
Merged
Show file tree
Hide file tree
Changes from all 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 ensureIndexesUpdated(cmd, args)
return ensureIndexes(cmd, args)
},
}

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: ensureIndexesUpdated,
PreRunE: func(_ *cobra.Command, _ []string) error { return ensureIndexesUpdated() },
}

var indexUpgradeCmd = &cobra.Command{
Expand Down
34 changes: 32 additions & 2 deletions cmd/krew/cmd/update.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,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: ensureIndexesUpdated,
RunE: ensureIndexes,
}

func showFormattedPluginsInfo(out io.Writer, header string, plugins []string) {
Expand Down Expand Up @@ -120,7 +120,37 @@ func loadPlugins(indexes []indexoperations.Index) []pluginEntry {
return out
}

func ensureIndexesUpdated(_ *cobra.Command, _ []string) error {
func ensureIndexes(_ *cobra.Command, _ []string) error {
if os.Getenv(constants.EnableMultiIndexSwitch) != "" {
klog.V(3).Infof("Will check if there are any indexes added.")
if err := ensureDefaultIndexIfNoneExist(); err != nil {
return err
}
}
return ensureIndexesUpdated()
}

// ensureDefaultIndexIfNoneExist adds the default index automatically
// (and informs the user about it) if no plugin index exists for krew.
func ensureDefaultIndexIfNoneExist() error {
idx, err := indexoperations.ListIndexes(paths)
if err != nil {
return errors.Wrap(err, "failed to retrieve plugin indexes")
}
if len(idx) > 0 {
klog.V(3).Infof("Found %d indexes, skipping adding default index.", len(idx))
return nil
}

klog.V(3).Infof("No index found, add default index.")
fmt.Fprintf(os.Stderr, "Adding \"default\" plugin index from %s.\n", constants.DefaultIndexURI)
return errors.Wrap(indexoperations.AddIndex(paths, constants.DefaultIndexName, constants.DefaultIndexURI),
"failed to add default plugin index in absence of no indexes")
}

// ensureIndexesUpdated iterates over all indexes and updates them
// and prints new plugins and upgrades available for installed plugins.
func ensureIndexesUpdated() error {
indexes, err := indexoperations.ListIndexes(paths)
if err != nil {
return errors.Wrap(err, "failed to list indexes")
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 ensureIndexesUpdated(cmd, args)
return ensureIndexes(cmd, args)
},
}

Expand Down
55 changes: 55 additions & 0 deletions integration_test/index_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ package integrationtest

import (
"os"
"regexp"
"strings"
"testing"

Expand Down Expand Up @@ -171,3 +172,57 @@ func TestKrewIndexRemoveForce_nonExisting(t *testing.T) {
// --force returns success for non-existing indexes
test.Krew("index", "remove", "--force", "non-existing").RunOrFail()
}

func TestKrewDefaultIndex_notAutomaticallyAdded(t *testing.T) {
skipShort(t)
test, cleanup := NewTest(t)
test = test.WithEnv(constants.EnableMultiIndexSwitch, 1)
defer cleanup()

test.Krew("help").RunOrFail()
out, err := test.Krew("search").Run()
if err == nil {
t.Fatalf("search must've failed without any indexes. output=%s", string(out))
}
out = test.Krew("index", "list").RunOrFailOutput()
if len(lines(out)) > 1 {
t.Fatalf("expected no indexes; got output=%q", string(out))
}
}

func TestKrewDefaultIndex_AutoAddedOnInstall(t *testing.T) {
skipShort(t)
test, cleanup := NewTest(t)
test = test.WithEnv(constants.EnableMultiIndexSwitch, 1)
defer cleanup()

test.Krew("install", validPlugin).RunOrFail()
ensureIndexListHasDefaultIndex(t, string(test.Krew("index", "list").RunOrFailOutput()))
}

func TestKrewDefaultIndex_AutoAddedOnUpdate(t *testing.T) {
skipShort(t)
test, cleanup := NewTest(t)
test = test.WithEnv(constants.EnableMultiIndexSwitch, 1)
defer cleanup()

test.Krew("update").RunOrFail()
ensureIndexListHasDefaultIndex(t, string(test.Krew("index", "list").RunOrFailOutput()))
}

func TestKrewDefaultIndex_AutoAddedOnUpgrade(t *testing.T) {
skipShort(t)
test, cleanup := NewTest(t)
test = test.WithEnv(constants.EnableMultiIndexSwitch, 1)
defer cleanup()

test.Krew("upgrade").RunOrFail()
ensureIndexListHasDefaultIndex(t, string(test.Krew("index", "list").RunOrFailOutput()))
}

func ensureIndexListHasDefaultIndex(t *testing.T, output string) {
Copy link
Member

Choose a reason for hiding this comment

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

Seems like you could just pass *ITest here as the param.

Copy link
Member Author

@ahmetb ahmetb Apr 20, 2020

Choose a reason for hiding this comment

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

🤔 why? Only testing.T seems sufficient?

For now I prefer we do list exec outside this.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I only mentioned since the string(test.Krew("index", "list").RunOrFailOutput()) is being passed to each one. I thought it might be better to put that in the ensureIndexListHasDefaultIndex function rather than relying on the caller to pass the desired output.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah got it now. I think that's fine. this way we can actually see the error from RunOrFail at the correct line number.

t.Helper()
if !regexp.MustCompile(`(?m)^default\b`).MatchString(output) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: isn't it better to put this at the package level so that the regular expression is only parsed once instead of for each time this function is called?

Copy link
Member Author

Choose a reason for hiding this comment

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

since it's only part of integration tests, I figured it doesn't matter a whole lot.

t.Fatalf("index list did not return default index:\n%s", output)
}
}