Refactor ListInstalledPlugins #558
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/M
| 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 |
There was a problem hiding this comment.
this sounds wrong. what if two plugins with identical names from different indexes exist? it'll show both as installed.
There was a problem hiding this comment.
This is here to preserve the existing functionality. I'm not trying to update search to work with custom indexes in this PR.
There was a problem hiding this comment.
Yeah makes sense. That said do you mind adding a TODO so we don't forget these things? The same works below, too.
cmd/krew/cmd/update.go
Outdated
| 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) |
There was a problem hiding this comment.
this part need more comprehensive and careful refactoring.
I think we need to think heavier refactoring of this part because of
- plugin name collisions between indexes
- 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?
| @@ -129,6 +129,7 @@ func ensureIndexUpdated(_ *cobra.Command, _ []string) error { | |||
| installedPlugins[receipt.Name] = receipt.Spec.Version | |||
| } | |||
|
|
|||
| // TODO(chriskim06) consider commenting this out when refactoring for custom indexes | |||
There was a problem hiding this comment.
not quite commenting out, but I think we need to pass a similar Pair<indexName,plugin> here as well :) :)
There was a problem hiding this comment.
Ya pluginEntry would be available here. I was just making a note of possibly commenting out the show updated plugins feature like you mentioned in your other comment
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahmetb, chriskim06 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
Small refactor that I think will be necessary for updating search/list/upgrade to work with custom indexes.
Related issue: #483