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

Refactor ListInstalledPlugins #558

Merged
Merged
Show file tree
Hide file tree
Changes from 3 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
10 changes: 5 additions & 5 deletions cmd/krew/cmd/list.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,16 +40,16 @@ Remarks:
the names of the plugins installed. This output can be piped back to the
"install" command.`,
RunE: func(cmd *cobra.Command, args []string) error {
plugins, err := installation.ListInstalledPlugins(paths.InstallReceiptsPath())
receipts, err := installation.GetInstalledPluginReceipts(paths.InstallReceiptsPath())
if err != nil {
return errors.Wrap(err, "failed to find all installed versions")
}

// return sorted list of plugin names when piped to other commands or file
if !isTerminal(os.Stdout) {
var names []string
for name := range plugins {
names = append(names, name)
for _, receipt := range receipts {
names = append(names, receipt.Name)
}
sort.Strings(names)
fmt.Fprintln(os.Stdout, strings.Join(names, "\n"))
Expand All @@ -58,8 +58,8 @@ Remarks:

// print table
var rows [][]string
for p, version := range plugins {
rows = append(rows, []string{p, version})
for _, receipt := range receipts {
rows = append(rows, []string{receipt.Name, receipt.Spec.Version})
}
rows = sortByFirstColumn(rows)
return printTable(os.Stdout, []string{"PLUGIN", "VERSION"}, rows)
Expand Down
6 changes: 5 additions & 1 deletion cmd/krew/cmd/search.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,10 +54,14 @@ Examples:
pluginMap[p.Name] = p
}

installed, err := installation.ListInstalledPlugins(paths.InstallReceiptsPath())
receipts, err := installation.GetInstalledPluginReceipts(paths.InstallReceiptsPath())
if err != nil {
return errors.Wrap(err, "failed to load installed plugins")
}
installed := make(map[string]string)
for _, receipt := range receipts {
installed[receipt.Name] = receipt.Spec.Version
Copy link
Member

Choose a reason for hiding this comment

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

this sounds wrong. what if two plugins with identical names from different indexes exist? it'll show both as installed.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is here to preserve the existing functionality. I'm not trying to update search to work with custom indexes in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah makes sense. That said do you mind adding a TODO so we don't forget these things? The same works below, too.

}

var matchNames []string
if len(args) > 0 {
Expand Down
6 changes: 5 additions & 1 deletion cmd/krew/cmd/update.go
Original file line number Diff line number Diff line change
Expand Up @@ -120,10 +120,14 @@ func ensureIndexUpdated(_ *cobra.Command, _ []string) error {
return errors.Wrap(err, "failed to load plugin index after update")
}

installedPlugins, err := installation.ListInstalledPlugins(paths.InstallReceiptsPath())
receipts, err := installation.GetInstalledPluginReceipts(paths.InstallReceiptsPath())
if err != nil {
return errors.Wrap(err, "failed to load installed plugins list after update")
}
installedPlugins := make(map[string]string)
for _, receipt := range receipts {
installedPlugins[receipt.Name] = receipt.Spec.Version
}

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.

this part need more comprehensive and careful refactoring.
I think we need to think heavier refactoring of this part because of

  1. plugin name collisions between indexes
  2. we ought to show index/ prefix to updated plugins as well.

Maybe let's comment out this feature (showing updated stuff) and let's add a TODO to reactivate in upcoming PRs. Not sure. @corneliusweig thoughts?


Expand Down
6 changes: 3 additions & 3 deletions cmd/krew/cmd/upgrade.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,12 +47,12 @@ kubectl krew upgrade foo bar"`,
var pluginNames []string
if len(args) == 0 {
// Upgrade all plugins.
installed, err := installation.ListInstalledPlugins(paths.InstallReceiptsPath())
installed, err := installation.GetInstalledPluginReceipts(paths.InstallReceiptsPath())
if err != nil {
return errors.Wrap(err, "failed to find all installed versions")
}
for name := range installed {
pluginNames = append(pluginNames, name)
for _, receipt := range installed {
pluginNames = append(pluginNames, receipt.Name)
}
ignoreUpgraded = true
skipErrors = true
Expand Down
19 changes: 3 additions & 16 deletions internal/installation/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,24 +25,10 @@ import (
"sigs.k8s.io/krew/pkg/index"
)

// ListInstalledPlugins returns a list of all install plugins in a
// name:version format based on the install receipts at the specified dir.
func ListInstalledPlugins(receiptsDir string) (map[string]string, error) {
installed := make(map[string]string)
receipts, err := getInstalledPluginReceipts(receiptsDir)
if err != nil {
return nil, err
}
for _, r := range receipts {
installed[r.GetObjectMeta().GetName()] = r.Spec.Version
}
return installed, nil
}

// InstalledPluginsFromIndex returns a list of all install plugins from a particular index.
func InstalledPluginsFromIndex(receiptsDir, indexName string) ([]index.Receipt, error) {
var out []index.Receipt
receipts, err := getInstalledPluginReceipts(receiptsDir)
receipts, err := GetInstalledPluginReceipts(receiptsDir)
if err != nil {
return nil, err
}
Expand All @@ -54,7 +40,8 @@ func InstalledPluginsFromIndex(receiptsDir, indexName string) ([]index.Receipt,
return out, nil
}

func getInstalledPluginReceipts(receiptsDir string) ([]index.Receipt, error) {
// GetInstalledPluginReceipts returns a list of receipts.
func GetInstalledPluginReceipts(receiptsDir string) ([]index.Receipt, error) {
files, err := filepath.Glob(filepath.Join(receiptsDir, "*"+constants.ManifestExtension))
if err != nil {
return nil, errors.Wrapf(err, "failed to glob receipts directory (%s) for manifests", receiptsDir)
Expand Down
21 changes: 7 additions & 14 deletions internal/installation/util_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,31 +33,24 @@ func testdataPath(t *testing.T) string {
return filepath.Join(pwd, "testdata")
}

func TestListInstalledPlugins(t *testing.T) {
func TestGetInstalledPluginReceipts(t *testing.T) {
tests := []struct {
name string
plugins []index.Receipt
expected map[string]string
receipts []index.Receipt
}{
{
name: "single plugin",
plugins: []index.Receipt{
receipts: []index.Receipt{
testutil.NewReceipt().WithPlugin(testutil.NewPlugin().WithName("test").WithVersion("v0.0.1").V()).V(),
},
expected: map[string]string{"test": "v0.0.1"},
},
{
name: "multiple plugins",
plugins: []index.Receipt{
receipts: []index.Receipt{
testutil.NewReceipt().WithPlugin(testutil.NewPlugin().WithName("plugin-a").WithVersion("v0.0.1").V()).V(),
testutil.NewReceipt().WithPlugin(testutil.NewPlugin().WithName("plugin-b").WithVersion("v0.1.0").V()).V(),
testutil.NewReceipt().WithPlugin(testutil.NewPlugin().WithName("plugin-c").WithVersion("v1.0.0").V()).V(),
},
expected: map[string]string{
"plugin-a": "v0.0.1",
"plugin-b": "v0.1.0",
"plugin-c": "v1.0.0",
},
},
}

Expand All @@ -66,15 +59,15 @@ func TestListInstalledPlugins(t *testing.T) {
tempDir, cleanup := testutil.NewTempDir(t)
defer cleanup()

for _, plugin := range test.plugins {
for _, plugin := range test.receipts {
tempDir.WriteYAML(plugin.Name+constants.ManifestExtension, plugin)
}

actual, err := ListInstalledPlugins(tempDir.Root())
actual, err := GetInstalledPluginReceipts(tempDir.Root())
if err != nil {
t.Fatal(err)
}
if diff := cmp.Diff(test.expected, actual); diff != "" {
if diff := cmp.Diff(test.receipts, actual); diff != "" {
t.Error(diff)
}
})
Expand Down